feat: execute concat_ws with array arguments natively - #5725
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed 309dd7dc against c348f775. This replaces the array-argument JVM dispatch/fallback with DataFusion Spark 55's SparkConcatWs, and also replaces the existing native implementation for string-only calls. The explicit return type avoids DataFusion's string-only argument signature while preserving Spark's analyzed children. The literal-NULL separator shortcut and all-foldable fallback remain.
I compared the implementation with the maintained Spark 3.5 (5947fd6e) and 4.0 (03f28fc4) sources. Their ConcatWs flattens arrays in argument order; NULL separators return NULL; NULL strings, arrays and elements contribute nothing; empty strings still count as elements; zero retained elements and separator-only calls return an empty string. The new kernel follows those rules for column inputs, including varying separators and UTF-8 strings. Spark's analyzer still owns implicit casts and invalid argument rejection; the native planner preserves the already-analyzed child types rather than invoking the kernel's broader coercion rules. Concatenation itself adds no ANSI arithmetic behavior. Spark 4.0 carries collation metadata but concatenates the original UTF-8 bytes. The maintained 3.4 and 4.1 branches were unavailable, so this is not a source-based compatibility qualification for those versions.
[P2] The all-foldable guard does not protect runtime scalar inputs. A string scalar subquery is non-foldable but evaluates to ColumnarValue::Scalar in Comet. With a literal separator and no column-valued argument, the new kernel expands inputs to one row and then indexes them using the surrounding batch's row count. It reaches an out-of-bounds string access on the second row. The previous DataFusion string implementation returns a scalar for this shape. The inline finding describes the required runtime handling and regression coverage; this conclusion is established from the exact sources, not a local execution claim.
Validation and CI
The new Rust registration test passed in job 101381376783. Its checkout log identifies merge eb17a9fd; I verified raw parents [c348f775, 309dd7dc] and matching feature/dependency/planner blobs. The four decimal-cast file differences between the current base and the PR head are branch divergence, not changes introduced by this PR.
The final current-head CI refresh at 2026-09-05 22:49:38 UTC reported 46 successful, four failed, 15 in progress and seven skipped checks. The four Java-lint failures report Scalafix errors; the Spark 3.5 lint diagnostic specifically requests removal of the now-unused ArrayType import in strings.scala. That must be cleared. The Scala regression explicitly disables codegen dispatch, but its presence is not a local test result. The PR body reports Spark 4.1 SQL/Scala passes; I did not independently run Spark/JVM tests or a local kernel reproduction, and the wider expression CI remained pending at the snapshot cutoff.
Performance
[P2] Add representative microbenchmark results before enabling this implementation. No results are provided in the PR body or discussion, and the benchmark CI job was skipped. The existing CometStringExpressionBenchmark only benchmarks concat_ws(' ', c1, c1); the PR adds no array case. Compare the new native array path against Spark codegen, and measure the existing string-only case before and after this change with the same data, configuration and verified execution paths.
The kernel writes directly into an Arrow builder, avoiding an intermediate concatenated string per row. However, values_to_arrays expands scalar separators and literal arguments across the batch, list rows create slices, and the output starts with a fixed 16-byte-per-row reservation. The former string kernel keeps scalar references and computes an input-derived capacity. These are material differences for a string hot path. Cover column and literal separators, mixed arguments, realistic array lengths, nulls, and short/long strings; correctness comparisons alone cannot establish the performance benefit or absence of regression.
Design
Reusing the already-locked Spark-specific kernel is a small and appropriate change: it avoids maintaining a second array-flattening implementation in Comet. The name-based factory arm takes precedence over the default registry, and the explicit return type prevents an unrelated string-only signature from rejecting Spark-valid arrays. Spark analysis remains responsible for admissible inputs, so bypassing DataFusion coercion is deliberate here.
The remaining correctness issue is at the boundary between Spark's foldability and DataFusion's runtime scalar/array representation. A runtime adapter that handles all-scalar batches, or a corrected upstream kernel, addresses that contract directly. Extending a compile-time literal check alone does not establish the required batch cardinality.
Abstraction & complexity
The PR introduces no new framework or broad abstraction: one factory arm and a smaller serde replace the array fallback branch. The SQL tests and dispatch-disabled Scala checks use existing test infrastructure. The separation between Spark admission, serialization and the reused kernel is understandable. Any adapter needed for the scalar case should stay local to this UDF and preserve existing fallback behavior; no further abstraction change is warranted by the reviewed scope.
Which issue does this PR close?
Closes #5687.
Rationale for this change
concat_wswith string-array arguments currently uses Spark codegen dispatch, or falls back to Spark when dispatch is disabled. DataFusion Spark 55, already used by Comet, provides the required native implementation.What changes are included in this PR?
Route arrays and separator-only calls through
spark_concat_ws, with an explicit return type and a small adapter around upstreamSparkConcatWs. The adapter evaluates all-scalar arguments once and returns a scalar for DataFusion to broadcast. This handles non-foldable scalar subqueries, which otherwise panic when the upstream kernel indexes a one-row array using the enclosing batch length.Keep ordinary string-only calls on the existing DataFusion kernel. Retain the all-foldable fallback and NULL-literal separator shortcut. Add native and Spark regressions covering ordered mixed arguments, multiple arrays, nulls, empty values, Unicode, column separators, and runtime scalar subqueries. Update expression support and audit documentation.
Add
CometConcatWsBenchmarkfor string-only and mixed-array inputs, including plan assertions with codegen dispatch disabled. Theimplement-comet-expressionskill was used to scaffold the implementation workflow.How are these changes tested?
-D warningspasses.ArrayTypeimport is now used by the routing check.Matched benchmark results
Apple M4, 24 GiB RAM, macOS 26.6.2, Zulu JDK 21.0.6, Spark 4.0.4. Native libraries use the same optimized Cargo release profile without
target-cpu=native. Each run uses a fresh JVM with 4 GiB heap,local[1], one Comet worker thread, AQE disabled, and 8,192-row batches for both readers. Standard SparkBenchmarkwarmup and minimum measurement duration apply.Inputs are 65,536 deterministic Parquet rows in one file. String widths are 8 and 128; arrays vary from 1 to N elements for N=2/8/32. Every 13th string and 11th array is NULL, every fifth array element is NULL, and the column separator alternates
|/--with NULL every 17th row. Mixed expression:concat_ws(separator, a1, c1, a2, 'tail'); string expression:concat_ws(separator, c1, c1)(the previously supported benchmark shape).All measurements verified Spark
WholeStageCodegenExecwith concat_ws present and fully nativeCometProjectoverCometNativeScan, with Scala UDF codegen dispatch disabled. Timing includes Parquet scanning, projection, and the noop sink; these are query timings, not isolated kernel speedups. This is a development laptop with background applications, so the variance below matters and small differences should not be interpreted as improvements.Performance tradeoff: native support is not a universal speedup. Short-string cases are faster in both runs. With 128-character strings, arrays up to 32 elements and a literal separator, Comet takes 153 vs 126 ms in run 1 and 126 vs 114 ms in run 2 (about 21% and 11% slower). Column separators are near parity for that shape. The first run has substantial outliers, so both runs are included. This leaves a measurable long-string array optimization opportunity in the new path while preserving the existing string kernel.
Array/mixed cases, updated library, two fresh-JVM runs. Times are mean ± standard deviation in milliseconds; ratio is Spark mean / Comet mean.
Existing string-only path, before and after. Before uses the native factory from parent
75fdddc9285ec61c0cd326977c61dd41fca39a8b; after uses this PR's final routing and adapter. The JVM benchmark and inputs are identical. Run order: before-1, after-1, before-2, after-2, updated matrix-1, updated matrix-2. No string regression was observed in either pair; the final implementation retains the original string kernel.Library SHA-256: before
ffe3b864c56bbfa2620250d1591380965d06733b7fc0b9581f8c64f1c9d346dd; after3dad10e0a2d7d51ef67bec0b01ede096e771f91c79be6b1fc441e4fedc0f7d67.The benchmark can be run through
make benchmark-org.apache.spark.sql.benchmark.CometConcatWsBenchmark PROFILES=-Pspark-4.0 BENCH_HEAP=4g; passstringsfor only the existing string cases. For the measurements above, the same class was launched directly with the reactor test classpath and an explicitjava.library.pathto select each release library, using JVM options frommake print-benchmark-args. The normal make target additionally enablestarget-cpu=native, so use identical Cargo flags for both libraries when repeating the before/after comparison.