sunchao commented on code in PR #7117:
URL: https://github.com/apache/datafusion-comet/pull/7117#discussion_r4319838347
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -436,10 +436,64 @@ 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)
+ case comparison: BinaryComparison =>
+ val rightMask = if (comparison.left.nullable)
Some(current.nodeName) else None
+ pending = (comparison.left, None) :: (comparison.right, rightMask)
:: pending
+ case _: SortOrder => pushChildren(Some(current.nodeName))
+ case _ if current.children.size <= 1 => pushChildren(None)
Review Comment:
[P2] Preserve skipped evaluation for empty IN predicates
An empty In has one child, so this branch permits native make_date even
though Spark skips that operand. With spark.sql.ansi.enabled=false,
spark.sql.legacy.nullInEmptyListBehavior=false, and
org.apache.spark.sql.catalyst.optimizer.OptimizeIn excluded through
spark.sql.optimizer.excludedRules, applying an empty .isin() to
make_date(y,1,1) for nullable Parquet y=6000000 returns false in Spark. The
current guard accepts this retained expression, but native InList evaluates
make_date first and propagates the newly introduced overflow. Before this PR,
non-ANSI make_date returned NULL and empty membership returned false. Handle
empty In/InSet before the general one-child case, or lower supported empty
membership directly to a Boolean literal. Default optimizer settings eliminate
this expression, so this is a nondefault configuration regression.
--
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]
