andygrove opened a new pull request, #7505: URL: https://github.com/apache/datafusion-comet/pull/7505
## Which issue does this PR close? Closes #5112. Closes #7494. ## Rationale for this change The native columnar-to-row converter (`CometNativeColumnarToRowExec`) has been off by default since #5114. In the discussion on #4440, nobody could measure a benefit from it. An SF1000 cluster comparison showed the same throughput, GC time and peak memory as the JVM `CometColumnarToRowExec`. #5112 shows that in isolation it is 3.7x slower per row at 8192-row batches, and the gap grows as batches get smaller. The reason is the fixed cost of the FFI export, the JNI call and the import that every batch pays. The JVM operator also supports whole-stage codegen, so it fuses into its consumer's stage. The native operator can't do that, so it always ends the stage. Keeping the converter still costs something. It is about 2,700 lines of Rust plus a JVM operator, and every transition rule, test and Spark SQL diff has to handle it. It also has a critical open bug: row offsets wrap past 2 GiB (#7494). Deleting it fixes that bug and makes #7504 unnecessary. ## What changes are included in this PR? - Remove `native/core/src/execution/columnar_to_row.rs`, the `columnarToRowInit` / `columnarToRowConvert` / `columnarToRowClose` JNI entry points and their tests, `NativeColumnarToRowInfo`, `NativeColumnarToRowConverter`, `CometNativeColumnarToRowExec` and `CometNativeColumnarToRowSuite`. - `EliminateRedundantTransitions` now always creates `CometColumnarToRowExec`. A schema that contains Variant still keeps Spark's `ColumnarToRowExec`. The rules and tests that matched both Comet transitions now match only the JVM one. - `spark.comet.exec.columnarToRow.native.enabled` is in the `exec` category, so the versioning policy requires a deprecation cycle before it can be removed. It stays registered as a deprecated no-op, and the driver logs a warning when it is set in the Spark configuration. The upgrade guide has a new 1.3.0 section for this. - `CometFuzzTestBase` used to run each test with native shuffle, and with JVM shuffle under both C2R settings. It now runs once per shuffle mode. - The benchmarks drop their native-converter cases. `CometStringFfiImportBenchmark` drops its columnar-to-row case, because that import site no longer exists. - The five Spark SQL test diffs are regenerated from patched Spark checkouts. They drop the `CometNativeColumnarToRowExec` match arms and imports in `SubquerySuite`, `SparkPlanSuite` and `BucketedReadSuite`. For 4.0 and later they also drop the `VariantEndToEndSuite` hunk, which existed only for this operator. Before any edit, each diff round-tripped byte for byte, and each new diff applies cleanly to its tag. - Docs, the CI suite lists and the labeler are updated. ## How are these changes tested? The existing tests cover this, and `CometPluginsNativeColumnarToRowWarningSuite` is new: it checks the deprecation warning. I ran these locally on the default Spark 4.1 profile: `EliminateRedundantTransitionsSuite`, `RevertNativeForTransitionHeavyStagesSuite`, `CometDirectColumnarToRowSuite`, `CometVariantTypeSuite`, `CometVariantProjectionSuite`, `CometInMemoryCacheSuite`, `CometFuzzTestSuite`, `CometExecRuleSuite`, and the touched tests in `CometExecSuite` and `CometParquetWriterSuite`. All 308 tests passed. `cargo clippy --all-targets --workspace -- -D warnings` is clean. The Spark SQL diffs changed, so the Spark SQL suites need a run before this leaves draft. -- 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]
