peterxcli opened a new pull request, #5679:
URL: https://github.com/apache/datafusion-comet/pull/5679
## Which issue does this PR close?
Closes #5675.
## Rationale for this change
Spark's `ConcatWs` accepts `array<string>` arguments after the separator and
flattens their elements into the strings to join (`concat_ws(',',
array('a','b'), 'c')` = `a,b,c`, null elements skipped).
`CometConcatWs.getSupportLevel` never inspected child types and lowered every
non-foldable call to DataFusion's `concat_ws`, which accepts only string
arguments, so `concat_ws(',', arr, s)` failed at native execution with `Input
was List(...) which is not a supported datatype for concat_ws function` where
Spark returns a result.
## What changes are included in this PR?
- `CometConcatWs` mixes in `CodegenDispatchFallback` and returns
`Unsupported` (with a documented reason) whenever any argument has an
`ArrayType`. These calls now run through the JVM codegen dispatcher (Spark's
own `ConcatWs.doGenCode` inside the Comet pipeline) and fall back to Spark with
that reason only when `spark.comet.exec.scalaUDF.codegen.enabled=false`. The
NULL-separator shortcut and the native path for plain string arguments are
unchanged.
- `expressions.md`: `concat_ws` is now `Hybrid` (what `GenerateDocs` emits
for a `CodegenDispatchFallback` serde) with a note; the compatibility page
picks up the new unsupported reason automatically.
- Follow-up (not in this PR): a Spark-compatible native `concat_ws` kernel
that flattens list arguments (skipping null elements like Spark) so these calls
can leave the dispatcher.
## How are these changes tested?
- New `CometStringExpressionSuite` test `concat_ws with array<string>
arguments`: over a Parquet table with an `array<string>` column it runs
`concat_ws(',', arr, s)`, `concat_ws(',', s, arr)`, `concat_ws(',', arr)`,
`concat_ws('-', arr, s, arr)`, `concat_ws(',', split(s, ' '))` and
`concat_ws(',', split(s, ' '), arr)` and asserts Spark's answer with the whole
plan in Comet and the dispatcher actually running; with the dispatcher disabled
it asserts the Spark answer plus the serde's fallback reason; it also asserts a
NULL separator still runs natively and that plain string arguments keep the
native path.
- `sql-tests/expressions/string/concat_ws.sql`: a new `array<string>` table
and eight query blocks (column, mixed, single, repeated, `split`-produced and
literal array arguments, NULL separator), all asserting Spark-identical results.
- Ran `CometStringExpressionSuite` and `CometSqlFileTestSuite concat_ws`
(Spark 4.1 profile): 35/35 passed. Native code is untouched.
--
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]
