Repository navigation
[DISCUSSION] Rethinking Code Review and Testing in the Agent Era #23839
Description
Motivation
It seems large PRs generated by AI is becoming a norm nowadays. SQL query engine is highly structured machine that the output is predictable from the given inputs. I think it is possible that we could build a testing framework that every AI generated code changes could run against the testing machine and we have highly confidence to ship the changes.
#23738 is an example recently which has 4500+ LoC diffs from @Xuanwo. I believe such trends will continue, presenting an opportunity for us to review how we can conduct code reviews in agent era.
What we expect after running testing framework
- Correctness
- No Performance regression
- Clean crate separation (logical plan stuffs doesn't go to physical layer and vice versa) Great crate design so it has clear responsibility and not ends up the mess.
What we have
- Correctness - sqllogictest / datafusion-fuzzer
- Performance - benchmark bot
- Documentation to tell AI the purpose of each crate / component
What are the GAPs
-
Correctness coverage
I think we need more test coverage for correctness that gives us high confidence to ship the code if the changes pass the test (e.g., catching silent semantic bugs in complex logic without relying solely on manual test writing). -
Benchmarking for agents
I'm thinking of more benchmark coverage that is easy to run and reproduce locally (assume most contributors run benchmark in their laptop) and also cost efficient for benchmark bot. -
Architecture & System Design
It should be carefully reviewed by reviewers (human). We might need tools to help us easily understand whether changes fit our system design or maybe we need spec in details to tell AI to follow the purpose of each components
Objective
This discussion is to gather ideas for how code review could be like in agent era, and what gaps are left for us to achieve the goals. I hope we could ship large PRs without carefully review by human but with high quality code one day.
Activity
alamb commentedon Aug 2, 2026
Thanks for filing this @jayzhan211
Yes, 100% agree.
Another gap I think is in how do we find what is most importnt to review (aka use our review bandwidht)
I think the sqllogictesting framework is the best we have in this regard. Running more such tests I think is scalable and adds good end to end coverage.
I have found LLms are quite good at creating sqllogictests, so maybe we can spend more time having them expand our existing coverage. For example I have been trying to do this with
This is good -- I tink our bench.sh script is reasonable for this, and @Omega359 's work in this ticket I think will help
I think the idea of formalizing / documenting (for humans and agents) what we are looking for in reviews would probably help a lot. We already have https://github.com/apache/datafusion/blob/main/AGENTS.md and https://github.com/apache/datafusion/tree/main/.ai/skills (for ffi that @timsaucer added).
Perhaps we can add something similar for performance related or other skills (e.g. that if changing join logic tests should always include nulls, etc).
Another idea could be to add a review skill that tries to explain what we are looking for in each PR
kumarUjjawal commentedon Aug 2, 2026
Use this at work, very effective. The skill can be used to catch cases which are easy to spot and more general in nature.
alamb commentedon Aug 2, 2026
Awesome -- here is an initial attempt at documenting it
Docs: Update PR template to ask for user-visible rationale (apache#24053
HairstonE commentedon Aug 5, 2026
We could add cargo-mutants to the checklist an agent follows after making changes. Here are the docs. Essentially, it injects small changes into the source code (a flipped comparison, a swapped return value) and checks whether the tests catch them. It gives some assurance that the tests are robust, if a mutant is uncaught then the agent and developer can iterate.
Something like,
cargo mutants --in-diff <diff> --package <crate>keeps the scope to just the local change set so it can be run before a PR.alamb commentedon Aug 5, 2026
I am a big fan of "ablation testing" -- where you remove the new code that is added and run the tests and the tests should fail.
This ensures:
2010YOUY01 commentedon Aug 10, 2026
This is a good idea, I have thought about it and there are two challenges if we want to implement it:
kumarUjjawal commentedon Aug 10, 2026
I was experimenting this locally and I agree with you here. I will do some experiment maybe ask codex to suggest the best method without the overhead.
Docs: Add PR review guide (apache#24051)
Docs: Update PR template to ask for user-visible rationale (apache#24053
Docs: Add PR review guide (apache#24051)
HairstonE commentedon Aug 18, 2026
There is more nuance to using cargo mutants than I initially thought. The constraint is runtime.
Against unit tests it's fine. The suite is fast, so a contributor can run mutants on their diff and get through it. Against slt it isn't. slt is slow enough that running it once per mutant would take hours to over a day to run.
A fix could be a coverage map from each mutant to the slt files that touch its path, so you only run those. That cuts runtime, but now that map needs to be maintained. I don't think that is worth the trouble.
Running mutants against unit tests only is much faster but some "uncaught" mutants are actually caught by slt tests and others are actual gaps. Differentiating them needs a certain amount of codebase familiarity.
A possible solution to this issue would be to have only the surviving mutants run against the slt suite. Running cargo mutants would have two phases.
Phase 1 — fast filter (unit tests only, on the diff):
Phase 2 — run slt on only the survivors:
Phase 2 could still take quite a bit of time depending on how good the existing unit suite is. If there are too many survivors then this falls back to being too much to do.
This feels like a decent option for ensuring correctness coverage but could be too heavyweight to have an agent/contributor/both run for every PR. Everyone please let me know your thoughts!
jayzhan211 commentedon Sep 18, 2026
Maybe you could describe what Brain Scanner is and how it helps DataFusion?
kumarUjjawal commentedon Sep 18, 2026
Hi @kevin-lozada-santos I appreciate that you are trying to help but this post seems like a promotion which is not ideal for a discussion.
alamb commentedon Sep 18, 2026
Since DataFusion is all open source, why not just run your tool and then open an
[DISCUSSION]issue with a link for anyone to check it if they wanted. Better yet, perhaps review your tool's results first and see if you can find some insight of value that is worth a ticket or somethingalamb commentedon Oct 22, 2026
FWIW I am trying a new idea to label PRs from authors that made PRs in over 20 other repositories int he last 30 days
label_prolific.py
To be clear there is nothing wrong with this, though I will likely personally prioritize such PRs lower in my review queue
There are quite a few
Labeling #25561 (DeviousCardi): fix: do not evaluate a volatile coalesce or nvl argument twice
Labeling #25624 (tayfuryldz): fix: preserve dynamic filter identity across FFI
Labeling #25647 (Shoryamishra61): fix: propagate projection benefit through extension nodes in OptimizeProjections
Labeling #25772 (mrhard9090): fix: keep nested inner joins with a different null equality in EliminateCrossJoin
Labeling #25971 (mrhard9090): fix: hash every NaN like the canonical NaN in Spark xxhash64
Labeling #26006 (patrickswedish): fix: route sort-merge semi/anti/mark key equality through JoinKeyComparator
Labeling #26010 (mrhard9090): fix: evaluate sort-merge join filters that reference no columns
Labeling #26011 (mrhard9090): fix(spark): array_contains float equality matches Spark
Labeling #26025 (cargopete): docs: add nuthatch to known users
Labeling #26031 (efegokdemir): fix: remap nested TopK filter expressions before their children
Labeling #26040 (Prajwal-k-tech): docs: explain dialect requirement for lambda examples
Labeling #26041 (Prajwal-k-tech): docs: explain date_bin daylight saving behavior
Labeling #26042 (Prajwal-k-tech): fix: preserve outer join equivalences for null rows
Labeling #26045 (mikamikasuki): fix: validate temporal display formats
Labeling #26049 (mikamikasuki): fix: preserve timezone casts in timestamp comparisons
Labeling #26056 (mikamikasuki): fix: reset aggregate dynamic filters between recursive iterations
Labeling #26059 (mikamikasuki): fix: preserve unassigned inner join keys
Labeling #26060 (mikamikasuki): fix: preserve DISTINCT semantics in lateral decorrelation
Labeling #26146 (entity-0-0-2): fix: avoid sort-based joins for nested float join keys with negative zeros
Labeling #26151 (jdymitarai): fix: downcast to FixedSizeBinaryArray in CASE bytes lookup table
Labeling #26198 (Shahzeb-DataEngineer): fix: validate join_integer_prefilter_min_pruning_ratio at SET time
Labeling #26211 (jdymitarai): fix: cast outer projection expr to declared field type in SingleDistinctToGroupBy
Labeling #26246 (nikolas-sapa): docs: add NestedLoopJoinExec metrics to user-guide metrics page
Labeling #26291 (Harshil-1603): docs: show runnable query examples in API documentation
Labeling #26294 (Harshil-1603): docs: explain timestamp timezone comparisons
Labeling #26312 (ozenyunus35): refactor: share the REPLACE and validation steps of the wildcard arms in project_with_validation
Labeling #26344 (ozenyunus35): fix(spark): return NULL from parse_url for a malformed URL unless ANSI
Labeling #26345 (ozenyunus35): fix(spark): decode unbase64 input like Spark's MIME Base64 decoder
Labeling #26346 (ozenyunus35): fix(spark): format_string %s of a DECIMAL and %g like Java's Formatter
Labeling #26350 (DeviousCardi): fix: handle aggregate name collisions from casts
Labeling #26373 (rawsun007): docs: fix SQL examples that the default dialect cannot parse
Labeling #26394 (spicysharkco): fix(spark): normalize -0.0 in atan2 like Spark
Labeling #26432 (ump45nose): fix: return no rows from TopK with LIMIT 0 instead of panicking
Labeling #26443 (ahlag): refactor(unparser): move the Join arm of select_to_sql_recursively into join_to_sql
Labeling #26477 (paoValle): fix: plan ORDER BY on a source column hidden by a select alias
Labeling #26482 (choksi2212): fix: return NULL from bit_xor when a sliding frame has no non-NULL values
Labeling #26489 (Shahzeb-DataEngineer): refactor(spark): deduplicate bucket computation in width_bucket_interval_mdn_exact
Labeling #26494 (charan-rathore): fix: do not treat float min==max Parquet statistics as constant columns
Labeling #26529 (ahlag): fix: plan TRY_CAST struct casts by field name like CAST
Labeling #26531 (ump45nose): perf: create DiskManager Directories mode temp dirs on first spill
Labeling #26551 (princejain756): fix(optimizer): preserve correlated columns when removing filter above SubqueryAlias
Omega359 commentedon Oct 22, 2026
I wonder if it should be labelled 'Unlimited AI Budget' ;)
xudong963 commentedon Oct 22, 2026
The signal makes sense to me :D