andygrove commented on issue #6399:
URL:
https://github.com/apache/datafusion-comet/issues/6399#issuecomment-5936927315
Phase 5: what to do with each regression, and why review missed them.
## What to do with each regression
rc2 comes from `branch-1.1`, where #6449 already fixes #6424 and #6464. For
the other 11:
| Regression | Effect
| Fix | Recommendation
|
| ------------------------------------ |
-------------------------------------- | ------------------------------------ |
--------------------------------------------------------- |
| #6423 `regr_*` | wrong results
| #6451 (approved), backport #6489 | rc2
|
| #6426 Iceberg temporal functions | wrong results for rare pre-1970
inputs | #6456 (approved), backport #6486 | rc2
|
| #6334 native `IF` | error
| #6458 (approved), backport #6491 | rc2
|
| #5507 `WindowGroupLimit` | wrong results for nested float keys
| #6468 (merged), backport #6487 | rc2
|
| #6466 adaptive partial aggregation | slower
| #6474 (merged), backport #6488 | rc2
|
| #6505 `_metadata` block columns | wrong results
| #6510 (approved), backport #6511 | rc2
|
| #6425 DSv2 decimal functions | wrong results
| #6455 (needs review), backport #6490 | rc2 if it's reviewed in time,
otherwise release note |
| #5701 `array_distinct`/`array_union` | wrong results with `-0.0`
| #5750 (changes requested) | rc2 if a narrow fallback is ready,
otherwise release note |
| #6506 empty-file conversion error | error, rare
| #6515 (needs review) | release note, fix in 1.1.1
|
| #6254 spilled aggregate replay | error under memory pressure
| upstream apache/datafusion#25814 | release note, fix in 1.1.1
|
| #6504 Iceberg nested-field evolution | error
| none yet | release note, fix in 1.1.1
|
The fixes I'd take in rc2 are small, and each either falls back to Spark or
restores 1.0.0's behavior. All of them except #6425 are approved or merged. The
draft release notes in #6469 have a checked workaround for every entry, so
anything that misses rc2 is covered there. #6515 changes when scan errors are
raised, so I'd rather it had some time on `main` than went in at the last
minute.
## Why review missed them
Across all 20 regressions, the 7 fixed before rc1 and the 13 that shipped in
it, the same few things come up:
- Tests used the inputs the author had in mind, not the ones where the new
path differs from Spark. This is by far the biggest group: `-0.0` and NaN
(#5469, #5507, #5701), a constant like 0.1 merged across partitions (#6423),
pre-1970 unit boundaries (#6426), a batch where every row takes one branch
(#6334), output sliced past the first batch (#6464), files Spark splits or that
are empty (#6505, #6506), data that defeats a heuristic (#6466), and an
asymmetric config (#5825).
- A change removed or widened a guard without asking what else it was
shielding: the Iceberg null-check fallback (#6504), the codegen dispatcher
catch-all (#6424, #6425), and native map literals (#6334).
- A known divergence shipped on a default path. #4870 documented its
signed-zero gap and still enabled the operator (#5469), and #5262 switched the
`array_distinct` and `array_union` tests to `ignore` instead of falling back
(#5701).
- Tests compared against something other than Spark: Comet's own accumulator
for `regr_*` (#6423), and iceberg-rust instead of Iceberg Java for the temporal
functions (#6426).
- CI couldn't see it. The pull request tier runs only the default Spark
profile (#6156), CI builds only the newest patch of each Spark line (#6042),
and nothing runs Iceberg's forward-compatibility tables (#5759) or a spill
under a tight memory pool (#6254).
- A dependency upgrade was reviewed as an API migration rather than as a set
of behavior changes (#5701, #6254, #5759).
- One review asked for a supported off switch, and the PR merged without it
(#6466).
Ten of the 18 PRs that introduced these had a single approver.
#6516 turns these lessons into checks in the `review-comet-pr`,
`review-comet-expression-pr` and `review-comet-memory-pr` skills.
--
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]
