sunchao commented on code in PR #5867:
URL: https://github.com/apache/datafusion-comet/pull/5867#discussion_r4316130109
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1347,10 +1352,50 @@ object QueryPlanSerde extends Logging with
CometExprShim with CometTypeShim {
val later = expr.children.tail
// Subexpression elimination never shares a leaf, such as a column or a
literal
later.forall(_.isInstanceOf[LeafExpression]) ||
sharedSubexpressions.value.exists { s =>
- !later.exists(_.exists(e => s.common.exists(_.semanticEquals(e))))
+ !holdsSharedSubexpression(later, s)
}
}
+ /**
+ * Whether some node of `expr` with more than one child holds, in a child it
can skip, a
+ * subexpression that the operator's expressions share. A node can skip any
child after its
+ * first, a `DivModLike` or `Pmod` its dividend too, and a `StringLocate`,
which evaluates its
+ * start first, its other two children, while an `ArrayAppend` skips none.
Spark evaluates that
+ * subexpression for every row, while `expr` evaluated alone, as the JVM
codegen dispatcher
+ * does, can skip it on the rows where a child evaluated before it is NULL,
so an ANSI error
+ * Spark raises goes unraised. With
`spark.sql.subexpressionElimination.enabled` off only an
+ * aggregation shares subexpressions (see `sharedSubexpressions`). A
subexpression that `expr`
+ * itself repeats does not count while the setting is on: the dispatcher
then eliminates it too,
+ * and evaluates it for every row. False outside `withSharedSubexpressions`,
where what the
+ * operator shares is unknown.
+ */
+ def skipsSharedSubexpression(expr: Expression): Boolean =
+ sharedSubexpressions.value.exists { s =>
+ // The dispatcher eliminates subexpressions only while the setting is on
+ lazy val repeated =
+ if (SQLConf.get.subexpressionEliminationEnabled)
commonSubexpressions(Seq(expr))
+ else Nil
+ expr.exists { node =>
+ val skippable = node match {
+ // `DivModLike` and `Pmod` skip the dividend when the divisor,
evaluated first, is NULL
+ case _: DivModLike | _: Pmod => node.children
+ // `StringLocate` evaluates `start`, its last child, first and skips
the others on NULL
+ case _: StringLocate => node.children.init
+ // Spark 3.x's `ArrayAppend` evaluates both children before its NULL
check
+ case _: ArrayAppend => Nil
+ case _ => node.children.drop(1)
Review Comment:
[P2] Account for `ArrayJoin` evaluating its replacement first.
Three-argument `ArrayJoin.doGenCode` evaluates `nullReplacement` before either
the array or delimiter, and skips both when it is NULL. The default
`children.drop(1)` therefore misses a shared expression inside its array
argument. With ANSI and subexpression elimination enabled, two newly dispatched
DOUBLE-key lookups can return NULL where Spark raises `DIVIDE_BY_ZERO`. This
affects the supported Spark versions checked, and the base retained Spark
fallback for these lookups. Could the match include `ArrayJoin` with a
replacement, treating its first two children as skippable, with a regression
fixture for this shape?
Evidence: Reproduced on Spark 4.1.3 with freshly compiled pinned planner and
dispatcher sources. SQL-equivalent input:
```sql
SET spark.sql.ansi.enabled=true;
SET spark.sql.subexpressionElimination.enabled=true;
CREATE TABLE t(m1 MAP<DOUBLE,INT>, m2 MAP<DOUBLE,INT>, x DOUBLE, r STRING)
USING parquet;
INSERT INTO t VALUES (NULL, NULL, 0.0D, NULL);
SELECT
element_at(IF(array_join(array(CAST(1.0D/x AS STRING)), ',', r) IS NULL,
m1, m2), 0.5D),
element_at(IF(array_join(array(CAST(1.0D/x AS STRING)), ';', r) IS NULL,
m2, m1), 0.5D)
FROM t;
```
Spark `collect()` raises `DIVIDE_BY_ZERO`. The current planner returns
`skipsSharedSubexpression=false` and serializes both lookups for dispatch.
Evaluating those payloads returns NULL for both columns. Disabling Spark
subexpression elimination returns `[NULL,NULL]`, confirming the trigger. A
disposable `ArrayJoin`-aware guard declines both calls. Artifacts:
`/tmp/review5867-92a0-current-31dut1_p/ArrayJoinSharedProbe.scala`,
`ArrayJoinSharedProbe.log`, and `fix/gate.log`. This is a component
reproduction, not a full native execution run.
--
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]
