sunchao commented on code in PR #5558:
URL: https://github.com/apache/datafusion-comet/pull/5558#discussion_r3916750231
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -357,13 +357,38 @@ object CometArrayJoin
with CometTypeShim
with CodegenDispatchFallback {
- private val incompatReason = "Null handling may differ from Spark"
-
private val collationReason =
"array_join does not propagate non-UTF8_BINARY collations to the output
string " +
"(https://github.com/apache/datafusion-comet/issues/2190)"
- override def getIncompatibleReasons(): Seq[String] = Seq(incompatReason,
collationReason)
+ private val nonDeterministicReason =
+ "array_join would have to evaluate a non-deterministic argument twice to
reproduce Spark's " +
+ "null short-circuiting
(https://github.com/apache/datafusion-comet/issues/3178)"
+
+ /**
+ * Arguments needing an `IsNull` guard around the native call, in Spark's
evaluation order.
+ *
+ * Spark short-circuits to null on the array, then the delimiter, then the
null replacement, and
+ * never evaluates the later arguments; DataFusion evaluates all of them
eagerly. `IfExpr` is a
+ * DataFusion `CaseExpr`, which evaluates branches against a filtered batch,
so nesting restores
+ * that ordering. Only a non-foldable later argument needs protecting. The
null replacement is
+ * guarded whenever it is nullable: `array_to_string` reads a null
`null_string` as "omit nulls"
+ * rather than nullifying the row (#3178).
+ */
+ private def guardedArgs(expr: ArrayJoin): Seq[Expression] = {
+ val afterArray = expr.delimiter +: expr.nullReplacement.toSeq
+ val arrayGuard =
+ if (expr.array.nullable && afterArray.exists(!_.foldable))
Seq(expr.array) else Nil
+ val delimiterGuard =
+ if (expr.delimiter.nullable && expr.nullReplacement.exists(!_.foldable))
{
+ Seq(expr.delimiter)
+ } else Nil
+ val replacementGuard = expr.nullReplacement.filter(_.nullable).toSeq
+ arrayGuard ++ delimiterGuard ++ replacementGuard
Review Comment:
[P2] Preserve the generated code's replacement-first evaluation
Could these guards preserve `ArrayJoin.doGenCode`'s evaluation order? In the
maintained Spark 3.5/4.0 implementations, the replacement is evaluated and
null-checked before the array and delimiter. With nullable Parquet columns `arr
= ['a','b']`, `delims = [',']`, `idx = 0`, and `nullrep = NULL`,
`array_join(arr, element_at(delims, idx), nullrep)` therefore returns NULL in
Spark's generated path. Here the deterministic arguments remain `Compatible`,
and the delimiter guard runs before the replacement guard, so `ListExtract`
raises `INVALID_INDEX_OF_ZERO`, even with ANSI disabled. The previous
revision's outer replacement guard skipped that delimiter.
This also matters for a non-nullable replacement such as
`CAST(monotonically_increasing_id() AS STRING)`. It is excluded from
`guardedArgs`, so the nondeterminism check permits it, but the new array guard
advances its counter only for retained rows. Spark evaluates it before checking
the array on every row. Moving only the nullable replacement guard would leave
this case.
Could the lowering preserve replacement-first, single evaluation, or use the
dispatcher for cases it cannot represent, with regression coverage for these
shapes? These are source-derived cases, not executed queries.
--
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]
