dwsmith1983 commented on code in PR #5867:
URL: https://github.com/apache/datafusion-comet/pull/5867#discussion_r4320844036
##########
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:
Fixed in 9e95eca27. When the replacement can be NULL, `array_join`'s array
and delimiter now count as skippable, since the generated code evaluates the
replacement first and skips both when it is NULL. A replacement that can't be
NULL keeps the default rule, because the array is then evaluated for every row.
`map_lookup_shared_subexpression.sql` now has your query, plus a shared
subexpression in the delimiter, one only in the replacement (still dispatched),
a literal and a NULL literal replacement, and the two-argument form. The joins
read their delimiters from nullable columns. Before Spark 4.2, generated code
for a join whose array and delimiter can't be NULL returns NULL on every row
when the replacement can (SPARK-57200), which would hide the branch the `if`
takes.
--
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]
