You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "rdtr (via GitHub)" <gi...@apache.org> on 2026/10/01 07:04:06 UTC

[PR] [GLUTEN-10134][VL] Throw on decimal Add/Subtract/Multiply overflow in ANSI mode [gluten]

rdtr opened a new pull request, #13170:
URL: https://github.com/apache/gluten/pull/13170

    ## What changes are proposed in this pull request?
   
   This continues https://github.com/apache/gluten/pull/12192, which looks stale and upstream changes now require an overhaul so I created a separate PR here.
   
     With `spark.sql.ansi.enabled=true` and ANSI fallback disabled, decimal Add, Subtract and Multiply return NULL on overflow instead of throwing. For example:
   
         SELECT CAST(99999999999999999999999999999999999999 AS DECIMAL(38,0)) + CAST(1 AS DECIMAL(38,0))
   
     returns NULL on Velox, and Spark then fails with `NullPointerException` while decoding the non-nullable result.
   
     #12192 handled this in a `CheckOverflow(BinaryArithmetic, ...)`. On Spark 3.4+, `CheckOverflow` no longer wraps arithmetic (see #13002). This PR instead passes the arithmetic expression to `getDecimalArithmeticExprName`, as @philo-he suggested on #12192. The Velox backend maps ANSI-mode decimal Add, Subtract and Multiply to Velox's `checked_add`, `checked_subtract` and `checked_multiply`, which throw on overflow.
   
   The ClickHouse and Bolt backends keep their current function names. TRY mode is unchanged, because the regular decimal functions already return NULL on overflow.
   
     Part of #10134.
   
     ## How was this patch tested?
   
     Added decimal overflow and `try_*` tests to `ArithmeticAnsiValidateSuite`, which runs with ANSI fallback disabled, including a test with `allowPrecisionLoss` disabled that also
     checks the query runs on Velox. The overflow tests fail without this change.
   
     ## Was this patch authored or co-authored using generative AI tooling?
   
     Generated-by: Claude Code (Claude Opus 5.5)
   


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUTEN-10134][VL] Throw on decimal Add/Subtract/Multiply overflow in ANSI mode [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13170:
URL: https://github.com/apache/gluten/pull/13170#issuecomment-5926427207

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUTEN-10134][VL] Throw on decimal Add/Subtract/Multiply overflow in ANSI mode [gluten]

Posted by "philo-he (via GitHub)" <gi...@apache.org>.
philo-he merged PR #13170:
URL: https://github.com/apache/gluten/pull/13170


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUTEN-10134][VL] Throw on decimal Add/Subtract/Multiply overflow in ANSI mode [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13170:
URL: https://github.com/apache/gluten/pull/13170#issuecomment-6047279558

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUTEN-10134][VL] Throw on decimal Add/Subtract/Multiply overflow in ANSI mode [gluten]

Posted by "philo-he (via GitHub)" <gi...@apache.org>.
philo-he commented on code in PR #13170:
URL: https://github.com/apache/gluten/pull/13170#discussion_r4180491011


##########
backends-velox/src/test/scala/org/apache/gluten/functions/ArithmeticAnsiValidateSuite.scala:
##########
@@ -93,4 +93,137 @@ class ArithmeticAnsiValidateSuite extends FunctionsValidateSuite {
     }
   }
 
+  test("decimal add overflow") {
+    // Normal decimal add should succeed and match Spark results
+    runQueryAndCompare(
+      "SELECT CAST(1.0 AS DECIMAL(10,2)) + CAST(2.0 AS DECIMAL(10,2))") {
+      checkGlutenPlan[ProjectExecTransformer]
+    }
+
+    // Overflow: max DECIMAL(38,0) + 1 should throw in ANSI mode
+    if (isSparkVersionGE("4.0")) {

Review Comment:
   Can we use withSQLConf to test ANSI ON and OFF instead? This is more explicit and allows testing non-ANSI behavior even when the support for Spark 3.5 and earlier versions is dropped in the future.
   
   Ditto for other code where if (isSparkVersionGE("4.0")) is used.



##########
backends-velox/src/test/scala/org/apache/gluten/functions/ArithmeticAnsiValidateSuite.scala:
##########
@@ -93,4 +93,137 @@ class ArithmeticAnsiValidateSuite extends FunctionsValidateSuite {
     }
   }
 
+  test("decimal add overflow") {
+    // Normal decimal add should succeed and match Spark results
+    runQueryAndCompare(
+      "SELECT CAST(1.0 AS DECIMAL(10,2)) + CAST(2.0 AS DECIMAL(10,2))") {
+      checkGlutenPlan[ProjectExecTransformer]
+    }
+
+    // Overflow: max DECIMAL(38,0) + 1 should throw in ANSI mode
+    if (isSparkVersionGE("4.0")) {

Review Comment:
   Can we use withSQLConf to test ANSI ON and OFF instead? This is more explicit and allows testing non-ANSI behavior even when the support for Spark 3.5 and earlier versions is dropped in the future.
   
   Ditto for other test code where if (isSparkVersionGE("4.0")) is used.



-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [GLUTEN-10134][VL] Throw on decimal Add/Subtract/Multiply overflow in ANSI mode [gluten]

Posted by "rdtr (via GitHub)" <gi...@apache.org>.
rdtr commented on code in PR #13170:
URL: https://github.com/apache/gluten/pull/13170#discussion_r4212250035


##########
backends-velox/src/test/scala/org/apache/gluten/functions/ArithmeticAnsiValidateSuite.scala:
##########
@@ -93,4 +93,137 @@ class ArithmeticAnsiValidateSuite extends FunctionsValidateSuite {
     }
   }
 
+  test("decimal add overflow") {
+    // Normal decimal add should succeed and match Spark results
+    runQueryAndCompare(
+      "SELECT CAST(1.0 AS DECIMAL(10,2)) + CAST(2.0 AS DECIMAL(10,2))") {
+      checkGlutenPlan[ProjectExecTransformer]
+    }
+
+    // Overflow: max DECIMAL(38,0) + 1 should throw in ANSI mode
+    if (isSparkVersionGE("4.0")) {

Review Comment:
   Thanks for the review! Addressed now. All the `isSparkVersionGE("4.0")` checks in this suite are replaced by one helper, `checkOverflow`, which runs each overflow case with `withSQLConf`:
   
   - ANSI on: the query throws, and the message is Velox's error (for example "Decimal overflow in add"), so we know it ran in Velox.
   - ANSI off: the result matches Spark (NULL for decimals, a wrapped value for integers), and the plan has `ProjectExecTransformer`.
   
   I also fixed the existing `multiply` test, which checked `2147483647 + 1` instead of a multiplication. I ran the suite on Spark 3.5 and 4.1.
   



-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org