andygrove commented on issue #6399:
URL:
https://github.com/apache/datafusion-comet/issues/6399#issuecomment-5893428164
Phase 1 is done. Sonnet agents checked all 83 fix → origin links from the
Phase 0 blame against the `1.0.0` tag, and I spot-checked the verdicts that
mattered. That is 86 links minus 3 that were a `main` commit and its
`branch-1.1` backport.
Seven are regressions, and all seven were fixed before 1.1.0-rc1. They're
listed in #6402 with the introducing PR, the fix, and why review missed each
one. None of the seven introducing PRs was backported to `branch-1.0`, and all
seven fixes are in rc1. The five fixes that merged to `main` after the cut
without reaching `branch-1.1` (#6261, #5880, #5403, #5846 and #5169) all fix
bugs that were already in 1.0.0, so none of them needs a backport as a
regression fix.
Of the other 76, 45 fixed bugs that were already in 1.0.0. There the blame
link was noise, because the fix touched lines that a PR in the window had only
moved or edited. 28 were bugs in functionality that is new in 1.1.0 and didn't
break anything that worked in 1.0.0. Most of those are in the native Iceberg
writer, the native Celeborn shuffle and the native in-memory cache, which are
all off by default. The last 3 weren't fixes for a defect. I overrode two
regression verdicts:
- #6098: the shuffle schema cache that #5809 added had a wrong eviction
order and no byte cap, but 1.0.0 had no cache and nothing that worked there
broke.
- #6198: a new memory-overhead warning from #6054 fired when it shouldn't.
That's noise, not a regression.
The reasons review missed the seven fall into three groups:
- CI coverage. The pull request CI runs the Comet suites on the default
Spark profile only and skips the Iceberg suites. Iceberg's
`TestForwardCompatibility` caught #5262's break after merge, and the nightly
Spark 3.4 job caught #6041's. No CI profile could have caught #4775's, because
CI builds only the newest patch of each Spark line and the behavior it got
wrong changed in earlier patches.
- New native paths on by default. #4870 documented its signed-zero rank
mismatch in the compatibility guide but still enabled `WindowGroupLimitExec` by
default, instead of gating it behind `allowIncompatible`. #4775 has the same
shape.
- Tests that covered the targeted case but not its neighbor:
- #5602 tested case-insensitive duplicate fields but not byte-identical
ones.
- #5314 tested the default scheme list but not one naming only `s3`.
- #5368 tested incompatible schemas but not two operators labelling UTC
differently.
The seven introducing PRs (#5602, #5262, #5314, #5368, #6041, #4870 and
#4775) are the known answers for checking the Phase 2 triage.
--
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]
