andygrove opened a new pull request, #5120: URL: https://github.com/apache/datafusion-comet/pull/5120
## Which issue does this PR close? Part of #5119 (first sub-task). Stacked on #5118; review the last commit only until that merges. ## Rationale for this change The interpreted columnar-to-row path (`rowIterator` plus `UnsafeProjection`, used by `CometColumnarToRowExec.doExecute` and the broadcast relation build) routes every value through two virtual calls and allocates per object-typed value: a `Decimal` per compact decimal value and a `byte[]` / `BigInteger` / `BigDecimal` chain per decimal value with precision above 18. The benchmarks in #5118 show this is where the JVM path loses to the native converter on decimal-heavy schemas, and where it produces up to 392 bytes/row of heap garbage. ## What changes are included in this PR? `DirectColumnarToRowConverter`, a prototype JVM converter that resolves each column to a primitive type code once at construction and writes values straight from the Arrow buffers into a reused row buffer: - compact decimals as unscaled longs via `CometVector.getLongDecimal`, no `Decimal` allocation - wide decimals as raw big-endian bytes via `CometVector.copyBinaryDecimal` with minimal two's-complement trimming, no BigInteger chain - strings via direct memory copy from the Arrow data buffer Output rows are byte-identical to `UnsafeProjection` output (this matters because `UnsafeRow` equality and hashing are byte-wise), replicating `UnsafeRowWriter` details: null slot zeroing, NaN canonicalization, deterministic padding bytes, and the 16-byte reservation for wide decimals including the null case. The converter is wired into `CometC2RIsolatedBench` as a third case. It is not yet wired into `CometColumnarToRowExec`; that should follow once the approach is agreed. Results (M3 Ultra, Spark 4.1, per-row ns at batchSize=8192, and heap bytes/row): | Scenario | UnsafeProjection | Direct | Native | Alloc: UP / Direct / Native | |---|---|---|---|---| | long, int, double, string | 8.7 | 12.3 | 32.7 | 24 / 7 / 100 | | 4 x decimal(12,2), date, 2 x string (tpch-like) | 35.6 | **25.2** | 52.2 | 132 / 3 / 112 | | 4 x decimal(12,2), date, long (all fixed-width) | 20.5 | **17.5** | 16.7 | 152 / 24 / 146 | | 2 x decimal(38,10), long | 56.0 | **33.5** | 70.5 | 392 / 24 / 137 | | 16 x long | 23.7 | 46.0 | 31.4 | 25 / 24 / 229 | The converter beats both existing implementations on every decimal scenario and is batch-size insensitive (no per-batch FFI cost), but loses to the codegen-unrolled `UnsafeProjection` on wide pure-primitive schemas, where the per-column switch dispatch cannot compete with straight-line generated code. That gap is the target of the column-at-a-time fast path sub-task in #5119. ## How are these changes tested? New `DirectColumnarToRowConverterSuite` asserts byte-identical output vs `UnsafeProjection` across all supported types with nulls, NaN values, empty strings, a 70-column schema exercising multi-word null bitsets, and wide decimal boundary values (sign boundaries at 127/128/-128/-129, Long.MaxValue/MinValue, and 38-digit extremes). Benchmark results above from `make benchmark-org.apache.spark.sql.benchmark.CometC2RIsolatedBench`. -- 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]
