sunchao commented on code in PR #7261:
URL: https://github.com/apache/datafusion-comet/pull/7261#discussion_r4394554577
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -436,6 +437,89 @@ object CometNextDay extends CometExpressionSerde[NextDay]
with CodegenDispatchFa
object CometMakeDate extends CometExpressionSerde[MakeDate] {
+ private val sharedArgumentReason =
+ "make_date requires Spark evaluation when a computed argument after a
nullable " +
+ "argument cannot preserve Spark's shared-subexpression evaluation."
+
+ override def getUnsupportedReasons(): Seq[String] = Seq(sharedArgumentReason)
+
+ override def getSupportLevel(expr: MakeDate): SupportLevel = {
+ val firstNullable = expr.children.indexWhere(_.nullable)
+ val hasComputedMaskedArgument = firstNullable >= 0 &&
+ expr.children.drop(firstNullable +
1).exists(!_.isInstanceOf[LeafExpression])
+ if (hasComputedMaskedArgument && !canShortCircuitNulls(expr)) {
+ Unsupported(Some(sharedArgumentReason))
+ } else {
+ Compatible()
+ }
+ }
+
+ // Native IN evaluates dynamic candidates for the whole batch. Only these
constructors and
+ // leaves are safe on rows where Spark skips a candidate after a match or a
NULL probe.
+ private def canEvaluateInCandidateEagerly(expr: Expression): Boolean = expr
match {
+ case _: Literal | _: Attribute => true
+ case _: CreateArray | _: CreateNamedStruct =>
+ expr.children.forall(canEvaluateInCandidateEagerly)
+ // IN coercion can widen nested nullability without converting any
candidate values.
+ case cast: Cast if equalsIgnoreCompatibleNullability(cast.child.dataType,
cast.dataType) =>
+ canEvaluateInCandidateEagerly(cast.child)
+ case _ => false
+ }
+
+ /** A native parent must preserve Spark's evaluation of a date and its
sibling expressions. */
+ private[serde] def unsupportedParentReason(expr: Expression): Option[String]
= {
+ var pending: List[(Expression, Option[String])] = (expr, None) :: Nil
+ while (pending.nonEmpty) {
+ val (current, parent) = pending.head
+ pending = pending.tail
+ if (current.isInstanceOf[MakeDate] && parent.isDefined) {
+ return Some(s"make_date requires Spark evaluation under ${parent.get}")
+ }
+ def pushChildren(mask: Option[String]): Unit = {
+ current.children.reverseIterator.foreach(child => pending = (child,
mask) :: pending)
+ }
+ if (parent.isDefined) {
+ pushChildren(parent)
+ } else {
+ current match {
+ // These native expressions already select the rows passed to their
children.
+ case _: If | _: CaseWhen | _: MakeDate => pushChildren(None)
+ // COALESCE lowers to CASE and evaluates an earlier child again in
its result branch.
+ case coalesce: Coalesce if coalesce.deterministic =>
pushChildren(None)
+ // These parents evaluate every child in Spark, including NULL
values.
+ case _: EqualNullSafe | _: CreateArray | _: CreateNamedStruct | _:
Greatest |
+ _: Least =>
+ pushChildren(None)
+ // These parents evaluate left first and skip right only when left
is NULL.
+ case _: BinaryComparison | _: DateAdd | _: DateSub | _: DateDiff |
_: DateFormatClass =>
+ val left = current.children.head
+ val right = current.children(1)
+ val rightMask = if (left.nullable) Some(current.nodeName) else None
+ pending = (left, None) :: (right, rightMask) :: pending
+ case _: SortOrder => pushChildren(Some(current.nodeName))
+ // Empty membership can skip its sole operand in Spark.
+ case in: In if in.list.isEmpty =>
pushChildren(Some(current.nodeName))
+ case inSet: InSet if inSet.hset.isEmpty =>
pushChildren(Some(current.nodeName))
+ // Nonempty IN always evaluates its probe, but may skip candidates
on individual rows.
+ case in: In =>
+ in.list.reverseIterator.foreach { child =>
+ pending = (child, Some(in.nodeName)) :: pending
+ }
+ val probeParent = if
(in.list.forall(canEvaluateInCandidateEagerly)) {
+ None
+ } else {
+ Some("In with computed candidates")
+ }
+ pending = (in.value, probeParent) :: pending
+ case _ if current.children.size <= 1 => pushChildren(None)
+ // Other native multi-child expressions may evaluate operands that
Spark skips.
+ case _ => pushChildren(Some(current.nodeName))
Review Comment:
[P2] Preserve native execution for always-evaluated operands
This catch-all still rejects supported expressions such as
`hash(make_date(y, m, 1), y)` and `xxhash64(make_date(y, m, 1), y)` with
non-foldable integer columns. The current guard returns `Some(...Murmur3Hash)`
/ `Some(...XxHash64)`, causing the enclosing expression to fall back. Spark's
`HashExpression` evaluates every child in both interpreted and generated
execution, including when an earlier child is null; these parents cannot skip
the date operand. The native hash serializers already support these
Date/Integer inputs and are unchanged from base, so this unnecessarily removes
existing native execution. The same branch rejects `unix_date(make_date(y, m,
1)) + 1`, even though the date is in the required first operand and the other
operand is a harmless literal. Please account for these eager parents and
required operands while retaining inherited masks, with native-plan regression
tests.
--
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]
