andygrove commented on code in PR #5225:
URL: https://github.com/apache/datafusion-comet/pull/5225#discussion_r3706832148
##########
spark/src/main/scala/org/apache/comet/serde/arithmetic.scala:
##########
@@ -263,7 +263,7 @@ object CometDivide extends CometExpressionSerde[Divide]
with MathBase {
// For now, use NullIf to swap zeros with nulls.
val rightExpr =
if (expr.evalMode != EvalMode.ANSI) nullIfWhenPrimitive(expr.right) else
expr.right
- val divideExpr = createMathExpression(
+ createMathExpression(
Review Comment:
One thing I want to check here. The guard you removed fired on
`expr.dataType.isInstanceOf[DecimalType]` alone. The `promote` rule requires
both operands to match `DecimalExpression` as well, and `getSupportLevel` only
looks at `expr.left.dataType`. So if a decimal-typed `Divide` with a
non-decimal operand ever reaches this serde, there is no `CheckOverflow`
anywhere and the native `i128::MAX` sentinel comes back as a real value.
I could not construct that shape through Spark's type coercion, so I believe
it is unreachable today. Since the failure mode is silent wrong data rather
than a fallback, would you be up for a defensive guard? Returning `Unsupported`
from `getSupportLevel` when `expr.dataType` is decimal but the operands would
not match `promote`'s pattern would keep the invariant checkable next to the
code that depends on it.
##########
spark/src/test/spark-4.1+/org/apache/spark/sql/comet/CometDecimalArithmeticViewSuite.scala:
##########
@@ -119,4 +104,38 @@ class CometDecimalArithmeticViewSuite extends
CometTestBase {
}
}
}
+
+ test("issue #5190: recursive serialization does not duplicate decimal
CheckOverflow") {
+ val left = "CAST(id AS DECIMAL(10, 0))"
+ val right = "CAST(id + 1 AS DECIMAL(10, 0))"
+ val operations: Seq[(String, Boolean, String, ExprOuterClass.Expr =>
Boolean, Boolean)] = Seq(
Review Comment:
This tuple has two unlabeled booleans in it, and `("divide TRY", true, ...,
false)` is hard to read without counting positions back to the destructuring on
the next block. A small case class with named fields, or moving `ansiEnabled`
and `failOnError` next to their names, would make this easier to extend when
someone adds a case later.
##########
spark/src/test/spark-4.1+/org/apache/spark/sql/comet/CometDecimalArithmeticViewSuite.scala:
##########
@@ -119,4 +104,38 @@ class CometDecimalArithmeticViewSuite extends
CometTestBase {
}
}
}
+
+ test("issue #5190: recursive serialization does not duplicate decimal
CheckOverflow") {
Review Comment:
This regression does not depend on anything 4.1-specific, but sitting under
`spark-4.1+` means 3.4, 3.5, and 4.0 CI never runs it, and the duplication it
guards against exists on all of them. Could it move to a version-agnostic suite
under `spark/src/test/scala/org/apache/spark/sql/comet/`? A new
`CometDecimalPromotionSuite` would work. The other two tests in this file can
stay where they are since they need `NumericEvalContext`.
While it is moving, two additions would be worth it. ANSI is only exercised
for `divide`, but the collapse guard compares `nullOnOverflow`, so ANSI is
exactly the case that would catch a wrong guard for add, subtract, multiply,
and remainder. And a direct assertion that `promote(promote(e)) == promote(e)`
would state the idempotency property outright instead of leaving it to be
inferred from the `array_contains` proto shape.
##########
spark/src/main/scala/org/apache/spark/sql/comet/DecimalPrecision.scala:
##########
@@ -45,6 +45,15 @@ object DecimalPrecision {
// happen if the Spark version is < 3.4
case e: BinaryArithmetic if e.left.prettyName == "promote_precision" => e
+ // Recursive exprToProto calls can re-promote every decimal binary
operator below. Collapse
+ // only equivalent wrappers so the shared promotion rule remains
idempotent.
+ case outer @ CheckOverflow(
Review Comment:
This is a good fix, and the bottom-up collapse converges correctly at any
depth.
The re-promotion it works around comes from serdes calling the public
`exprToProto` on children that `promote` has already visited. There are 64 such
call sites in the serde package. The aggregate ones genuinely need the
promoting entry point, but the ones in `arrays.scala` and `bitwise.scala` are
re-walking an already-promoted tree, and each walk drags `liftCoverageTags`
along with it. That is separate work from this PR, but could you file a
tracking issue for it and link it here? Otherwise it will not get picked up.
--
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]
