Fix #5164: Detect integer/long overflow using Calcite's native checked arithmetic

[penghuo]

Description

Use Calcite's built-in ConvertToChecked visitor to rewrite arithmetic operators
(PLUS/MINUS/TIMES) to their overflow-safe CHECKED variants. These generate
SqlFunctions.checkedPlus(int, int) → Math.addExact() calls that throw
ArithmeticException on integer/long overflow instead of silently wrapping.

Root cause: Standard Calcite operators (SqlStdOperatorTable.PLUS/MINUS/MULTIPLY)
generate plain Java +/-/* in the Enumerable code path, which silently wrap
on overflow (e.g., 2147483647 + 1 returns -2147483648). This is also the default
behavior in Calcite itself — confirmed by testing with CsvTest.

Approach: Calcite 1.41.0 provides a native solution via SqlConformance.checkedArithmetic()
and the ConvertToChecked RelNode visitor. This PR applies that visitor in the
query execution pipeline, controlled by a new plugins.calcite.checked_arithmetic.enabled
setting (default: true).

How it works:

1. After plan optimization, ConvertToChecked rewrites PLUS → CHECKED_PLUS,
   MINUS → CHECKED_MINUS, TIMES → CHECKED_MULTIPLY in the RelNode tree
2. Calcite's code generator produces SqlFunctions.checkedPlus(int, int) calls
   that use Math.addExact() internally
3. ArithmeticException is caught at the QueryService level and wrapped in
   NonFallbackCalciteException to prevent silent fallback to the V2 engine
4. Float/double arithmetic is unaffected (IEEE 754 behavior)

Comparison with custom UDF approach: This replaces the initial approach of creating
custom UDF operators (AddFunction, SubtractFunction, MultiplyFunction). The native
Calcite approach is simpler (46 lines vs 484 lines), avoids boxing overhead, and
also covers division and unary minus overflow.

Related Issues

#5164

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 59578cf)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add checked arithmetic setting and apply ConvertToChecked in query pipeline Relevant files: common/src/main/java/org/opensearch/sql/common/setting/Settings.java opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java core/src/main/java/org/opensearch/sql/executor/QueryService.java Sub-PR theme: Extend existing rules to handle CHECKED_PLUS/MINUS/TIMES SqlKind variants Relevant files: core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLAggregateConvertRule.java opensearch/src/main/java/org/opensearch/sql/opensearch/util/OpenSearchRelOptUtil.java Sub-PR theme: Add documentation and integration tests for overflow detection Relevant files: docs/user/admin/settings.rst docs/user/ppl/functions/expressions.md integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5164.yml

⚡ Recommended focus areas for review Exception Ordering The ArithmeticException catch block is placed before the general Throwable catch block, but it's important to verify that ArithmeticException thrown during execution (inside executionEngine.execute()) is also caught here. If the exception is thrown asynchronously (e.g., in a callback or separate thread), it may not be caught by this try-catch block and could still cause silent fallback or unhandled behavior. } catch (ArithmeticException e) { listener.onFailure( new NonFallbackCalciteException("Arithmetic overflow: " + e.getMessage(), e)); Inconsistent Default The isCheckedArithmeticEnabled() method returns true when settings is null (as a fallback default). However, other similar methods like isCalciteEnabled() return false or rely on the settings object directly. This inconsistency could cause unexpected behavior in test environments or when settings are not initialized. private boolean isCheckedArithmeticEnabled() { if (settings != null) { Boolean enabled = settings.getSettingValue(Settings.Key.CALCITE_CHECKED_ARITHMETIC); return enabled != null && enabled; } return true; // default: overflow checking enabled } Incorrect Description The documentation states that division (/) is also covered by overflow detection ("+, -, *, /"), but the PR description and code comments explicitly state that division is NOT supported (commented out in CONVERTABLE_FUNCTIONS). This is misleading and should be corrected. When enabled, integer and long arithmetic operations (``+``, ``-``, ``*``, ``/``) detect overflow and return an error instead of silently wrapping. For example, ``2147483647 + 1`` returns an error rather than ``-2147483648``. Floating-point (``float``, ``double``) arithmetic is unaffected and follows IEEE 754 behavior. Missing CHECKED_TIMES The condition at line 137-140 checks for PLUS, MINUS, CHECKED_PLUS, and CHECKED_MINUS, but does not include TIMES or CHECKED_TIMES. If the aggregate rule needs to handle multiplication similarly, this may be an oversight. Verify whether TIMES/CHECKED_TIMES should also be included in this condition. if (rexCall.getOperator().kind == SqlKind.PLUS || rexCall.getOperator().kind == SqlKind.MINUS || rexCall.getOperator().kind == SqlKind.CHECKED_PLUS || rexCall.getOperator().kind == SqlKind.CHECKED_MINUS) {

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 59578cf
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix inaccurate documentation about division overflow The documentation states that division (/) is also covered by checked arithmetic, but the code only applies CHECKED_PLUS, CHECKED_MINUS, and CHECKED_TIMES — there is no CHECKED_DIVIDE in the implementation. This is misleading and should be corrected to list only +, -, and *. docs/user/admin/settings.rst [974] -When enabled, integer and long arithmetic operations (``+``, ``-``, ``*``, ``/``) detect overflow and return an error instead of silently wrapping. +When enabled, integer and long arithmetic operations (``+``, ``-``, ``*``) detect overflow and return an error instead of silently wrapping. Suggestion importance[1-10]: 7 __ Why: The documentation incorrectly lists / as a checked arithmetic operation, but the code only implements CHECKED_PLUS, CHECKED_MINUS, and CHECKED_TIMES. This is a genuine documentation inaccuracy that could mislead users.	Medium
Possible issue	Add overflow error handling in explain path The ArithmeticException catch block is placed inside the try block that also handles the explainWithCalcite path, but the same overflow catch is missing in the explainWithCalcite method. The explainWithCalcite method applies ConvertToChecked but does not catch ArithmeticException, so overflow during explain will propagate as an unhandled exception rather than a proper error response. core/src/main/java/org/opensearch/sql/executor/QueryService.java [114-116] +} catch (ArithmeticException e) { + listener.onFailure( + new NonFallbackCalciteException("Arithmetic overflow: " + e.getMessage(), e)); +} catch (Throwable t) { - Suggestion importance[1-10]: 6 __ Why: The explainWithCalcite method applies ConvertToChecked but lacks an ArithmeticException catch block, meaning overflow during explain would propagate unhandled. The improved_code is identical to existing_code (it doesn't show the fix in explainWithCalcite), but the issue identified is real and valid.	Low
	Fix unsafe default when settings are null When settings is null (e.g., in tests or minimal configurations), the method defaults to true, enabling checked arithmetic unexpectedly. This could cause ArithmeticException in environments where settings are not configured, breaking existing behavior. The default should be false to avoid unintended overflow errors when no settings are present. core/src/main/java/org/opensearch/sql/executor/QueryService.java [297-303] private boolean isCheckedArithmeticEnabled() { if (settings != null) { Boolean enabled = settings.getSettingValue(Settings.Key.CALCITE_CHECKED_ARITHMETIC); return enabled != null && enabled; } - return true; // default: overflow checking enabled + return false; // default: overflow checking disabled when no settings available } Suggestion importance[1-10]: 4 __ Why: The suggestion argues that returning true when settings is null could cause unexpected ArithmeticException. However, the PR intentionally defaults to true (overflow checking enabled) as indicated by the comment, and the OpenSearchSettings default is also true. This is a design choice, not a bug, though the concern about test environments is valid.	Low

---

Previous suggestions

Suggestions up to commit 8714c88

Category	Suggestion	Impact
General	Avoid swallowing non-404 deletion errors Silently swallowing all exceptions during index deletion can hide real errors (e.g., network failures or permission issues). Consider catching only the specific ResponseException with a 404 status code to distinguish "index not found" from other failures. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteArithmeticOverflowIT.java [35-40] // Delete index if it exists from a previous test run try { client().performRequest(new Request("DELETE", "/" + TEST_INDEX)); -} catch (Exception e) { - // Index may not exist yet; ignore +} catch (org.opensearch.client.ResponseException e) { + if (e.getResponse().getStatusLine().getStatusCode() != 404) { + throw e; + } + // Index does not exist yet; ignore } Suggestion importance[1-10]: 4 __ Why: This is a valid improvement for test reliability — catching only ResponseException with a 404 status would prevent masking real errors like network failures. However, it's a test setup method and the impact is limited to test infrastructure quality.	Low
	Prevent unsafe operand reordering in mixed-type arithmetic The reverse() method captures sqlKind from the enclosing toUDF method parameter. Since sqlKind is a method parameter (not a final field), it is effectively final in Java, but it's worth confirming this is the intended sqlKind at the time of operator creation and not subject to mutation. More importantly, for SqlKind.PLUS, reversing to this is correct only if the operator is truly commutative in all type combinations — mixed-type arithmetic (e.g., int + double) may not be commutative due to type coercion order. Consider returning null for all cases to avoid unexpected query rewriting by the planner. core/src/main/java/org/opensearch/sql/expression/function/UserDefinedFunctionBuilder.java [117-124] @Override public SqlOperator reverse() { - // Commutative operators (PLUS, TIMES) reverse to themselves. - // Non-commutative operators (MINUS) return null (the default). - if (sqlKind == SqlKind.PLUS || sqlKind == SqlKind.TIMES) { - return this; - } + // Return null to prevent the planner from reordering operands, + // which could change type coercion behavior in mixed-type arithmetic. return null; } Suggestion importance[1-10]: 3 __ Why: The concern about mixed-type arithmetic commutativity is theoretically valid, but in practice PLUS and TIMES are mathematically commutative for numeric types. The suggestion to always return null could prevent valid planner optimizations. This is a debatable design choice rather than a clear bug.	Low
Possible issue	Move catch block to wrap only overflow-throwing code The decimal branch is only reached when both operands are NOT integral, but it checks isDecimal(left) || isDecimal(right). If one operand is integral and the other is decimal (e.g., Integer + BigDecimal), the isIntegral check fails (since one is not integral), the decimal branch is entered, and BigDecimal.valueOf(left.doubleValue()) is used for the integral operand — which is fine. However, if one operand is integral and the other is a Float, neither branch matches and the code falls through to the double path, which is correct. The real issue is that a mixed Integer + BigDecimal case bypasses the overflow check entirely. Consider restructuring the logic to handle mixed integral/decimal cases explicitly. core/src/main/java/org/opensearch/sql/expression/function/udf/math/AddFunction.java [69-82] -if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) { +public static Number add(Number left, Number right) { + if (MathUtils.isIntegral(left) && MathUtils.isIntegral(right)) { + try { + if (left instanceof Long || right instanceof Long) { + return MathUtils.coerceToWidestIntegralType( + left, right, Math.addExact(left.longValue(), right.longValue())); + } + return MathUtils.coerceToWidestIntegralType( + left, right, Math.addExact(left.intValue(), right.intValue())); + } catch (ArithmeticException e) { + throw new NonFallbackCalciteException( + "integer overflow in addition: " + left + " + " + right, e); + } + } + if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) { BigDecimal b0 = left instanceof BigDecimal ? (BigDecimal) left : BigDecimal.valueOf(left.doubleValue()); BigDecimal b1 = right instanceof BigDecimal ? (BigDecimal) right : BigDecimal.valueOf(right.doubleValue()); return b0.add(b1); } double result = left.doubleValue() + right.doubleValue(); return MathUtils.coerceToWidestFloatingType(left, right, result); } Suggestion importance[1-10]: 3 __ Why: The suggestion points out a structural concern about the try-catch placement, but the actual code already has the try-catch wrapping only the integral overflow check (lines 56-68). The improved_code restructures the method but the logic is essentially the same as the existing code. The mixed Integer + BigDecimal case mentioned doesn't bypass overflow since isIntegral would be false for BigDecimal, making this a minor concern at best.	Low

[github-actions]

Persistent review updated to latest commit 59578cf