sunchao commented on code in PR #5558:
URL: https://github.com/apache/datafusion-comet/pull/5558#discussion_r3889896849
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -352,7 +350,9 @@ object CometArrayJoin
if (hasNonDefaultStringCollation(expr.array.dataType)) {
Incompatible(Some(collationReason))
} else {
- Incompatible(Some(incompatReason))
+ // Null handling matched Spark once the nullReplacement guard in
convert() landed (#3178);
+ // collation is the only remaining deviation.
+ Compatible()
Review Comment:
[P2] Keep delimiter evaluation behind the null-array guard
Making this `Compatible` changes the default behavior of `array_join(arr,
element_at(delims, idx))` over supported Parquet input columns. For a row with
`arr = NULL`, `delims = [',']`, and `idx = 0` (`arr`/`delims` are
`ARRAY<STRING>` and `idx` is `INT`), Spark's generated code returns NULL
without evaluating the delimiter. The native `ScalarFunctionExpr` evaluates
every argument first, so `ListExtract` throws for index zero before
`array_to_string` can inspect the null array, even with ANSI disabled. The
previous default JVM dispatcher preserved Spark's guard, and the native error
propagates rather than falling back. Please preserve that guard on this newly
enabled path. This is source reasoning for the inspected Spark 3.5/4.0 paths; I
have not run it.
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -374,7 +374,28 @@ object CometArrayJoin
delimiterExprProto,
nullReplacementExprProto)
- arrayJoinScalarExpr
+ // Spark's ArrayJoin returns null as soon as nullReplacement evaluates
to null, whether
+ // or not the array actually contains any nulls. DataFusion's
array_to_string instead
+ // reads a null null_string as "omit null elements", which is what the
two-argument form
+ // means, so wrap the call in an explicit null guard (#3178). A
non-nullable replacement
+ // -- the common literal case -- cannot trigger this and is left
unwrapped.
+ if (!nullReplacementExpr.nullable) {
+ arrayJoinScalarExpr
+ } else {
+ for {
+ joined <- arrayJoinScalarExpr
+ replacementIsNull <- exprToProto(IsNull(nullReplacementExpr),
inputs, binding)
Review Comment:
[P2] Evaluate a nullable replacement once per input row
The replacement is serialized both as the function argument and again under
`IsNull`. Consider `IF(monotonically_increasing_id() % 2L = 0L, CAST(NULL AS
STRING), 'X')` with a sole input array column containing four rows of `['a',
NULL, 'b']` in one native batch. The two native replacement trees have separate
counters: the guard sees all four rows, while the function's copy sees only the
two rows that passed the guard. That second copy produces one NULL replacement,
which `array_to_string` treats as omission, yielding `'a,b'`. Spark's single
evaluation can produce only NULL or `'a,X,b'`. The mismatch survives either
starting parity, including counters advanced during planning. Please compute
the replacement once and reuse its value for both the guard and join. This is
source reasoning for the inspected Spark 3.5/4.0 paths, not an executed
reproduction.
--
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]
