sunchao commented on PR #5420: URL: https://github.com/apache/datafusion-comet/pull/5420#issuecomment-5443492968
Updated in [6dca7f116](https://github.com/apache/datafusion-comet/commit/6dca7f11670ca7ed4f7a1ec69731245d77497bc2). I worked through the review points and added the wide-decimal routing benchmark, carried the directional buffer policy from #5421, and corrected the grouped-AVG explanation. The updated description includes current validation and measured costs, with earlier validation clearly identified as historical. 1. **Scope beyond AVG.** Yes, the hash-key check affects every native shuffle whose decimal key has precision greater than 18. In `auto` mode, eligible queries use columnar shuffle and can retain native aggregates. In `native` mode, they use Spark shuffle, which can also require aggregate fallback. Wide decimal payloads and range keys are not excluded by this hash-key check. The description and existing tuning guidance now make that distinction explicit. 2. **Hash compatibility versus test determinism.** This is a real partition-assignment mismatch, also tracked in #3079. Spark hashes wide decimals through `BigInteger` bytes, while the current native path uses the fixed-width little-endian integer representation. Decimal partial-sum overflow depends on which values meet in a partition, so matching the final numeric type is insufficient. The guard is not just a way to stabilize one test, and I kept it. 3. **Splitting the changes.** The planner repair is already separated as #5421. I kept the AVG state repair, admission restrictions and wide-key partitioning change together because the `AVG(DISTINCT ...)` regression needs compatible partial buffers and partition assignments. Splitting those into independently mergeable fixes would leave that regression incomplete. The unrelated, pre-existing ObjectHash case now has a separate issue, #5509. 4. **Focused shuffle tests.** The existing `wide decimal hash keys use Spark-compatible shuffle partitioning` regression covers precision 18/19/38 across `native`, `auto` and `jvm`, together with payload/range/single-partition controls. It passed again in the current 47-test run. The new benchmark also checks actual input partition counts, results and routes rather than inferring execution from configuration. 5. **Migration guide.** The migration guide excludes correctness and native-admission changes from its scope. The tuning guide is the existing home for the shuffle-mode behavior, so I retained that explanation there and expanded the PR description rather than adding a migration flag or suggesting users restore incompatible hashing. 6. **Measured performance.** For 1,048,576 rows and 10,000 `DECIMAL(38,2)` groups, the four-process ABBA run measured 137→501.5 ms in `native` mode and 137→188.5 ms in `auto` mode, averaging the two run means for each variant. All eight variant/mode comparisons matched all Spark results and asserted their routes. This isolates the guard by removing only that check while keeping the same current native library. It is not the entire PR against its base. The description includes both raw run means and the resulting plans. The cost is material, especially when `native` mode restores Spark aggregation, but the faster incompatible route is not a safe alternative. 7. **Relationship to #5421.** #5421 remains the prerequisite and should land first. Its latest fix conservatively rejects native AVG partials before Spark Final because that branch still emits `(NULL, 0)` for never-updated partials. This PR repairs that state and explicitly enables non-decimal NativePartialToSparkFinal again. Decimal buffers remain excluded. The positive counterpart passed 12 mixed-route combinations plus six native controls with this branch's own rebuilt library. I have not treated #5421's temporary AVG restriction as appropriate for the repaired implementation. 8. **Why the high-precision guard selects ungrouped AVG.** The original wording was too broad. Ordinary Spark hash aggregation stores grouped decimal buffers in `UnsafeRow`, whose precision checks differ from generated global aggregation's local buffer. That explains the intended guard but does not prove all grouped implementations equivalent. The description now names the representation and qualifies the ObjectHash exception. 9. **SortAggregateExec.** Spark sort aggregation is already outside this native conversion path. The two ANSI controls in the earlier review probe remained in Spark and matched Spark results. No additional admission guard is needed for that path. 10. **ObjectHashAggregateExec and spill.** This concern is valid as a separate existing issue. `SpecificInternalRow` can preserve a wider temporary decimal sum, including after ObjectHash falls back to sort, while native grouped AVG can lose it earlier. I filed #5509 with the `0.6, 0.6, -0.4` counterexample, an all-null group, default/forced-spill checks and exact native base/head evidence showing the discrepancy predates this PR. The description no longer claims that grouping alone establishes compatibility. 11. **All-null and overflow groups.** The existing Rust `grouped_overflow_with_zeroed_null_count_is_not_empty` test and Scala `grouped decimal AVG preserves overflow across columnar shuffle` regression cover the distinction, including the all-null group alongside overflow. I retained them instead of adding duplicate tests. Their earlier validation is identified separately from this follow-up's focused run. 12. **Return in windowExprToProto.** The return is in a regular match within the method, not a closure, so it is not a Scala non-local return. I did not extract a helper solely for that concern. The current optimized native build, full Spark 4.1.3 JVM reactor/style checks, and all 47 focused Scala tests pass. The full historical cross-version suite was not rerun for this follow-up. CI on the pushed revision is still separate from these local results. -- 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]
