dwsmith1983 commented on code in PR #5867:
URL: https://github.com/apache/datafusion-comet/pull/5867#discussion_r4332734804
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -60,7 +60,120 @@ object CometArrayRemove
}
}
-object CometArrayAppend extends CometExpressionSerde[ArrayAppend] with
ArraysBase {
+/**
+ * Shared gate for serdes whose native NULL guard (`CASE WHEN child IS NOT
NULL`) serializes the
+ * child twice: a stateful child drifts between the two copies, so it is
declined and runs through
+ * the JVM codegen dispatcher, where Spark evaluates it once. Nullability is
not consulted: a
+ * non-nullable stateful child only stays in step because DataFusion skips the
filter when the
+ * guard matches every row, which is not a contract to lean on.
+ */
+private[serde] object NullGuardSupport {
+
+ val nondeterministicReason: String =
+ "Comet has no native path for a nondeterministic operand such as `rand()`
or " +
+ "`monotonically_increasing_id()`, because the native `NULL` guard would
evaluate it twice."
+
+ /** `Unsupported` when any of `children` is nondeterministic, otherwise
`None`. */
+ def nondeterministicChild(children: Seq[Expression]): Option[SupportLevel] =
+ children
+ .find(child => !child.deterministic)
+ .map(_ => Unsupported(Some(nondeterministicReason)))
+}
+
+/**
+ * Keeps a result holding a calendar interval, at any nesting depth, off the
JVM codegen
+ * dispatcher. The dispatcher writes the interval's microseconds to Arrow as
nanoseconds, which
+ * overflows a long beyond about 292 years, a range Spark represents
+ * ([#5279](https://github.com/apache/datafusion-comet/issues/5279)). Such an
expression falls
+ * back to Spark instead.
+ */
+private[serde] object CalendarIntervalOutput {
+
+ private val reason: String =
+ "the result holds a calendar interval, whose microseconds overflow the
dispatcher's Arrow " +
+ "nanoseconds beyond about 292 years"
+
+ def declineReason(expr: Expression): Option[String] =
+ if (SupportLevel.containsType(expr.dataType,
classOf[CalendarIntervalType])) Some(reason)
+ else None
+}
+
+/**
+ * Keeps an expression off the JVM codegen dispatcher when one of its nodes
could skip an argument
+ * holding a subexpression that the operator's other expressions share (see
+ * `QueryPlanSerde.skipsSharedSubexpression`). Spark evaluates such a
subexpression for every row,
+ * so the expression falls back to Spark instead.
+ */
+private[serde] object SharedSubexpressionArgument {
+
+ private val reason: String =
+ "an argument this expression can skip holds a subexpression that the
operator's other " +
+ "expressions share, which Spark evaluates for every row before this
expression's NULL check"
+
+ def declineReason(expr: Expression): Option[String] =
+ if (skipsSharedSubexpression(expr)) Some(reason) else None
+}
+
+/**
+ * Whether evaluating `expr` at a different point than Spark does, or not at
all, is unobservable.
+ *
+ * A literal or column read cannot throw, carry state or have a side effect,
so neither its
+ * evaluation order nor a skipped evaluation can be observed; anything else
may raise or drift
+ * where Spark does not. `foldable` is not usable here: ConstantFolding leaves
a throwing foldable
+ * expression unfolded in a conditional branch.
+ */
+private[serde] object EvaluationOrder {
+
+ def isUnobservable(expr: Expression): Boolean = expr match {
+ case _: Literal | _: Attribute | _: BoundReference => true
+ case _ => false
+ }
+}
+
+object CometArrayAppend
+ extends CometExpressionSerde[ArrayAppend]
+ with ArraysBase
+ with CodegenDispatchFallback {
+
+ override def dispatchDeclineReason(expr: Expression): Option[String] =
+ CalendarIntervalOutput
+ .declineReason(expr)
+ .orElse(SharedSubexpressionArgument.declineReason(expr))
Review Comment:
Done in e3a5cd2b7: `GatedCodegenDispatchFallback` sits next to
`CodegenDispatchFallback` and holds both gates, and the six serdes mix it in
instead of copying the override. `CometSize` takes it too, since the interval
half can't fire for an int result.
For `getUnsupportedReasons`, array_append now keeps its own reason, since
after #7025 it declines a nondeterministic operand only over a nullable array.
That leaves `CometSize`, `CometArraysZip` and `CometMapFromArrays` sharing the
one-line `Seq(NullGuardSupport.nondeterministicReason)`, and `CometArraysZip`
adds a second reason, so a trait for that line wouldn't be shorter.
Making the gates the default on `CodegenDispatchFallback` itself would mean
checking every other dispatched expression's fixtures, so I'd rather do that
separately.
--
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]
