ajsquared commented on code in PR #7261:
URL: https://github.com/apache/datafusion-comet/pull/7261#discussion_r4392362341
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -436,6 +436,65 @@ 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))
+ 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] A nonempty Catalyst `In` reaches this catch-all, which marks its probe
as skippable. For example, `make_date(y, m, 1) IN (DATE '2024-01-01', DATE
'2024-02-01')` now forces the enclosing projection back to Spark when the two
literals remain an `In` list. Spark must evaluate the probe to test membership,
and Comet already supports this date-valued native predicate. For nonempty
`In`, visit the probe without a mask and retain the guard for list expressions
that Spark can skip.
##########
native/spark-expr/src/datetime_funcs/make_date.rs:
##########
@@ -119,6 +119,11 @@ impl ScalarUDFImpl for SparkMakeDate {
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
let [year, month, day] = take_function_args(self.name(), args.args)?;
+ // Scalar arguments must not evaluate a date when the input batch has
no rows.
+ if args.number_rows == 0 {
Review Comment:
[P2] The zero-row check runs after argument evaluation. For `make_date(y,
CAST('bad' AS INT), 1)` with constant folding disabled, `NullShortCircuit`
evaluates the month on an empty batch because it finds no NULL rows to mask.
The ANSI scalar cast converts `'bad'` to a one-element array and throws before
`invoke_with_args` reaches this check. An empty batch can therefore still abort
a query that has no date to evaluate. Return the empty Date32 result before the
physical expression evaluates its children.
--
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]
