sunchao commented on PR #6076: URL: https://github.com/apache/datafusion-comet/pull/6076#issuecomment-5847801204
Re-reviewed head **`72f12f2` with five independent agents**. **One P2 Spark-compatibility issue remains; I would hold approval.** **[P2] Apply the corrected merge to empty partials too** — [welford.rs:83](https://github.com/apache/datafusion-comet/blob/72f12f2cb71c7130a3e842b302a369b256a0ba15/native/spark-expr/src/agg_funcs/welford.rs#L83). With one partial containing **100 copies of `1e155`**, followed by an all-null partial for the same group: | Implementation | Population variance/covariance | |---|---:| | Spark 4.1.3 | `NaN` | | Previous PR revision | `NaN` | | Current native accumulators | `0` | The new arithmetic preserves zero variance during the first merge. Spark then processes the empty partial, where an intermediate multiplication overflows before multiplying by zero, producing `NaN`. Comet’s existing scalar/grouped variance and covariance callers skip that partial. This is specifically a compatibility regression at extreme finite magnitudes; zero is mathematically reasonable. Apply the canonical merge to zero-count partials too, and add coverage for both partition orders. I independently reproduced the native results using the actual modules; another agent confirmed Spark’s behavior through repeated SQL executions. Elsewhere: - **Correctness:** The original nearby-value bug and the previously reported nonempty-merge issue are fixed. Central-moment versus Pearson routing matches Spark. - **Performance:** No new Spark fallback, buffers, or per-group allocations. Focused kernel measurements found no material regression; no whole-query speedup is established. - **Design and complexity:** The small update-mode enum represents a necessary numerical distinction. No additional abstraction concerns. **Validation:** 35 native module tests passed. Separately, 46,212 kernel comparisons against actual Spark Catalyst expressions found no semantic mismatch; the finding above concerns callers skipping the kernel. [CI](https://github.com/apache/datafusion-comet/actions/runs/35847885378) passed 1,603 Rust tests and 989 Spark 4.1 execution tests, including both new regressions. CI tested a merge commit whose changed native files and new regression block match this head. Dedicated SQL/macOS suites were skipped. Local validation did not include a full Comet/Spark integration run because of disk constraints. -- 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]
