andygrove opened a new pull request, #7506:
URL: https://github.com/apache/datafusion-comet/pull/7506
> [!WARNING]
> **This is an experiment, and it will probably not be merged.** I opened it
to share the code and numbers behind the discussion on #7504 about whether
native columnar-to-row (C2R) is worth keeping. Please don't review it for merge.
## Which issue does this PR close?
None. This is related to the discussion in #7504.
## Rationale for this change
On #7504 the question came up of why Comet keeps the native C2R path when it
is not the default and has not shown a clear win. I benchmarked the current
native path, then tried two rounds of optimization to see whether a native C2R
can beat the JVM path at all.
Short answer: the current native C2R never beats the JVM codegen path end to
end. The prototype here does win on decimal-heavy, wide and dictionary-encoded
string schemas, ties on primitives and strings, and still loses on nested types.
End-to-end results from `CometColumnarToRowBenchmark` (Comet native scan,
then C2R, then a JVM sink; ns/row, lower is better; M3 Ultra, `local[1]`):
| Schema | Comet JVM (codegen) | Native, current | Native, this prototype |
|---|---|---|---|
| TPC-H-like (4 x decimal(12,2), date, 2 strings) | 45.5 | 66.8 | **27.5** |
| Same, rows consumed by a Scala UDF (fused codegen) | 46.7 | 67.3 |
**28.7** |
| Wide rows (50 columns) | 265 | 346 | **208** |
| Dictionary-encoded strings | 42.8 | 62.3 | **31.4** |
| Primitives | 33.0 | 67.9 | 32.8 |
| Strings | 49.1 | 69.2 | 48.3 |
| Fixed-width | 30.5 | 37.4 | 31.0 |
| Struct | 162 | 193 | 190 |
| Array | 86 | 113 | 110 |
| Map | 180 | 229 | 223 |
| TPC-H-like, 1% selective filter | 24.7 | 24.6 | 24.0 |
The UDF-consumer row shows that the decimal win holds when the rows feed
fused whole-stage codegen, not just a `noop` sink. In the isolated converter
bench (`CometC2RIsolatedBench`), native is still 3 to 6 times slower at 32-row
batches. That costs about 2 µs per batch, mostly JNI transitions.
## What changes are included in this PR?
- **Flat path.** For schemas where every column is fixed-width, string,
binary or decimal, the native side plans the row sizes, then writes the rows
column by column straight into a JVM `byte[]`. The output is byte-identical to
the existing row-at-a-time path, which a new Rust test checks.
- **No per-row copy.** Each row is a new `UnsafeRow` over its batch's array.
This keeps the guarantee that rows stay valid after later batches (#3308).
- **Raw buffer path.** Plain, non-dictionary JVM vectors are passed as raw
validity/offset/data buffer addresses instead of being exported through the
Arrow C Data interface. Other columns use the C Data interface, with FFI
structs reused across batches.
- Unboxed row-count tracking in `NativeUtil.exportBatch`.
- `-Dcomet.c2r.legacy=true` selects the old path for A/B runs.
- Benchmark changes:
- "legacy" and "optimized" native arms.
- TPC-H-like groups: all rows, a UDF consumer, and a 1% selective filter.
- Nested and long-string scenarios in the isolated bench.
Known gaps and side findings:
- Nested types still use the old row path. A real version would send them to
the JVM path.
- A batch whose rows exceed about 2 GiB fails with a clear error. A real
version would split the batch into chunks (see #7494).
- The raw path skips the lossy UTF-8 decode, so its output bytes match
Spark's raw copy. This is a behavior change from the current native path.
- The current `exportBatchToAddresses` allocates FFI structs per batch and
never closes them. This is a separate leak, independent of this PR.
- `GetPrimitiveArrayCritical` blocks GC while a batch is written.
- I have not validated this on a cluster or on TPC-H at scale.
## How are these changes tested?
- New Rust test `test_flat_path_matches_general_path`. It compares the flat
path byte for byte against the existing conversion, covering nulls, slices,
more than 64 columns, wide decimals and large binary.
- `CometNativeColumnarToRowSuite` and `DirectColumnarToRowConverterSuite`
pass (33 tests) with the prototype as the default.
- Benchmarks as above.
--
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]
