0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4307693702
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1486,6 +1487,111 @@ class CometExpressionSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
}
}
+ test("division on negative-scale decimal reports unsupported via
getSupportLevel") {
+ // Issue #5013: `spark_decimal_div` computes its scale-alignment exponents
in `u32`, so a
+ // negative scale wraps to roughly 4.29e9 and the BigInt branch attempts
an unbounded
+ // allocation. Only division is affected. The other arithmetic operators
scale-align by
+ // `10^(max_scale - s)`, which is never negative, so they stay native and
are pinned by
+ // "safe ops on negative-scale decimal run natively" below. Constructing
the expressions
+ // requires allowNegativeScaleOfDecimal so DecimalType(_, s<0) passes
Spark's own check.
+ withSQLConf("spark.sql.legacy.allowNegativeScaleOfDecimal" -> "true") {
+ Seq(DecimalType(10, -1), DecimalType(20, -5)).foreach { negScale =>
+ val posDec = DecimalType(10, 0)
+ val negAttr = AttributeReference("n", negScale)()
+ val posAttr = AttributeReference("p", posDec)()
+ val expected =
Unsupported(Some(CometDivide.negScaleDecimalArithmeticReason))
+
+ assert(CometDivide.getSupportLevel(Divide(negAttr, posAttr)) ==
expected)
+ assert(CometIntegralDivide.getSupportLevel(IntegralDivide(negAttr,
posAttr)) == expected)
+ // Guard also fires when negative scale is only on the right operand.
+ assert(CometDivide.getSupportLevel(Divide(posAttr, negAttr)) ==
expected)
+
+ // The unguarded operators must not report unsupported for the same
operands.
+ assert(CometAdd.getSupportLevel(Add(negAttr, posAttr)) == Compatible())
+ assert(CometSubtract.getSupportLevel(Subtract(negAttr, posAttr)) ==
Compatible())
+ assert(CometRemainder.getSupportLevel(Remainder(negAttr, posAttr)) ==
Compatible())
+ assert(CometMultiply.getSupportLevel(Multiply(negAttr, posAttr)) ==
Compatible())
+ }
+ }
+ }
+
+ test("division on negative-scale decimal falls back and returns correct
results") {
+ // End-to-end proof for issue #5013 that the previously-panicking queries
now complete.
+ // `checkSparkAnswerAndFallbackReason` rather than `checkSparkAnswer`:
these five serdes do
+ // not mix in `CodegenDispatchFallback`, so `Unsupported` falls the whole
projection back to
Review Comment:
All three updated. "Five serdes" is two. The `WideDecimalBinaryExpr` note
described a native path this test never reaches, so it is gone rather than
reworded. The references to "an earlier revision of this PR" and "this PR
rewrites" now describe the code as it stands, naming the two width tests
directly.
--
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]
