Visorgood commented on issue #3178:
URL:
https://github.com/apache/datafusion-comet/issues/3178#issuecomment-5468605639
I looked into this and the outcome is a bit different from what the issue
describes, so summarizing before opening a PR.
**The two cases in the issue already behave correctly.** On the native path
(`allowIncompatible=true`), `array_join(array('a', null, 'b'), ',')` returns
`a,b` and `array_join(array('a', null, 'b'), ',', 'X')` returns `a,X,b`, both
matching Spark. DataFusion's `array_to_string` documents its third argument as
"If not provided, nulls will be omitted", which is the same rule Spark applies.
The issue's "Current Comet Implementation" snippet quotes only the two-argument
branch of `CometArrayJoin.convert`; the three-argument branch that forwards
`nullReplacement` has been there since #1490.
**There is a real incompatibility, but it is a different one.** Spark's
`ArrayJoin` short-circuits to null on three conditions: a null array, a null
delimiter, and a null `nullReplacement`. DataFusion's `array_to_string` only
short-circuits on the first two – `generate_string_array` destructures just the
array and the delimiter. A null `null_string` collapses to `None`, which it
reads as "omit null elements", identical to passing no third argument.
So the result diverges whenever `nullReplacement` evaluates to null,
**including when the array contains no nulls at all**:
| `arr` | `nullrep` | Spark | Comet native |
| --- | --- | --- | --- |
| `["a", null, "c"]` | `NULL` | `NULL` | `a,c` |
| `["a", "b", "c"]` | `NULL` | `NULL` | `a,b,c` |
| `["a", null, "c"]` | `X` | `a,X,c` | `a,X,c` |
Repro on Spark 4.1.3, plan confirmed native via `EXPLAIN`
(`CometColumnarToRow` → `CometProject` → `CometNativeScan`):
```sql
CREATE TABLE aj(arr array<string>, delim string, nullrep string) USING
parquet;
INSERT INTO aj VALUES
(array('a','b','c'), ',', NULL),
(array('a',NULL,'c'), ',', NULL),
(array('a',NULL,'c'), ',', 'X');
SELECT array_join(arr, delim, nullrep) FROM aj;
```
with `--conf spark.comet.expression.ArrayJoin.allowIncompatible=true`.
The JVM codegen dispatcher path is unaffected and matches Spark throughout,
so this only reaches users who have opted into the native path.
**Proposed fix**, which I have working locally: wrap the three-argument
`array_to_string` call in an `IsNull(nullReplacement)` guard inside
`CometArrayJoin.convert`. It needs no native or protobuf change, and a
non-nullable replacement (the common literal case) skips the guard entirely.
Since the null-handling reason was the only non-collation entry for
`array_join` in `expression-audits/array_funcs.md`, the fix also lets
`getSupportLevel` return `Compatible()` for non-collated input, taking
`array_join` off the opt-in path and making it native by default. Collated
input stays `Incompatible` under #2190. Happy to split that part out if it is
better to land the correctness fix on its own.
--
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]
