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 729438c)

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 but does not guard against circular references. If cause == cause.getCause() is false but the chain loops back (e.g., A -> B -> A), the loop runs indefinitely. While the condition cause != cause.getCause() prevents the trivial self-loop, it does not prevent multi-step cycles. 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; Possible Issue The withCheckedArithmetic rewrite runs after convertToCalcitePlan but before pushdown. If pushdown or subsequent optimization rules re-derive types or re-create arithmetic calls, those new calls may revert to unchecked operators (PLUS/MINUS/TIMES) instead of the checked variants. The comment states the rewrite is "applied before pushdown," but the code structure shows it wraps the result of convertToCalcitePlan, which may not guarantee that all downstream transformations preserve the checked operators. /** * Rewrite {@code +}/{@code -}/{@code *} to their overflow-checked variants ({@code CHECKED_PLUS} * / {@code CHECKED_MINUS} / {@code CHECKED_MULTIPLY}) so integer and long arithmetic overflow * throws {@link ArithmeticException} (via {@code Math.addExact} etc.) instead of silently * wrapping. Applied before pushdown so both coordinator-executed and pushed-down (script) * arithmetic are checked. Floating-point arithmetic is unchanged (IEEE 754). * * <p>This does the same rewrite as Calcite's {@code ConvertToChecked} but preserves each call's * originally inferred type (via {@code makeCall(type, op, operands)}) and touches only the three * arithmetic operators, so it does not re-derive the types of unrelated calls (e.g. {@code * CEIL}/{@code DIVIDE}) the way {@code ConvertToChecked} does. */ private static RelNode withCheckedArithmetic(RelNode calcitePlan, CalcitePlanContext context) { RexShuttle checkedShuttle = new RexShuttle() { @Override public RexNode visitCall(RexCall call) { RexNode visited = super.visitCall(call); if (!(visited instanceof RexCall rexCall)) { return visited; } SqlOperator checked = switch (rexCall.getOperator().getKind()) { case PLUS -> SqlStdOperatorTable.CHECKED_PLUS; case MINUS -> SqlStdOperatorTable.CHECKED_MINUS; case TIMES -> SqlStdOperatorTable.CHECKED_MULTIPLY; default -> null; }; // Only integer/long arithmetic can overflow silently and has a checked // implementation (Math.addExact etc.). Float/double/decimal have no checked variant // (SqlFunctions.checkedMultiply(double,double) does not exist) and follow IEEE 754, so // leave them untouched. if (checked == null || !isCheckableIntegerArithmetic(rexCall)) { return visited; } return context.rexBuilder.makeCall(rexCall.getType(), checked, rexCall.getOperands()); } }; return calcitePlan.accept( new RelHomogeneousShuttle() { @Override public RelNode visit(RelNode other) { RelNode visited = super.visitChildren(other); return visited.accept(checkedShuttle); } }); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 729438c

Explore these optional code suggestions:

Category	Suggestion	Impact
General	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 pathological cases. core/src/main/java/org/opensearch/sql/executor/QueryService.java [475-484] 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 issue with circular exception chains. While the condition cause != cause.getCause() provides basic protection, using a Set to track visited exceptions is a more robust approach. However, circular exception chains are rare in practice, making this a moderate improvement rather than a critical fix.	Medium

---

Previous suggestions

Suggestions up to commit 729438c

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 pathological cases. core/src/main/java/org/opensearch/sql/executor/QueryService.java [475-484] 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 a circular cause chain exists. While the condition cause != cause.getCause() provides basic protection, using a Set to track visited exceptions is a more robust approach. However, circular exception chains are rare in practice, making this a moderate improvement rather than a critical fix.	Medium

Suggestions up to commit 1102cb2

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

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

[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

[penghuo]

simply comments. it duplicate with Line 431-434.

[ahkcs]

simplified

[penghuo]

Is there better way to handle expected calcite exception? can it wrapped with NonFallbackcalciteException?

[ahkcs]

Good suggestion. I moved overflow handling to the Calcite execution boundary, where the wrapped ArithmeticException is translated into a top-level NonFallbackCalciteException. The outer catch now uses the existing generic fallback logic. Addressed in 729438c.

[github-actions]

Persistent review updated to latest commit 729438c

[github-actions]

Persistent review updated to latest commit 729438c