andygrove opened a new issue, #6399: URL: https://github.com/apache/datafusion-comet/issues/6399
## What / Why I'd like to go through every PR that went into 1.1.0 and look for regressions against 1.0.0, specifically the kind a reviewer could have caught from the diff. That covers wrong or silently different results, new failures, a query that used to fall back to Spark now running natively and getting it wrong, lost native coverage, slower default paths, and config or API behavior changes. Bugs that were already in 1.0.0 are out of scope. For each regression found, I also want to write down why review missed it, so the answer can go back into the review checklists and the `review-comet-*` skills. The range is `af534e0fa..branch-1.1`, where `af534e0fa` is the merge base of `branch-1.1` and the `1.0.0` tag. That is 399 PRs at 1.1.0-rc1, not counting #5192, which shipped in 1.0.0 as #5261. 292 of them touch production code. The other 107 only change docs, CI, tests or the Spark diffs. This is too much for one sitting, so I've split it into phases that can run on separate days. Claude agents do the per-PR reading: Sonnet for the cheap passes and Opus for the deep reviews. I'll post the results of each phase as a comment here. ## Phase 0: scope and script-only checks (done) - The TPC-DS golden plans show no lost native coverage. I compared all 574 effective plans between `1.0.0` and `branch-1.1`, per Spark version and following the suite's fallback chain. 26 gained native operators and none lost any. - The dependency upgrades are DataFusion 54.1 → 55.1, arrow and parquet 58.4 → 59.3, opendal 0.57 → 0.58.2 and iceberg-rust 0.10.0 → 0.10.1. Upstream behavior changes won't show up in our own diffs, so they get their own review in Phase 3. - Blaming the lines that each `fix:` PR changed gives 86 links where the fixed code was written by another PR from the same window: 78 fixes in 1.1.0 and 8 on `main` after the cut. They point at 54 PRs, and 17 of those needed two or more fixes. Blame is noisy, because a fix can touch code that a refactor only moved, so Phase 1 confirms them. - Five fixes that merged to `main` after the cut touch code from this window and are not on `branch-1.1`: #6261 (touching #6128's code), #5880 (#5497), #5403, #5846 and #5169. The last three look like weak links. #5880 was on my backport list for rc1 and didn't make it. I think its bug predates 1.0.0 and only affects metrics, but Phase 1 should confirm that. ## Phase 1: confirm the fix → origin links (Sonnet) - [ ] Start with the five fixes that aren't on `branch-1.1`. If any of them fixes a regression from this window, 1.1.0 ships it. - [ ] For each of the 86 links, decide whether the origin PR introduced the bug that the fix addresses, and whether the bug was absent in 1.0.0. The output is the list of regressions that got past review and were caught after merge, plus any backport candidates for another RC or 1.1.1. The confirmed origins are also the known answers for checking the Phase 2 triage. ## Phase 2: triage the 292 production PRs (scripts, then Sonnet) - [ ] Build one packet per PR: the production diff, the PR description, who reviewed it, which CI tiers ran, and any later fixes to its lines. - [ ] Triage in batches of about 12, asking whether the PR changes behavior on a path that already ran in 1.0.0. That means new defaults, removed guards or fallbacks, an expression or operator that is now native by default, changed semantics, config parsing, error handling, or a perf rewrite of a default-path kernel. Anything unclear gets escalated. - [ ] Check the triage against the origins confirmed in Phase 1. If it misses more than one or two, rerun it with Opus. ## Phase 3: deep review of the escalated PRs (Opus) One area per day, each a batch of about 15 PRs: - [ ] Expressions and casts - [ ] Planner, operators and shuffle - [ ] Scans: Parquet, Iceberg and Delta - [ ] Memory, FFI, config and shims - [ ] The DataFusion 55.1 and arrow/parquet 59.3 changes to APIs we use Each review uses `review-comet-pr` plus the matching area skill, and compares behavior at `1.0.0` and rc1. Every finding needs a reproducer and a sentence on why review missed it. The checklist includes the misses we've seen before: a stale rebase undoing a refactor on `main`, a new sibling class missing a guard its predecessor had, a loud failure turned into a silent wrong answer, timestamp label drift, sliced arrays across FFI, the two shuffle paths disagreeing, and tests that pass without testing anything (cache-vs-cache comparisons, assertions against the initial AQE plan). ## Phase 4: verification (Sonnet, after each Phase 3 batch) - [ ] Build `1.0.0` and 1.1.0-rc1 once, then run each reproducer on both. A finding counts as a regression only if 1.0.0 gets it right and 1.1.0 doesn't, or if a loud failure became a silent wrong answer. Perf findings need evidence in the code, and only the high-impact ones get benchmarked. ## Phase 5: report - [ ] File each confirmed regression as its own issue with the `regression` label and link it here, with a recommendation: RC blocker, 1.1.1, or release note. - [ ] Summarize why review missed them, for example no human review, no Spark SQL or Iceberg suite run, or tests that only covered the new path, and turn that into updates to the `review-comet-*` skills. Rough sizes in agent turns: Phase 1 about 180, Phase 2 about 650, Phase 3 about 2,000 across its batches, and Phase 4 about 200 plus build and test time. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
