comphead commented on code in PR #7529:
URL: https://github.com/apache/datafusion-comet/pull/7529#discussion_r4532990462
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1339,22 +1339,56 @@ object QueryPlanSerde extends Logging with
CometExprShim with CometTypeShim {
* a filter or a sort, are treated like a projection, which at worst keeps
them evaluated for
* every row where Spark does not. They are found on the first
`canShortCircuitNulls` call for
* the operator, so an operator without an argument to skip never runs
`EquivalentExpressions`.
- * `None` outside `withSharedSubexpressions`, where they are unknown.
+ * `None` outside `withSharedSubexpressions`, where they are unknown. Only
those that can raise
+ * are kept, see `cannotRaise`.
*/
private val sharedSubexpressions = new
DynamicVariable[Option[SharedSubexpressions]](None)
private final class SharedSubexpressions(expressions: Seq[Expression],
alwaysShared: Boolean) {
- // Find shared nodes in Spark's original trees, then compare them after
decimal promotion.
- lazy val common: Seq[Expression] =
+ // The shared subexpressions that can raise. Find shared nodes in Spark's
original trees, then
+ // compare them after decimal promotion. Promotion wraps decimal
arithmetic in CheckOverflow,
+ // which `cannotRaise` does not list, so that arithmetic is always kept.
+ lazy val raising: Seq[Expression] =
if (alwaysShared || SQLConf.get.subexpressionEliminationEnabled) {
val equivalence = new EquivalentExpressions
expressions.foreach(equivalence.addExprTree(_))
- equivalence.getCommonSubexpressions.map(DecimalPrecision.promote)
+
equivalence.getCommonSubexpressions.map(DecimalPrecision.promote).filterNot(cannotRaise)
} else {
Nil
}
}
+ /**
+ * Whether evaluating `expr` on a row cannot raise, whatever the row holds.
This fails closed:
+ * an expression is assumed to raise unless it is listed. Casts and numeric
arithmetic raise
+ * only in the ANSI eval mode they carry, but some expressions raise in any
mode, such as
+ * `element_at` with the index 0 and the arithmetic of intervals, so those
stay out.
+ */
+ private def cannotRaise(expr: Expression): Boolean =
+ !expr.exists(!nodeCannotRaise(_))
+
+ private def nodeCannotRaise(node: Expression): Boolean = {
+ def plainArithmetic(a: BinaryExpression, mode: EvalMode.Value): Boolean =
+ mode != EvalMode.ANSI && a.dataType.isInstanceOf[NumericType]
+ node match {
+ case _: Literal | _: AttributeReference | _: BoundReference | _:
GetStructField => true
+ case _: BinaryComparison | _: IsNull | _: IsNotNull | _: Not | _: And |
_: Or | _: In |
+ _: InSet | _: IsNaN | _: Coalesce | _: If | _: CaseWhen =>
+ true
+ case _: Length | _: Upper | _: Lower | _: Substring | _: StartsWith | _:
EndsWith |
+ _: Contains =>
+ true
+ case c: Cast => evalMode(c) != CometEvalMode.ANSI
Review Comment:
A non-ANSI cast can still raise. `Cast` from `DATE` to `TIMESTAMP` or
`TIMESTAMP_NTZ` goes through `daysToMicros` and `instantToMicros`, which use
`Math.multiplyExact`, so a date past about year 294247 (dates go up to 5881580)
throws `ArithmeticException` in every eval mode. With such a cast shared
between `na.drop` children, Spark raises on every row while native evaluation
would skip it. The comment on `keyCastCannotFail` in `CometCast.scala` already
calls out these date and timestamp conversions. Could this arm also exclude
casts to or from date, timestamp and interval types, so the motivating `string`
to `int` cast keeps the skip and the list stays closed for the conversions that
overflow? I have not run it.
--
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]
