dwsmith1983 commented on code in PR #5867:
URL: https://github.com/apache/datafusion-comet/pull/5867#discussion_r4332721666
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1347,10 +1352,53 @@ 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, a `StringLocate`, which
evaluates its start
+ * first, its other two children, and an `ArrayJoin` with a nullable
replacement, which
+ * evaluates that first, its array and delimiter, 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
Review Comment:
Not intended. `DivModLike` and `Pmod` evaluate the divisor first and skip
only the dividend in 3.5, 4.0 and 4.1, in both `eval` and `doGenCode`, so it's
`node.children.take(1)` in e3a5cd2b7. `map_lookup_shared_subexpression.sql` now
has a divisor-only pair for `/` and `pmod` that stays dispatched, next to the
dividend pairs that fall back.
--
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]
