andygrove opened a new pull request, #5151: URL: https://github.com/apache/datafusion-comet/pull/5151
## Which issue does this PR close? Backport of #5114 (merged to `main` as `4c3f04805`) onto `branch-1.0`. Part of #5112, and carries the fix for #5148 into the 1.0.0 release branch. ## Rationale for this change Isolated benchmarking in #5112 shows the native columnar-to-row converter is 3.7x slower per row than the JVM `CometColumnarToRowExec` at the default 8192-row batch size, and up to 15.7x slower for small batches, because the native path carries roughly 10us of fixed JNI and FFI cost per batch. End-to-end microbenchmarks and the TPC-DS SF1000 comparison in #4440 show no measurable benefit from the native operator, and the original GC-pressure rationale no longer holds since #3367 made the converter deep-copy every row to the JVM heap. Real workloads that reach C2R with small batches (selective filters, small shuffle partitions, broadcast dimension tables) regress today on `branch-1.0`, so shipping 1.0.0 with the native converter on by default would ship that regression. The safer default for the release is the JVM implementation, which is also what Comet used before #3299. Users who want the native path can still opt in with `spark.comet.exec.columnarToRow.native.enabled=true`. ## What changes are included in this PR? A `git cherry-pick -x` of `4c3f04805`. Everything except four plan-stability golden files applied without conflict; see the conflict note below. - Change the default of `spark.comet.exec.columnarToRow.native.enabled` from `true` to `false`, and update the config doc to mention the per-batch JNI cost. - Update the `CometNativeColumnarToRowExec` scaladoc: it claimed zero-copy variable-length data and reduced GC pressure, neither of which holds since the #3308 fix. - Fix the DPP subquery-broadcast rewrite in `CometExecRule` to also strip the JVM `CometColumnarToRowExec`. Unlike `CometNativeColumnarToRowExec`, it is `CodegenSupport`, so by the time the rule sees the subquery plan it has been collapsed into `WholeStageCodegenExec(CometColumnarToRowExec(InputAdapter(cometPlan)))`. Without this the rule failed to recognize the Comet child, `SubqueryBroadcastExec` was never converted to `CometSubqueryBroadcastExec`, and DPP broadcast exchange reuse stopped working. - Regenerate the TPC-DS plan-stability golden files: the transition node in every affected plan changes from `CometNativeColumnarToRow` to `CometColumnarToRow`. - Update `dev/diffs/*.diff` so the Spark `SubquerySuite` partition-pruning assertion accepts the whole-stage-codegen-wrapped `CometColumnarToRowExec` shape in addition to `CometNativeColumnarToRowExec`, and so `CachedBatchSerializerNoUnwrapSuite` looks through the `WholeStageCodegenExec` before classifying the cached plan's transition node. - Fix a pre-existing decimal `avg` bug (#5148) that this change exposes: in ANSI mode, an ungrouped `avg` over an empty or all-null decimal input raised `[ARITHMETIC_OVERFLOW] Overflow in sum of decimals` instead of returning NULL, because `AvgDecimalAccumulator::merge_batch` derives `is_empty` from the nullness of the merged counts and a global partial aggregate over zero rows emits `sum = NULL, count = 0` with a non-null count. `evaluate` now guards that check with `count > 0`, matching the neighbouring `is_not_null` check and the `GroupsAccumulator` path. - Update the plan documentation (`understanding-comet-plans.md`, `installation.md`) so example plans show `CometColumnarToRow`, and the operator reference entry for `CometNativeColumnarToRow` no longer claims zero-copy variable-length handling and records that it is now opt-in. ### Conflict resolution Four golden files conflicted, all for the same reason: `main` carries #5139 (`chore: stop enabling incompatible casts in plan stability suite`), which added `[COMET-INFO: ...]` cast annotations to those plans, and `branch-1.0` does not. - `approved-plans-v1_4/q13/extended.txt` - `approved-plans-v1_4/q44/extended.txt` - `approved-plans-v1_4-spark3_5/q44/extended.txt` - `approved-plans-v1_4-spark4_0/q44/extended.txt` Resolved by keeping the `branch-1.0` plan text and applying only this PR's change to it, the `CometNativeColumnarToRow` to `CometColumnarToRow` rename. `git show 4c3f04805` confirms the rename is the only change the commit makes to those files. `grep -r CometNativeColumnarToRow spark/src/test/resources/` is now empty, so no occurrence was missed. ## How are these changes tested? Existing tests cover both code paths explicitly: `CometNativeColumnarToRowSuite` enables the config in its own `withSQLConf` blocks, `CometFuzzTestBase` parameterizes it, and plan assertions in `CometExecSuite` and `CometPlanChecker` match both operator variants, so no test depends on the default. Verified on this branch (JDK 11, so `-Pspark-3.5`; the Spark 4.x modules need JDK 17): - `CometTPCDSV1_4_PlanStabilitySuite` — 97 tests passed. This is the suite that validates the conflict resolution above. - `CometTPCDSV2_7_PlanStabilitySuite` — 32 tests passed. - `CometExecSuite` — 141 passed, 2 canceled. Covers the DPP `CometSubqueryBroadcastExec` and exchange-reuse assertions affected by the `CometExecRule` change. - `CometNativeColumnarToRowSuite` — 26 tests passed, confirming the now-opt-in native path still works. - `CometAggregateSuite` filtered to `avg decimal with nothing to average` — passed. Covers the ungrouped and grouped cases over both an empty table and an all-null table, with ANSI enabled and disabled; it fails with `ARITHMETIC_OVERFLOW` without the native fix. - `cargo check -p datafusion-comet-spark-expr` and `cargo build` — clean. Not run locally: the Spark SQL test lanes (`dev/diffs`) and the Spark 4.x plan-stability lanes, which need JDK 17 and a patched Spark clone. CI covers those. -- 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]
