feat: Support decimal type for the Spark checked_add and checked_subtract functions by n0r0shi · Pull Request #16302 · facebookincubator/velox

n0r0shi · 2026-02-08T16:39:59Z

Summary

Add checked_add and checked_subtract for decimal types that throw on
overflow instead of returning null, enabling Spark ANSI mode support for
decimal arithmetic

Test plan

Normal cases: all 4 input type combinations (short+short, short+long,
long+short, long+long)
Edge cases: add/subtract zero, negative numbers
Precision capped at 38, no overflow (small values and near-boundary)
Positive overflow throws error
Negative overflow throws error
Boundary overflow (max +/- 1) throws error

Closes #16301

meta-cla · 2026-02-08T16:40:05Z

Hi @kaishu-dev!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

netlify · 2026-02-08T16:40:05Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`f97bc38`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/69a87077ec05ac00082a106a

rui-mo

Thanks for this work! Please also update the relevant documentation.

https://github.com/facebookincubator/velox/blob/main/velox/docs/functions/spark/decimal.rst#arithmetic-functions

velox/functions/sparksql/DecimalArithmetic.cpp

n0r0shi · 2026-02-13T18:40:28Z

Updated the doc. Unchecked add and subtract were missing, so I added them as well. Please let me know if there's anything else needed for this PR.

Thanks! @rui-mo

Add checked decimal add and subtract functions that throw on overflow instead of returning null. These are needed for Spark's ANSI mode where arithmetic overflow should raise an error rather than silently produce null.

rui-mo

Thanks. I added some nits and this change LGTM overall.

velox/docs/functions/spark/decimal.rst

velox/functions/sparksql/tests/DecimalArithmeticTest.cpp

velox/functions/sparksql/DecimalArithmetic.cpp

…_add/subtract

n0r0shi · 2026-02-21T03:39:24Z

@rui-mo I believe I've addressed all the review comments. We'll proceed with the checked_* approach for add and subtract, as aligned in the discussion on #16361. Please let me know if there's anything else that needs to be updated. Thanks!

rui-mo

Thanks for all your efforts!

rui-mo · 2026-02-23T17:20:40Z

velox/docs/functions/spark/decimal.rst

+The allow-precision-loss flag applies to both regular and checked (ANSI mode) arithmetic functions.
+In Spark, there are no separate checked expression classes. The same expression (e.g., ``Add``)
+handles both ANSI and non-ANSI behavior, controlled by an ``EvalMode`` flag. In Velox, the checked
+variants are registered as separate functions (e.g., ``checked_add``, ``checked_subtract``).


nit: perhaps mention that they are registered as separate functions to support the TRY evaluation mode.

addressed in 8dc879f

@n0r0shi Thanks. There are some workflow failures:
https://github.com/facebookincubator/velox/actions/runs/22347057553/job/64672777202?pr=16302 and format issues. Would you please take a look?

I just added a fix in 28d89e9. I will check the next run, thanks.

n0r0shi · 2026-02-26T18:38:27Z

Fixed the format issue and AddSubtractArgTypesGenerator in 28d89e9. Thanks!

n0r0shi · 2026-02-27T03:16:56Z

There are 3 failures at this point, as far as I checked

Fuzzer Jobs / Spark Aggregate Fuzzer (pull_request)

20260226 14:12:32.165205 19022 File.cpp:254] preadv failed with error: Bad file descriptor

this can be related to #16543

Linux Build using GCC / Build with GCC / Linux release with adapters (pull_request)
Linux Build using GCC / Build with GCC / Ubuntu debug with resolve_dependency (pull_req..

[  FAILED  ] TaskTest.addSplitAfterBarrier

which looks relevant to #16510

So it doesn't look like my PR has introduced these failures. Please let me know otherwise @rui-mo

rui-mo · 2026-02-27T10:22:40Z

@n0r0shi Can you please rebase to latest main? The Linux test failures should be fixed, and the Aggregate fuzzer issue is going to be fixed in #16548.

rui-mo · 2026-02-27T10:24:26Z

velox/functions/sparksql/tests/DecimalArithmeticTest.cpp

+      std::optional<T> t,
+      std::optional<U> u) {
+    return evaluateOnce<int128_t>(
+        "checked_subtract(c0, c1)", {tType, uType}, t, u);


Please also review this comment: #16307 (comment), and consider adding try_* tests.

Added try_* tests in 69afcb2. Also consolidated the default and denyPrecisionLoss cases — they now share a common helper that tests the same core scenarios, while denyPrecisionLoss retains additional test cases for its specific behavior.

rui-mo

Thanks!

meta-codesync · 2026-03-02T19:42:39Z

@kKPulla has imported this pull request. If you are a Meta employee, you can view this in D94939837.

n0r0shi · 2026-03-03T08:20:04Z

Some checks failed but they don't appear related to my change — do I need to take any action?

kKPulla · 2026-03-03T21:42:20Z

Some checks failed but they don't appear related to my change — do I need to take any action?

Yeah the failures are for the newly added tests failing in debug mode

Note: Google Test filter = DecimalArithmeticTest.checkedSubtractDenyPrecisionLoss
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from DecimalArithmeticTest
[ RUN      ] DecimalArithmeticTest.checkedSubtractDenyPrecisionLoss

fbcode/velox/functions/sparksql/DecimalArithmetic.cpp:184:40: runtime error: signed integer overflow: 0x4b3b4ca85a86c47a098a223fffffffff + 0x4b3b4ca85a86c47a098a223fffffffff cannot be represented in type '__int128'
    #0 0x7fa39b4562bb in __int128 facebook::velox::functions::sparksql::(anonymous namespace)::DecimalAddSubtractBase::addLargeNonNegative<__int128, __int128, __int128>(__int128, __int128, unsigned char, unsigned char, unsigned char, bool&) fbcode/velox/functions/sparksql/DecimalArithmetic.cpp:184

...

SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow fbcode/velox/functions/sparksql/DecimalArithmetic.cpp:184:40

We also have another internal CI that detects build speed regression for sparksql/ directory that failed. @kgpai any thoughts for that?

n0r0shi · 2026-03-03T22:38:40Z

It looks like the addition of whole parts at this line is undefined behavior when the sum overflows int128_t, and my test cases are the first to hit that path with values large enough to trigger it:

  const auto whole = TResult(aWhole) + TResult(bWhole) + TResult(carryToLeft);

I can wrap this with __builtin_add_overflow to detect the overflow safely:

  TResult whole;
  if (__builtin_add_overflow(TResult(aWhole), TResult(bWhole), &whole) ||
      __builtin_add_overflow(whole, TResult(carryToLeft), &whole)) {
    overflow = true;
    return 0;
  }

Does that look right to you? @kKPulla

n0r0shi · 2026-03-04T17:50:21Z

@kKPulla @kgpai Pushed the change above. Let me know if there's anything I should check regarding the build speed regression.

kKPulla · 2026-03-04T18:08:55Z

@kKPulla @kgpai Pushed the change above. Let me know if there's anything I should check regarding the build speed regression.

Thanks for the update. I am reimporting to run the CI again to confirm, will keep you posted.

kKPulla · 2026-03-04T21:57:13Z

@n0r0shi @rui-mo The test failures seem to have been fixed but the build speed regression is still there. Looking further into the change, it is very likely because of the templatizing we are doing in this PR. If you have any ways to optimize build speed for sparksql functions, would appreciate if you can look into it as we are trying to reduce build speeds generally in Velox. cc: @kgpai

n0r0shi · 2026-03-05T03:42:57Z

Thanks for looking into this, @kKPulla @kgpai.

The regression comes from 20 new SimpleFunctionAdapter instantiations (4 registerDecimalBinary calls × 5 type combinations each), following the exact same pattern as the existing non-checked add/subtract/multiply/divide in this file. I looked into reducing the cost but the only options would sacrifice runtime performance or change the registration framework architecture.

These checked functions are necessary for Spark ANSI and TRY mode support. Build time improvements are worth pursuing separately at the framework level, but I don't think individual feature work should be blocked by costs inherent to the existing registration pattern.

Let me know how you'd like to proceed.

kgpai · 2026-03-05T05:57:16Z

@kKPulla Lets go ahead and merge this and pursue build speed improvements as a separate overall topic.

meta-codesync · 2026-03-05T07:21:48Z

@kKPulla merged this pull request in 7bc1be1.

Routes decimal `Add` and `Subtract` to Velox's `checked_add` and `checked_subtract` functions when ANSI mode is enabled (`nullOnOverflow = false`). These checked variants throw on overflow instead of returning null, matching Spark's ANSI behavior. Depends on facebookincubator/velox#16302 which adds `checked_add` and `checked_subtract` support for decimal types.

Resolve conflicts with merged checked_add/checked_subtract (PR facebookincubator#16302). Update multiply tests to use generic checkedDecimalArithmetic helpers.

n0r0shi requested review from jinchengchenghh, rui-mo and zhli1142015 as code owners February 8, 2026 16:40

n0r0shi force-pushed the checked-decimal-add-subtract branch from e3d80dc to b22b85d Compare February 8, 2026 16:46

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 8, 2026

n0r0shi force-pushed the checked-decimal-add-subtract branch 5 times, most recently from 9b2aa0e to bbe0e86 Compare February 9, 2026 07:10

This was referenced Feb 9, 2026

Add checked_multiply for Spark decimal types #16306

Open

Add checked_divide for Spark decimal types" #16322

Open

rui-mo reviewed Feb 10, 2026

View reviewed changes

velox/functions/sparksql/DecimalArithmetic.cpp Show resolved Hide resolved

FelixYBW mentioned this pull request Feb 10, 2026

[VL] useful Velox PRs not merged into upstream apache/incubator-gluten#11585

Open

n0r0shi mentioned this pull request Feb 12, 2026

Add decimal remainder and checked_remainder for Spark arithmetic #16354

Open

n0r0shi force-pushed the checked-decimal-add-subtract branch 2 times, most recently from 4181b00 to 22c7417 Compare February 13, 2026 18:35

n0r0shi mentioned this pull request Feb 14, 2026

feat: Add ANSI mode support for Spark arithmetic operations #16361

Open

n0r0shi force-pushed the checked-decimal-add-subtract branch from 22c7417 to 8805bc2 Compare February 14, 2026 05:13

rdtr mentioned this pull request Feb 14, 2026

feat: Add checked_multiply for Spark decimal arithmetic #16307

Open

6 tasks

rui-mo reviewed Feb 16, 2026

View reviewed changes

velox/docs/functions/spark/decimal.rst Show resolved Hide resolved

velox/functions/sparksql/tests/DecimalArithmeticTest.cpp Outdated Show resolved Hide resolved

velox/functions/sparksql/DecimalArithmetic.cpp Show resolved Hide resolved

n0r0shi added 2 commits February 19, 2026 06:09

address PR comments: add denyPrecisionLoss tests and docs for checked…

b3a57e0

…_add/subtract

address PR commits: reorder decimal arithmetic functions alphabetically

a263e60

Merge branch 'main' into checked-decimal-add-subtract

c524ce2

rui-mo approved these changes Feb 23, 2026

View reviewed changes

n0r0shi and others added 2 commits February 24, 2026 02:34

address PR commits: update the doc

8dc879f

Fix formatting and tests

28d89e9

Merge branch 'main' into checked-decimal-add-subtract

db5f487

rui-mo reviewed Feb 27, 2026

View reviewed changes

n0r0shi and others added 2 commits February 27, 2026 19:06

Merge branch 'main' into checked-decimal-add-subtract

b27e8ff

add test cases for try, consolidate allow/deny precisionLoss tests

69afcb2

zhli1142015 approved these changes Feb 28, 2026

View reviewed changes

rui-mo changed the title ~~feat: Add checked_add and checked_subtract for Spark decimal types~~ feat: Support decimal type for the Spark checked_add and checked_subtract functions Mar 2, 2026

rui-mo approved these changes Mar 2, 2026

View reviewed changes

rui-mo added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Mar 2, 2026

Fix UBSan error

f97bc38

meta-codesync bot closed this in 7bc1be1 Mar 5, 2026

facebook-github-bot added the Merged label Mar 5, 2026

n0r0shi mentioned this pull request Mar 5, 2026

[VL] Support ANSI mode decimal Add/Subtract with checked overflow apache/incubator-gluten#11705

Draft

4 tasks

Conversation

n0r0shi commented Feb 8, 2026 • edited by rui-mo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

meta-cla bot commented Feb 8, 2026

Action Required

Process

Uh oh!

netlify bot commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

n0r0shi commented Feb 13, 2026

Uh oh!

rui-mo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

n0r0shi commented Feb 21, 2026

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

rui-mo Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

n0r0shi Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

rui-mo Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

n0r0shi Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

n0r0shi commented Feb 26, 2026

Uh oh!

n0r0shi commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rui-mo commented Feb 27, 2026

Uh oh!

rui-mo Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

n0r0shi Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

meta-codesync bot commented Mar 2, 2026

Uh oh!

n0r0shi commented Mar 3, 2026

Uh oh!

kKPulla commented Mar 3, 2026

Uh oh!

n0r0shi commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n0r0shi commented Mar 4, 2026

Uh oh!

kKPulla commented Mar 4, 2026

Uh oh!

kKPulla commented Mar 4, 2026

Uh oh!

n0r0shi commented Mar 5, 2026

Uh oh!

kgpai commented Mar 5, 2026

Uh oh!

meta-codesync bot commented Mar 5, 2026

n0r0shi commented Feb 8, 2026 •

edited by rui-mo

Loading

netlify bot commented Feb 8, 2026 •

edited

Loading

rui-mo left a comment •

edited

Loading

n0r0shi commented Feb 27, 2026 •

edited

Loading

n0r0shi commented Mar 3, 2026 •

edited

Loading