Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed f7fd38b85083365f8312839b5d07b27c3263dca2 against 36146a87bf9ca9ca9e211b4372628ed2f9d8c8c6. I found no new P1/P2 issue in the nine-file change.
The existing non-codegen JVM path reads a ColumnarBatchRow and feeds it through UnsafeProjection. This adds an experimental direct writer for supported schemas. The shared conversion helper applies the same policy to partition execution and broadcast relation construction. The flag defaults to false, batches below 128 rows retain the existing projection, and an unsupported field keeps the whole schema on that fallback. Whole-stage codegen and native C2R do not use the new writer.
I compared the row layout and conversion contracts against the maintained Spark 3.5 and 4.0 branches. Null bitsets and unused slot bytes are cleared, strings retain eight-byte alignment and deterministic padding, and wide decimals reserve 16 bytes even when null. The decimal sign trimming preserves the minimal signed representation. Compact dictionary decimals use the dictionary accessor, while wide dictionary decimals use the existing decoded-value path. Dates and timestamps retain their already evaluated integer values, so this conversion adds no separate ANSI or timezone calculation. Buffer-size arithmetic is checked before narrowing or allocation.
The reused row has the same copy-before-retaining contract as the prior projection. The hash relation builders consume and copy its bytes before advancing. The fixed-width path owns a batch buffer, and the general path copies strings and decimals into its row buffer.
Validation
- An independent Java component probe compiled the exact converter and all eight vector source files, then passed 32 cases and 1,704 byte comparisons against Spark's row writer. It covered real Arrow plain and dictionary vectors, compact and wide decimals, nulls, noncanonical NaN payloads, long Unicode strings, growing and shrinking batches, multiword null bitsets, and retained row copies. It used cached Spark 4.1.3 and Arrow 18.3.0 dependencies with a small test-only field-type bridge. This was not a full Spark/JNI or repository build.
- The Spark 4.1/JDK 17 shuffle shard passed 498 tests, including all four new integration tests for mixed/null values, fixed-width conversion, small-batch fallback and unsupported-schema fallback. Its checkout was merge
9a7934c, whose tree matches the reviewed head. - The JVM build passed at that same tree with tests skipped. The execution shard subsequently passed 922 tests, including all six new converter unit tests, at the same verified merge checkout. Current-head CI is now complete with 23 successful checks and 14 skipped checks. Skipped jobs are not counted as executed validation.
- Maintained Spark 3.4, 4.1 and 4.2 source branches were unavailable locally. The Spark 4.1 runtime and CI evidence above do not replace that canonical-source comparison.
Performance
The expanded isolated benchmark compares the direct writer with a same-run JVM projection control across five schemas and batch sizes 8192, 512 and 32. It prepares Arrow batches outside the timed cases and measures JVM allocation separately.
The author's reported 8192-row results support the decimal-heavy motivation: compact decimals improve from 20.1 to 10.4 ns/row and wide decimals from 67.0 to 37.4 ns/row. The mixed primitive/string case slows from 9.4 to 12.1 ns/row, about 29%, despite allocating less. These are author measurements of conversion alone, not independently reproduced query-level gains. The measured inputs also do not establish performance for null-heavy or dictionary data.
The fixed-width path trades a retained buffer for the whole batch and eager conversion for fewer per-value calls. That matters for large batches and consumers that stop early. The default-off setting, small-batch fallback and explicit experimental scope fit the schema-dependent evidence.
Design
The two conversion modes are straightforward: fixed-width batches are written column by column at a constant row stride, while variable-width rows use a reusable buffer. Sharing the selection logic between the two non-codegen entry points keeps their fallback behavior consistent. Constructing the fallback projection lazily also avoids its setup when every batch uses the direct path.
Keeping Spark's projection as the unsupported-schema fallback limits the compatibility surface. The configuration and tuning documentation explain how to opt in and where the optimization applies. I found no design change that needs to be made before merge.
Abstraction & complexity
The specialized writer adds substantial type-specific code, but each branch corresponds directly to Spark's physical row representation. Separate fixed-width loops avoid adding a per-cell strategy abstraction or generating another schema-specific class. The decimal-heavy measurements provide a concrete reason for that specialization.
The main maintenance obligation is preserving byte-level compatibility as types or vector representations evolve. The byte comparisons, explicit supported-type gate and existing fallback address that obligation within this change. I found no additional actionable simplification.
|
I checked this out and byte-compared the direct converter against I also confirmed the flag-off path is the original code verbatim and that The part I could not get a test to reach is the three Two things worth adding to the tuning guide next to the flag. The fixed-width path converts the whole batch in Last one. The compact and wide decimal fast paths skip the |
Which issue does this PR close?
Closes #5119.
This completes the two implementation tasks retained in that issue. It refreshes and productionizes the prototype from #5120 on current
main, with expanded correctness coverage and a new same-machine A/B benchmark.Rationale for this change
The interpreted JVM columnar-to-row path used by
CometColumnarToRowExecreads every value throughColumnVectoraccessors and then writes it throughUnsafeProjection. That path is already efficient for simple primitive schemas, but it creates substantial per-value garbage for decimals:Decimalobject per value;byte[]/BigInteger/BigDecimalchain;The benchmark introduced in #5113 originally covered only
long, int, double, string, which is close to the existing JVM path's best case. The broader schema analysis in #5112 and #5118 showed that decimal-heavy and all-fixed-width schemas behave very differently. This PR therefore expands the benchmark matrix and adds a direct JVM converter that writes Arrow values into Spark'sUnsafeRowrepresentation while reusing its row and buffers.This approach also avoids crossing JNI and avoids the native converter's per-row defensive copies required by the row-lifetime contract fixed in #3367.
Related behavior and history were cross-checked against #5114, #5115, #3308, #3221, #3266, #3268, #3649, and the production GC discussion in #4440.
What changes are included in this PR?
Direct JVM converter
Adds
DirectColumnarToRowConverter, which resolves column types once and writes directly intoUnsafeRowstorage.Supported types are:
The converter uses:
The output is byte-identical to
UnsafeProjection, including null-slot zeroing, deterministic padding, multi-word null bitsets, wide-decimal reservations, and Spark's canonical NaN representation. The converter also rejects oversized schemas, batches, and row buffers before allocation.Integration and fallbacks
Regular execution and broadcast relation builds now share one conversion helper.
Two experimental settings control the feature:
spark.comet.exec.columnarToRow.direct.enabled=falsespark.comet.exec.columnarToRow.direct.minBatchSize=128The optimization is disabled by default. When enabled:
rowIteratorplusUnsafeProjectiononce per plan;minBatchSizefall back independently;The settings and fallback behavior are documented in the tuning guide.
Benchmark coverage
CometC2RIsolatedBenchnow compares the existing JVM path, the direct converter, and the native converter across five representative schemas and batch sizes 8192, 512, and 32. It reports both wall-clock time and JVM heap allocation per row. The sink reads rawUnsafeRowslots so benchmark-side wrapper allocation does not distort converter cost.Known trade-off
The direct path is not universally faster. The mixed primitive/string schema contains no expensive decimal accessors, and its string column prevents the fixed-width path from engaging.
UnsafeProjectionis already schema-specialized straight-line code for this case, while the general direct path still pays per-field dispatch and row-assembly costs.For that reason this PR keeps the feature opt-in. It does not add a benchmark-tuned schema heuristic, and the small-batch threshold only addresses per-batch amortization rather than the large-batch mixed-schema regression.
How are these changes tested?
Correctness and repository checks
DirectColumnarToRowConverterSuitecompares raw output bytes withUnsafeProjectionacross supported types, nulls, fixed- and variable-width paths, decimal boundaries, empty strings, multi-word null bitsets, noncanonical float/double NaN payloads, and oversized fixed-width batches.CometDirectColumnarToRowSuiteverifies end-to-end Spark results and plan selection with whole-stage codegen disabled, including mixed types, the fixed-width path, unsupported-schema fallback, and minimum-batch fallback.Commands run after rebasing onto current
main:make core ./mvnw test -Dtest=none \ -Dsuites=org.apache.comet.DirectColumnarToRowConverterSuite,org.apache.comet.exec.CometDirectColumnarToRowSuite make format PROFILES=-Pspark-4.0 python3 dev/ci/check-suites.py git diff --check upstream/main...HEADResults:
Benchmark
Command:
Environment:
-Xmx20g;-Ctarget-cpu=native.Protocol:
mainand the proposed converter;batchSize=8192.The A/B was recorded at the implementation base (
2699f59b7). The subsequent rebase did not changeCometColumnarToRowExec, the benchmark, or the vector accessors onmain; the current rebased code was recompiled and retested as listed above.long, int, double, string4 x decimal(12,2), date, 2 x string4 x decimal(12,2), date, long2 x decimal(38,10), long16 x longThe direct converter wins 4 of 5 representative large-batch schemas. Winning cases improve by 1.12-1.93x, while decimal-heavy schemas reduce JVM heap allocation by 84-97%. The mixed primitive/string regression is intentionally reported rather than averaged away and is why the feature remains disabled by default.