Visorgood opened a new pull request, #5558: URL: https://github.com/apache/datafusion-comet/pull/5558
## Which issue does this PR close? Closes #3178. ## Rationale for this change `array_join` was flagged `Incompatible` with the note "Null handling may differ from Spark", but the specific difference was never pinned down. Investigating it turned up a real divergence that is not the one #3178 guessed at. 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 — in `generate_string_array` the destructuring covers just the array and the delimiter. A null `null_string` collapses to `None`, which `array_to_string` reads as "omit null elements", i.e. the same as not passing a third argument at all. The result is wrong whenever `nullReplacement` evaluates to null, **including when the array contains no nulls for it to replace**: | `arr` | `nullrep` | Spark | Comet (before) | | --- | --- | --- | --- | | `["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` | Reproduced on Spark 4.1.3 with a plan confirmed native (`CometColumnarToRow` → `CometProject` → `CometNativeScan`). Worth noting for reviewers: the two cases #3178 actually asks about — null elements skipped without a replacement, and substituted with one — were **already correct** on the native path, and the tests added here lock that in. The issue's "Current Comet Implementation" snippet quoted only the two-argument branch of `convert`; the three-argument branch has existed since #1490. ## What changes are included in this PR? **The fix.** `CometArrayJoin.convert` wraps the three-argument `array_to_string` call in an `IfExpr` guarded by `IsNull(nullReplacement)`, yielding a typed null literal when the replacement is null. A non-nullable replacement — the common `array_join(arr, ',', 'X')` literal case — skips the wrap and is unchanged. No native or protobuf changes were required. **Support level.** With null handling now matching Spark, non-collated input reports `Compatible()` instead of `Incompatible`, so `array_join` runs natively without `allowIncompatible`. Non-default string collations continue to report `Incompatible` and route through the JVM codegen dispatcher (#2190). This mirrors `CometReverse`, which the existing comment in `CometArrayJoin` already named as the precedent for the collation handling. **Docs.** The `array_join` audit entry in `expression-audits/array_funcs.md` records the guard and the new status; the `expressions.md` note no longer describes the native path as opt-in. Per-Spark-version compatibility pages are generated at publish time and are not committed here. ## How are these changes tested? Comet SQL Tests under `spark/src/test/resources/sql-tests/expressions/array/`: - `array_join.sql` — expanded from 2 queries to 12. Covers null elements leading / trailing / only / all, empty-string elements versus nulls, null delimiters, empty and multi-character delimiters, and every combination of column and literal arguments. Previously this file ran without `allowIncompatible`, so it exercised the JVM codegen dispatcher rather than the native path and could not have caught this. - `array_join_null_replacement.sql` — new; regression coverage for the null `nullReplacement` cases above, including the array-with-no-nulls case. - `array_join_dispatch.sql` — added during investigation to prove the dispatcher path was unaffected, then removed once non-collated `array_join` became native by default and the file had no dispatcher path left to test. - `array_join_collation.sql` — unchanged; still covers the dispatcher for collated input. Verified with the full `CometArrayExpressionSuite` alongside the SQL tests, with scalastyle and spotless enabled: | Spark | Scala | JDK | Result | | --- | --- | --- | --- | | 4.1.3 | 2.13 | 21 | 49 passed | | 3.5.9 | 2.12 | 21 | 49 passed | | 3.4.3 | 2.12 | 17 | 47 passed, 2 cancelled (`isSpark35Plus` gates) | Each regression query was confirmed to genuinely fail without the fix, not pass vacuously. -- 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]
