Detect long (BIGINT) arithmetic overflow instead of silently wrapping (#5164)

[ahkcs]

Description

long (BIGINT) arithmetic (+, -, *) in PPL/SQL eval/SELECT expressions silently wrapped on overflow in the Calcite engine — e.g. long_field * 9999999999 at the top of the 64-bit range returned a negative value with HTTP 200. SqlStdOperatorTable.PLUS/MINUS/MULTIPLY generate plain Java +/-/* in the Enumerable code path, which wrap.

This rewrites long +/-/* to their overflow-checked variants (CHECKED_PLUS / CHECKED_MINUS / CHECKED_MULTIPLY), which generate Math.addExact / subtractExact / multiplyExact and throw ArithmeticException on overflow. The exception is caught in QueryService and surfaced as a 4xx client error (NonFallbackCalciteException) instead of wrapping or falling back to the V2 engine. (The V2 engine already uses Math.*Exact.)

Scope: BIGINT-only

The checked rewrite is gated to long (BIGINT) operands. Narrower integer arithmetic (byte/short/int) is widened to a type that cannot overflow before this rewrite runs — PPLFuncImpTable promotes byte/short → int and any int/long operand → long for +/-/* (#5603) — so long, which has no wider integer type to widen into, is the only remaining overflow case on the Calcite engine. float/double/decimal follow IEEE 754 / decimal semantics and have no CHECKED_* runtime, so they are left untouched.

The rewrite runs after the analytics-engine fork in convertToCalcitePlan, so only the Calcite/Enumerable path is affected. The DataFusion analytics backend handles its own long overflow (checked arrow kernels, opensearch-project/OpenSearch#22378) — the two engines now converge on erroring for long overflow.

It runs before pushdown so both coordinator-executed and pushed-down (script) arithmetic are checked. PPLAggregateConvertRule, OpenSearchRelOptUtil, and ExtendedRelJson recognize the CHECKED_* kinds so aggregate-pushdown, sort-expression pushdown, and script serialization keep working for pushed-down checked arithmetic.

Stacked on #5603

This PR is stacked on #5603 (operand widening) and depends on it: without the widening underneath, short/int overflow would slip through un-checked on the Calcite path. The first commit here is #5603's widening; it drops out of this diff once #5603 merges. Together the two PRs close integer-arithmetic overflow on the Calcite engine (#5603 widens the narrow tiers; this errors the long tier), matching the analytics-engine behavior.

Testing

test	behavior
issues/5164.yml (REST)	int/short overflow → widened correct value (200); long overflow (+/-/*) → 4xx; normal arithmetic unaffected
CalciteExplainIT (pushdown)	258/258 pass — explain fixtures regenerated to show CHECKED_* in pushed scripts
CalciteNoPushdownIT (CalciteExplainIT)	258/258 pass — pushdown-only arithmetic tests skipped as expected

Related Issues

Resolves #5164. Stacked on #5603. Analytics-engine counterpart: opensearch-project/OpenSearch#22378.

Check List

• New functionality includes testing.
• New functionality has been documented.
• Commits are signed per the DCO using --signoff or -s.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1102cb2)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The findArithmeticOverflow method walks the cause chain with cause != cause.getCause() to detect cycles. However, if a cause chain contains a self-referencing exception (where cause.getCause() == cause), this condition will be true and the loop will continue indefinitely, causing an infinite loop. While self-referencing exceptions are rare, they can occur in practice (e.g., via reflection or custom exception handling). A safer approach is to track visited exceptions in a set or limit the depth of traversal. private static ArithmeticException findArithmeticOverflow(@Nullable Throwable t) { for (Throwable cause = t; cause != null && cause != cause.getCause(); cause = cause.getCause()) { if (cause instanceof ArithmeticException arithmeticException) { return arithmeticException; } } return null; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1102cb2

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent infinite loop in exception chain The loop condition cause != cause.getCause() may not prevent infinite loops if a circular cause chain exists. Consider using a Set to track visited exceptions or limit the maximum depth to prevent potential infinite loops in malformed exception chains. core/src/main/java/org/opensearch/sql/executor/QueryService.java [477-486] private static ArithmeticException findArithmeticOverflow(@Nullable Throwable t) { - for (Throwable cause = t; - cause != null && cause != cause.getCause(); - cause = cause.getCause()) { + Set<Throwable> visited = new HashSet<>(); + for (Throwable cause = t; cause != null; cause = cause.getCause()) { + if (!visited.add(cause)) { + break; // Circular reference detected + } if (cause instanceof ArithmeticException arithmeticException) { return arithmeticException; } } return null; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential infinite loop risk if a circular cause chain exists. While the condition cause != cause.getCause() provides some protection, using a Set to track visited exceptions is a more robust approach. However, circular exception chains are rare in practice, so this is a moderate improvement rather than a critical fix.	Medium
General	Validate operator kind before type check The method checks if all operands are BIGINT but doesn't verify the operator is actually an arithmetic operation. If called with non-arithmetic operators, it may incorrectly return true for BIGINT operands with non-arithmetic operations. core/src/main/java/org/opensearch/sql/executor/QueryService.java [461-466] private static boolean isCheckableIntegerArithmetic(RexCall call) { if (!isCheckableLongType(call.getType())) { + return false; + } + SqlKind kind = call.getOperator().getKind(); + if (kind != SqlKind.PLUS && kind != SqlKind.MINUS && kind != SqlKind.TIMES) { return false; } return call.getOperands().stream().allMatch(op -> isCheckableLongType(op.getType())); } Suggestion importance[1-10]: 3 __ Why: The suggestion is technically correct but unnecessary in context. The method isCheckableIntegerArithmetic is only called from withCheckedArithmetic after already filtering for PLUS, MINUS, and TIMES operators in the switch statement (lines 425-429). Adding redundant validation would decrease code clarity without providing meaningful protection.	Low

---

Previous suggestions

Suggestions up to commit 8a628ff

Category	Suggestion	Impact
Possible issue	Prevent infinite loop in exception chain The loop condition cause != cause.getCause() does not prevent infinite loops when a throwable's cause is itself. Use a Set to track visited causes or check cause.getCause() != null separately to avoid potential infinite loops in malformed exception chains. core/src/main/java/org/opensearch/sql/executor/QueryService.java [477-486] private static ArithmeticException findArithmeticOverflow(@Nullable Throwable t) { - for (Throwable cause = t; - cause != null && cause != cause.getCause(); - cause = cause.getCause()) { + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<>()); + for (Throwable cause = t; cause != null && visited.add(cause); cause = cause.getCause()) { if (cause instanceof ArithmeticException arithmeticException) { return arithmeticException; } } return null; } Suggestion importance[1-10]: 8 __ Why: The loop condition cause != cause.getCause() does not prevent infinite loops if a throwable's cause is itself (a malformed exception chain). Using a Set to track visited causes prevents potential infinite loops and runtime hangs.	Medium
	Handle nested lambda expressions The recursive lambda body check should handle RexLambda nodes to avoid missing nested lambda parameters. A RexLambda wraps the lambda body expression, so the method should recurse into its body to detect any RexLambdaRef references within nested lambda scopes. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [847-855] private static boolean referencesLambdaParameter(RexNode node) { if (node instanceof RexLambdaRef) { return true; + } + if (node instanceof RexLambda lambda) { + return referencesLambdaParameter(lambda.getExpression()); } if (node instanceof RexCall call) { return call.getOperands().stream().anyMatch(AbstractBuilder::referencesLambdaParameter); } return false; } Suggestion importance[1-10]: 7 __ Why: The method referencesLambdaParameter should handle RexLambda nodes to detect lambda parameters in nested lambda bodies. Without this, the check may miss RexLambdaRef references wrapped inside a RexLambda, potentially allowing incorrect widening in lambda contexts.	Medium
General	Add null safety for type access The method does not handle nullable types explicitly. If arg.getType() or arg.getType().getSqlTypeName() returns null, a NullPointerException will occur. Add a null check before accessing getSqlTypeName() to prevent runtime errors. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [862-874] private static SqlTypeName promotedIntegerType(RexNode... args) { boolean needsLong = false; for (RexNode arg : args) { + if (arg.getType() == null || arg.getType().getSqlTypeName() == null) { + return null; + } switch (arg.getType().getSqlTypeName()) { case TINYINT, SMALLINT -> {} case INTEGER, BIGINT -> needsLong = true; default -> { return null; } } } return needsLong ? SqlTypeName.BIGINT : SqlTypeName.INTEGER; } Suggestion importance[1-10]: 6 __ Why: The method promotedIntegerType does not check if arg.getType() or getSqlTypeName() returns null, which could cause a NullPointerException. Adding a null check improves robustness, though the likelihood of encountering null types in this context may be low.	Low
	Preserve precision in decimal coercion The DECIMAL coercion uses BigDecimal.valueOf(num.doubleValue()), which can lose precision for large long or BigInteger values. Consider using BigDecimal.valueOf(num.longValue()) for integral types or new BigDecimal(num.toString()) to preserve exact numeric values. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendCore.java [60-75] private static Object coerce(Object value, SqlTypeName elementType) { if (elementType == null || !(value instanceof Number)) { return value; } Number num = (Number) value; return switch (elementType) { case TINYINT -> num.byteValue(); case SMALLINT -> num.shortValue(); case INTEGER -> num.intValue(); case BIGINT -> num.longValue(); case FLOAT, REAL -> num.floatValue(); case DOUBLE -> num.doubleValue(); - case DECIMAL -> num instanceof BigDecimal ? num : BigDecimal.valueOf(num.doubleValue()); + case DECIMAL -> num instanceof BigDecimal ? num : + (num instanceof Double || num instanceof Float) ? + BigDecimal.valueOf(num.doubleValue()) : + BigDecimal.valueOf(num.longValue()); default -> value; }; } Suggestion importance[1-10]: 5 __ Why: The DECIMAL coercion uses BigDecimal.valueOf(num.doubleValue()), which can lose precision for large integral values. Using BigDecimal.valueOf(num.longValue()) for integral types preserves exact values, though the impact depends on the actual data being processed.	Low

Suggestions up to commit da07e56

Category	Suggestion	Impact
General	Handle RexLambda nodes in recursion The recursive lambda check should handle RexLambda nodes that contain lambda bodies. Currently, only RexLambdaRef (parameter references) are detected, but a RexLambda wrapping the body should also trigger the skip logic to avoid breaking lambda resolution. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [847-855] private static boolean referencesLambdaParameter(RexNode node) { - if (node instanceof RexLambdaRef) { + if (node instanceof RexLambdaRef || node instanceof RexLambda) { return true; } if (node instanceof RexCall call) { return call.getOperands().stream().anyMatch(AbstractBuilder::referencesLambdaParameter); } return false; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that RexLambda nodes should also trigger the skip logic to avoid breaking lambda resolution. However, the existing code already handles lambda bodies through RexLambdaRef detection in operands, so this is a defensive improvement rather than a critical fix.	Medium
	Preserve precision for integer-to-decimal conversion Converting non-BigDecimal numbers to BigDecimal via doubleValue() loses precision for large long values. Use BigDecimal.valueOf(num.longValue()) for integer types to preserve exact values, or handle Long explicitly before falling back to doubleValue(). core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendCore.java [60-75] private static Object coerce(Object value, SqlTypeName elementType) { if (elementType == null || !(value instanceof Number)) { return value; } Number num = (Number) value; return switch (elementType) { case TINYINT -> num.byteValue(); - ... - case DECIMAL -> num instanceof BigDecimal ? num : BigDecimal.valueOf(num.doubleValue()); + case SMALLINT -> num.shortValue(); + case INTEGER -> num.intValue(); + case BIGINT -> num.longValue(); + case FLOAT, REAL -> num.floatValue(); + case DOUBLE -> num.doubleValue(); + case DECIMAL -> num instanceof BigDecimal ? num : + (num instanceof Long || num instanceof Integer || num instanceof Short || num instanceof Byte) + ? BigDecimal.valueOf(num.longValue()) + : BigDecimal.valueOf(num.doubleValue()); default -> value; }; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a precision loss issue when converting large long values to BigDecimal via doubleValue(). Using longValue() for integer types would preserve exact values. However, the impact is limited to DECIMAL array elements, which is a less common case.	Low
	Improve overflow error message clarity The error message concatenates overflow.getMessage(), which may be null or generic. Provide a more descriptive message that includes the operation context (e.g., "Long arithmetic overflow detected") to help users identify the source of the overflow. core/src/main/java/org/opensearch/sql/executor/QueryService.java [198-206] ArithmeticException overflow = findArithmeticOverflow(t); if (overflow != null) { + String message = overflow.getMessage() != null + ? "Arithmetic overflow: " + overflow.getMessage() + : "Arithmetic overflow detected in long (BIGINT) operation"; propagateCalciteError( - new NonFallbackCalciteException( - "Arithmetic overflow: " + overflow.getMessage(), overflow), + new NonFallbackCalciteException(message, overflow), listener); } Suggestion importance[1-10]: 5 __ Why: The suggestion improves error message clarity by handling null messages and providing more context. This is a minor usability improvement that helps users identify overflow sources, but the existing message is already functional.	Low
	Clarify operand type assumptions The method assumes all operands must be BIGINT for checked arithmetic, but the widening logic in PPLFuncImpTable casts narrower integers (INT/SMALLINT) to BIGINT. Verify that operands are already widened before this check runs, or adjust the logic to handle mixed-width operands that will be cast. core/src/main/java/org/opensearch/sql/executor/QueryService.java [461-466] private static boolean isCheckableIntegerArithmetic(RexCall call) { if (!isCheckableLongType(call.getType())) { return false; } + // Verify all operands are BIGINT after widening; narrower types should already be cast. return call.getOperands().stream().allMatch(op -> isCheckableLongType(op.getType())); } Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify operand widening assumptions but only adds a comment without changing logic. The existing implementation is correct since PPLFuncImpTable widens operands before this check runs, as documented in the method's javadoc.	Low

Suggestions up to commit c35a804

Category	Suggestion	Impact
Security	Prevent stack overflow in recursion The recursive lambda parameter check may cause a stack overflow for deeply nested expressions. Consider adding a depth limit or converting to an iterative approach with an explicit stack to prevent potential stack exhaustion on maliciously crafted or extremely complex queries. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [847-855] private static boolean referencesLambdaParameter(RexNode node) { + return referencesLambdaParameterWithDepth(node, 0, 100); +} + +private static boolean referencesLambdaParameterWithDepth(RexNode node, int depth, int maxDepth) { + if (depth > maxDepth) { + return false; + } if (node instanceof RexLambdaRef) { return true; } if (node instanceof RexCall call) { - return call.getOperands().stream().anyMatch(AbstractBuilder::referencesLambdaParameter); + return call.getOperands().stream().anyMatch(op -> referencesLambdaParameterWithDepth(op, depth + 1, maxDepth)); } return false; } Suggestion importance[1-10]: 5 __ Why: The suggestion addresses a potential stack overflow risk in deeply nested expressions. However, the risk is relatively low in typical query scenarios, and the proposed depth limit (100) is somewhat arbitrary. The improvement is valid but represents a defensive measure rather than fixing a critical bug.	Low
Possible issue	Validate numeric range before coercion The numeric coercion may lose precision or overflow when converting between types (e.g., BIGINT to INTEGER, DOUBLE to FLOAT). Verify that the source value fits within the target type's range before conversion to prevent silent data loss or unexpected behavior. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendCore.java [60-75] private static Object coerce(Object value, SqlTypeName elementType) { if (elementType == null || !(value instanceof Number)) { return value; } Number num = (Number) value; return switch (elementType) { - case TINYINT -> num.byteValue(); - case SMALLINT -> num.shortValue(); - case INTEGER -> num.intValue(); + case TINYINT -> { + byte b = num.byteValue(); + if (num.longValue() != b) throw new ArithmeticException("Value out of TINYINT range"); + yield b; + } + case SMALLINT -> { + short s = num.shortValue(); + if (num.longValue() != s) throw new ArithmeticException("Value out of SMALLINT range"); + yield s; + } + case INTEGER -> { + int i = num.intValue(); + if (num.longValue() != i) throw new ArithmeticException("Value out of INTEGER range"); + yield i; + } case BIGINT -> num.longValue(); case FLOAT, REAL -> num.floatValue(); case DOUBLE -> num.doubleValue(); case DECIMAL -> num instanceof BigDecimal ? num : BigDecimal.valueOf(num.doubleValue()); default -> value; }; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about precision loss during numeric coercion. However, the proposed solution of throwing exceptions on range violations may be too strict for the intended use case, which is aligning heterogeneously boxed values. The current implementation follows standard Java narrowing conversion semantics, which is likely intentional for this context.	Low

Suggestions up to commit 447b958

Category	Suggestion	Impact
General	Prevent infinite loop in exception chain The loop condition cause != cause.getCause() may not detect all circular references if the cause chain forms a cycle at a deeper level. Add a visited set or depth limit to prevent infinite loops in malformed exception chains. core/src/main/java/org/opensearch/sql/executor/QueryService.java [477-486] private static ArithmeticException findArithmeticOverflow(@Nullable Throwable t) { - for (Throwable cause = t; - cause != null && cause != cause.getCause(); - cause = cause.getCause()) { + Set<Throwable> visited = new HashSet<>(); + for (Throwable cause = t; cause != null; cause = cause.getCause()) { + if (!visited.add(cause)) { + break; + } if (cause instanceof ArithmeticException arithmeticException) { return arithmeticException; } } return null; } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a valid edge case where a circular exception chain could cause an infinite loop. The existing cause != cause.getCause() check only catches immediate self-references, not deeper cycles. Using a visited set is a robust solution to prevent this issue.	Low
	Validate numeric coercion bounds Narrowing conversions (e.g., longValue() to byteValue()) may silently truncate values outside the target type's range. Validate that the numeric value fits within the target type's bounds before coercing to prevent data loss. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendCore.java [60-75] private static Object coerce(Object value, SqlTypeName elementType) { if (elementType == null || !(value instanceof Number)) { return value; } Number num = (Number) value; return switch (elementType) { - case TINYINT -> num.byteValue(); - case SMALLINT -> num.shortValue(); - case INTEGER -> num.intValue(); - case BIGINT -> num.longValue(); + case TINYINT -> { + long val = num.longValue(); + if (val < Byte.MIN_VALUE || val > Byte.MAX_VALUE) { + throw new ArithmeticException("Value out of range for TINYINT: " + val); + } + yield num.byteValue(); + } + case SMALLINT -> { + long val = num.longValue(); + if (val < Short.MIN_VALUE || val > Short.MAX_VALUE) { + throw new ArithmeticException("Value out of range for SMALLINT: " + val); + } + yield num.shortValue(); + } ... }; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a potential data loss issue during narrowing conversions. However, the coerce method is designed to align boxed numeric types for array elements after widening has already occurred upstream (in PPLFuncImpTable), so out-of-range values should not reach this point. Adding bounds checks would be defensive but may be unnecessary given the context.	Low
Possible issue	Prevent stack overflow in recursion The recursive lambda parameter check may cause a StackOverflowError on deeply nested expressions. Consider adding a depth limit or using an iterative approach with an explicit stack to prevent unbounded recursion. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [847-855] private static boolean referencesLambdaParameter(RexNode node) { + return referencesLambdaParameterWithDepth(node, 0, 100); +} + +private static boolean referencesLambdaParameterWithDepth(RexNode node, int depth, int maxDepth) { + if (depth > maxDepth) { + return false; + } if (node instanceof RexLambdaRef) { return true; } if (node instanceof RexCall call) { - return call.getOperands().stream().anyMatch(AbstractBuilder::referencesLambdaParameter); + return call.getOperands().stream().anyMatch(op -> referencesLambdaParameterWithDepth(op, depth + 1, maxDepth)); } return false; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a potential stack overflow risk in the recursive referencesLambdaParameter method. However, the risk is low in practice since SQL expression trees are typically shallow. The proposed depth limit is a reasonable safeguard, though the hardcoded 100 could be configurable.	Low

Suggestions up to commit 2d6e9a1

Category	Suggestion	Impact
Possible issue	Prevent infinite loop in cause chain The loop condition cause != cause.getCause() is insufficient to prevent infinite loops if the cause chain contains a cycle where a cause references itself indirectly. Add a visited set or depth limit to guard against circular cause chains that could lead to infinite loops. core/src/main/java/org/opensearch/sql/executor/QueryService.java [451-460] private static ArithmeticException findArithmeticOverflow(@Nullable Throwable t) { - for (Throwable cause = t; - cause != null && cause != cause.getCause(); - cause = cause.getCause()) { + Set<Throwable> visited = new HashSet<>(); + for (Throwable cause = t; cause != null; cause = cause.getCause()) { + if (!visited.add(cause)) { + break; + } if (cause instanceof ArithmeticException arithmeticException) { return arithmeticException; } } return null; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential infinite loop risk if the cause chain contains indirect cycles. While the condition cause != cause.getCause() prevents direct self-reference, it doesn't guard against indirect cycles (A→B→A). Adding a visited set is a valid defensive improvement, though such cycles are rare in practice.	Medium

[github-actions]

Persistent review updated to latest commit 2d2ef9f

[github-actions]

Persistent review updated to latest commit 2d6e9a1

[github-actions]

Persistent review updated to latest commit fc2e0f2

[github-actions]

Persistent review updated to latest commit 447b958

[github-actions]

Persistent review updated to latest commit c35a804

[github-actions]

Persistent review updated to latest commit da07e56

[github-actions]

Persistent review updated to latest commit 8a628ff

[github-actions]

Persistent review updated to latest commit 1102cb2