ajsquared commented on code in PR #7261:
URL: https://github.com/apache/datafusion-comet/pull/7261#discussion_r4392598125
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -436,6 +436,71 @@ 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()
+ }
+ }
+
+ /** A native parent must not evaluate a date on rows where Spark skips that
child. */
+ 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 =>
+ 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 list items
after a match.
+ case in: In =>
+ in.list.reverseIterator.foreach { child =>
+ pending = (child, Some(in.nodeName)) :: pending
+ }
+ pending = (in.value, None) :: pending
Review Comment:
[P2] Allowing a `make_date` probe to stay native exposes a query failure
when a later `IN` candidate can throw. Consider the struct probe `(make_date(y,
1, 1), 0D)` with candidates `(DATE '2024-01-01', 0D)` and `(CAST(s AS DATE),
0D)`. For `(y, s)` rows `(2024, 'bad')` and `(2025, '2025-01-01')` in one
batch, Spark returns true for both because the first row matches before the
cast. Comet's `NestedPredicate::evaluate` evaluates the second candidate on
both rows because the second row still needs it. The ANSI cast on `'bad'` then
aborts the query. Evaluate later candidates only on unmatched rows, or fall
back for lists with candidates that can throw.
--
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]
