Skip to content

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

Open
malinjawi wants to merge 7 commits intofacebookincubator:mainfrom
malinjawi:spark-arithmetic-ansi-mode
Open

feat: Add ANSI mode support for Spark arithmetic operations#16361
malinjawi wants to merge 7 commits intofacebookincubator:mainfrom
malinjawi:spark-arithmetic-ansi-mode

Conversation

@malinjawi
Copy link
Contributor

Add ANSI mode support for arithmetic operations (add, subtract, multiply, divide, unary minus) and interval arithmetic in Spark dialect.

In ANSI mode:

  • Overflow throws errors instead of wrapping around
  • Division by zero throws errors instead of returning NULL
  • MIN_VALUE / -1 overflow throws errors

In non-ANSI mode (default):

  • Overflow wraps around (two's complement)
  • Division by zero returns NULL
  • MIN_VALUE / -1 returns MIN_VALUE

Key changes:

  • Add IntegerDivideFunction with ANSI overflow and division-by-zero checks
  • Update DivideFunction to support both float and double
  • Add ANSI overflow checks to Add, Subtract, Multiply functions
  • Add ANSI support for interval arithmetic (IntervalDayTime, IntervalYearMonth)
  • Add comprehensive tests for all arithmetic operations
  • Update documentation with ANSI behavior examples

Addresses: apache/incubator-gluten#10134

Add ANSI mode support for arithmetic operations (add, subtract, multiply,
divide, unary minus) and interval arithmetic in Spark dialect.

In ANSI mode:
- Overflow throws errors instead of wrapping around
- Division by zero throws errors instead of returning NULL
- MIN_VALUE / -1 overflow throws errors

In non-ANSI mode (default):
- Overflow wraps around (two's complement)
- Division by zero returns NULL
- MIN_VALUE / -1 returns MIN_VALUE

Key changes:
- Add IntegerDivideFunction with ANSI overflow and division-by-zero checks
- Update DivideFunction to support both float and double
- Add ANSI overflow checks to Add, Subtract, Multiply functions
- Add ANSI support for interval arithmetic (IntervalDayTime, IntervalYearMonth)
- Add comprehensive tests for all arithmetic operations
- Update documentation with ANSI behavior examples

Addresses: apache/incubator-gluten#10134
@netlify
Copy link

netlify bot commented Feb 12, 2026

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 1b83a30
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/69ac545a5fd6ab00087fbd3d

@meta-cla 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 12, 2026
@n0r0shi
Copy link
Contributor

n0r0shi commented Feb 14, 2026

Just a heads-up, ANSI-mode support for non-decimal integral arithmetic already exists via the checked_* functions. checked_add, checked_subtract, checked_multiply, checked_divide, and checked_div are already registered in Velox [link], and Gluten already selects between checked and unchecked versions based on Spark's evalMode [link].

This PR introduces a different approach by reading spark.ansi_enabled config inside the functions themselves. This is similar to how CAST currently works in SparkCastExpr, which reads sparkAnsiEnabled to decide between cast and try_cast behavior [link].

However, the checked_* pattern is arguably the better direction — it keeps Velox functions config-agnostic and lets the caller (Gluten) make the ANSI decision. Introducing the config-based approach to arithmetic would result in two competing patterns, which could be confusing to maintain. I am also planning to migrate CAST itself to the checked_* approach so that Velox no longer needs to read sparkAnsiEnabled at all.

I have a series of PRs extending the checked_* pattern to decimal arithmetic and other operators (#16302, #16307, #16323, and more to follow).

Happy to follow whatever direction the maintainers prefer. cc @rui-mo

@malinjawi
Copy link
Contributor Author

Just a heads-up, ANSI-mode support for non-decimal integral arithmetic already exists via the checked_* functions. checked_add, checked_subtract, checked_multiply, checked_divide, and checked_div are already registered in Velox [link], and Gluten already selects between checked and unchecked versions based on Spark's evalMode [link].

This PR introduces a different approach by reading spark.ansi_enabled config inside the functions themselves. This is similar to how CAST currently works in SparkCastExpr, which reads sparkAnsiEnabled to decide between cast and try_cast behavior [link].

However, the checked_* pattern is arguably the better direction — it keeps Velox functions config-agnostic and lets the caller (Gluten) make the ANSI decision. Introducing the config-based approach to arithmetic would result in two competing patterns, which could be confusing to maintain. I am also planning to migrate CAST itself to the checked_* approach so that Velox no longer needs to read sparkAnsiEnabled at all.

I have a series of PRs extending the checked_* pattern to decimal arithmetic and other operators (#16302, #16307, #16323, and more to follow).

Happy to follow whatever direction the maintainers prefer. cc @rui-mo

You're absolutely right, and thank you for catching this! I didn't realize that checked_add, checked_subtract, checked_multiply, and checked_divide already exist in Velox and that Gluten already selects between them based on evalMode.

I reviewed your PRs (#16302, #16307, #16323) and see that you're extending this pattern to decimal arithmetic, which is the correct approach. I also see that @rui-mo is approving this direction.

I'll refactor my PR to:

  1. Remove redundant changes to binary arithmetic operations (Add, Subtract, Multiply, Divide) - since checked_* versions already exist
  2. Create checked_unaryminus following the same pattern as CheckedAddFunction - always throw on overflow, no config reading
  3. Follow the established pattern for any other operations that need ANSI support

One question: I notice AbsFunction currently uses the config-based approach (reads sparkAnsiEnabled()). Should that also be migrated to checked_abs for consistency with the rest of the arithmetic operations?

Thanks again for the feedback.

cc @rui-mo

@rui-mo
Copy link
Collaborator

rui-mo commented Feb 18, 2026

However, the checked_* pattern is arguably the better direction — it keeps Velox functions config-agnostic and lets the caller (Gluten) make the ANSI decision. Introducing the config-based approach to arithmetic would result in two competing patterns, which could be confusing to maintain. I am also planning to migrate CAST itself to the checked_* approach so that Velox no longer needs to read sparkAnsiEnabled at all.

Hi @n0r0shi Sorry, I just saw your message. Could you help me understand the motivation for making the functions config-agnostic, given that Spark already controls the behavior via configuration? I don’t think ANSI ON/OFF functions should co-exist, and using a configuration in Velox to control ANSI behavior shouldn’t confuse users, since it follows Spark and keeps the behavior consistent with Spark. How do you think? cc: @zhli1142015 @jinchengchenghh

Please also note that the checked_* pattern had been introduced to support several try_* functions, and were not intended for the ANSI support, see #16403 (comment).

Please feel free to open a discussion in Gluten to gather more opinions from other maintainers as well.

@rui-mo
Copy link
Collaborator

rui-mo commented Feb 18, 2026

I would +1 moving forward with the approach proposed in this PR.

@malinjawi malinjawi force-pushed the spark-arithmetic-ansi-mode branch from 7f44d6c to 78b111e Compare February 19, 2026 17:44
@malinjawi
Copy link
Contributor Author

However, the checked_* pattern is arguably the better direction — it keeps Velox functions config-agnostic and lets the caller (Gluten) make the ANSI decision. Introducing the config-based approach to arithmetic would result in two competing patterns, which could be confusing to maintain. I am also planning to migrate CAST itself to the checked_* approach so that Velox no longer needs to read sparkAnsiEnabled at all.

Hi @n0r0shi Sorry, I just saw your message. Could you help me understand the motivation for making the functions config-agnostic, given that Spark already controls the behavior via configuration? I don’t think ANSI ON/OFF functions should co-exist, and using a configuration in Velox to control ANSI behavior shouldn’t confuse users, since it follows Spark and keeps the behavior consistent with Spark. How do you think? cc: @zhli1142015 @jinchengchenghh

Please also note that the checked_* pattern had been introduced to support several try_* functions, and were not intended for the ANSI support, see #16403 (comment).

Please feel free to open a discussion in Gluten to gather more opinions from other maintainers as well.

Thanks all for the context. I’ve updated the branch.

Before I proceed further, I’d like to align on direction for ANSI arithmetic in Velox: config‑based behavior inside functions (as in this PR) vs. checked_* selection by caller. I’m happy to follow whichever approach maintainers prefer.

For context, I also opened #16402 for checked interval arithmetic (checked_* pattern) in case that direction is preferred for intervals.

Would it help to loop in anyone else from Gluten or Spark folks who’ve weighed in on ANSI behavior? I can tag them or start a short discussion thread if useful.

cc: @n0r0shi @rui-mo @zhli1142015 @jinchengchenghh @PHILO-HE

@malinjawi malinjawi marked this pull request as ready for review February 19, 2026 17:56
@n0r0shi
Copy link
Contributor

n0r0shi commented Feb 20, 2026

@rui-mo Thank you for the feedback and for sharing the context.

When I started the work, what I observed was that checked_* for arithmetic is already the established pattern for ANSI support. In Velox, checked_add, checked_subtract, checked_multiply, checked_divide, and checked_div are registered for all integer and floating-point types, and on the Gluten side, genArithmeticTransformer already routes ANSI-mode arithmetic to these checked_* functions for all types:
https://github.com/apache/incubator-gluten/blob/a6739880664ecb7de06f0d888373b4e3bb897206/backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala#L154-L155

So it was a natural extension to me to support the same pattern on the Velox side to make it work for decimal. Unfortunately, in fact we already have two ways of supporting ANSI mixed: abs uses the config-based approach (sparkAnsiEnabled), while the binary arithmetic operators use checked_*.

Cast is a different beast, which has more to do depending on a combination of input types with ANSI mode — config-based makes sense there given the complexity.

Can we keep checked_* approach for the arithmetic operators specifically? checked_* must exist to support try_* as you said. Since checked_* already provides the exact throw-on-overflow behavior that ANSI needs, adding a config-based branch in the non-checked version would duplicate what the checked version already does, especially given that we can't eliminate the checked version anyway.

This has minimal impact on the code already merged (for both Gluten and Velox), I can check other PRs of mine and change to config based for the ones who don't have try_* if we agree on this approach.

@rui-mo
Copy link
Collaborator

rui-mo commented Feb 20, 2026

@n0r0shi Yes, it makes sense to treat try_* as special cases by supporting them through the corresponding checked_* functions. As you pointed out, separate checked implementations are unavoidable in this case. Given that, it is reasonable for me to proceed with your PRs for checked_add, checked_subtract, checked_divide, and checked_multiply to add decimal support.

@n0r0shi
Copy link
Contributor

n0r0shi commented Feb 21, 2026

@rui-mo @malinjawi Thank you for the alignment.

Based on the discussion, here's how I plan to proceed:

I'll keep PRs for checked_* for the operators that have try_* variants (since separate checked implementations are unavoidable there):

I'll yield unaryminus to this PR — closing my #16356, since unaryminus doesn't have a try_* variant and the config-based approach in this PR is the right fit.

For remainder and pmod (#16403, #16405, #16406), I'll also move to config-based since they don't have try_* variants either. For the rest of my open PRs (e.g. checked_sum, checked_avg, checked_element_at), let's discuss the approach separately on each — they have different nuances that
are worth reviewing individually. I can implement in either way.

@n0r0shi
Copy link
Contributor

n0r0shi commented Mar 7, 2026

I noticed the unaryminus ANSI support in this PR hasn't been updated since our discussion. I went ahead and submitted it as a separate config-based PR #16672 — hope that's okay. Happy to coordinate if you had plans for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants