Widen narrow integer operands in PPL +/-/* to prevent overflow

[ahkcs]

Description

Integer arithmetic (+, -, *) on narrow integer types silently overflowed because the operators kept Calcite's LEAST_RESTRICTIVE result type (SMALLINT * SMALLINT -> SMALLINT, INTEGER * INTEGER -> INTEGER). For example eval area = ResolutionWidth * ResolutionHeight (both short) computed 28,777,836 but wrapped into the 16-bit range, so where area > 2000000 returned 0 rows. The reported workaround was cast(... as long).

This widens narrow integer operands before the operation so the result cannot overflow the (mis-inferred) narrow type:

operands	result
byte/short	int
any int/long involved	long (bigint)
any float/double/decimal/datetime	unchanged (defers to Calcite)

The widening is a CAST injected on the operands in the SQL-plugin lowering (PPLFuncImpTable), applied to +, -, * and the named add/subtract/multiply functions. The string-concat + and the DATETIME - DATETIME variant are unchanged. Arithmetic inside higher-order-function lambda bodies (reduce/mvmap) is left unwidened — those operands are lambda-parameter placeholders resolved by the HOF's own type inference, and they execute JVM-side where operands already promote to int.

Why widening (and not error-on-overflow)

An alternative is to detect overflow and error (Calcite's CHECKED_PLUS/CHECKED_MINUS/CHECKED_MULTIPLY → Math.addExact, explored in #5604). Widening was chosen because it is the only approach that fixes both execution engines from a single change:

• The CAST is baked into the RelNode before the engine fork. PPL runs on two paths — the Calcite/Enumerable engine and the analytics-engine (DataFusion) backend. Because widening happens in PPLFuncImpTable (shared lowering, upstream of the fork), the widened operand types flow through to both — including through Substrait to DataFusion. Verified live: short * short (30000×30000) returns int 900000000 on the Calcite path and on the DataFusion route (profile confirms *(CAST($n):INTEGER, ...) with backends=[datafusion]); the reported | where area > 2000000 now returns the matching row on both.
• Error-on-overflow cannot be done consistently across engines. The checked operators are a Calcite-codegen mechanism (Math.addExact in generated Java). CHECKED_* has no Substrait representation, so on the analytics-engine route the arithmetic executes natively in DataFusion and the checked rewrite never reaches it — leaving the overflow unfixed there. Making DataFusion error would require separate backend work (a custom Rust checked-arithmetic UDF). So erroring would need two independent implementations (Calcite checked-arith + a DataFusion UDF) and still diverge; widening needs one and is consistent.

Widening fully resolves the reported short/int overflow on both engines. It does not address long * long overflow (no wider primitive type) — that residual is out of scope here and, if needed, is a separate error-on-overflow effort.

Verification

query	before	after (both engines)
eval area = short * short (30000²)	wraps (smallint, negative)	int 900000000
eval area = short * short | where area > 2000000	0 rows	1 row
eval x = int * int (2e9²)	wraps	bigint, correct

test	result
CalcitePPLBuiltinFunctionIT widening ITs (new)	pass
CalcitePPLEvalTest/ArrayFunctionTest/DedupTest/etc. + UnifiedQueryPlannerSqlV2Test (plan snapshots)	pass (updated for widening CAST)
CalciteExplainIT / CalciteNoPushdownIT (explain fixtures)	pass (regenerated)
collection.md doctest (reduce/mvmap unaffected)	pass

Related Issues

Resolves the "eval integer arithmetic overflow (no type widening)" bug. See #5164 / #5604 for the alternative error-on-overflow approach (Calcite-path only).

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 f272d30)

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 coerce method does not handle null values explicitly. When value is null and elementType is non-null, the method returns null directly (line 62), but if value is null and passes the first check, the cast to Number at line 64 will throw ClassCastException. This occurs when elementType is non-null but value is null and not caught by the !(value instanceof Number) check, which is impossible since null instanceof Number is false. However, the logic is correct as written; the concern is moot. 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()); default -> value; }; } Possible Issue The coerceNumericComparisonOperands method only coerces when one side contains a subquery, but the decorrelation issue it aims to fix (mismatched join key types) can also occur when both sides are scalar expressions of different integer widths without any subquery involved. If a comparison like int_col = bigint_col is decorrelated into a join (e.g., via a correlated EXISTS), the join keys will have mismatched types even without a subquery on either side. The fix may be incomplete for all decorrelated join scenarios. private static RexNode[] coerceNumericComparisonOperands( RexBuilder builder, BuiltinFunctionName functionName, RexNode... args) { if (args.length != 2 || !BuiltinFunctionName.COMPARATORS.contains(functionName)) { return args; } if (!containsSubQuery(args[0]) && !containsSubQuery(args[1])) { return args; } RelDataType leftType = args[0].getType(); RelDataType rightType = args[1].getType(); if (!SqlTypeName.INT_TYPES.contains(leftType.getSqlTypeName()) || !SqlTypeName.INT_TYPES.contains(rightType.getSqlTypeName()) || leftType.getSqlTypeName() == rightType.getSqlTypeName()) { return args; } RelDataType commonType = builder.getTypeFactory().leastRestrictive(List.of(leftType, rightType)); if (commonType == null) { return args; } return new RexNode[] { builder.makeCast( TYPE_FACTORY.createTypeWithNullability(commonType, leftType.isNullable()), args[0]), builder.makeCast( TYPE_FACTORY.createTypeWithNullability(commonType, rightType.isNullable()), args[1]) }; }

[RyanL1997]

Hi @ahkcs , thanks for the change, and here are somer reviews.

[RyanL1997]

This snapshot is the one I couldn't reconcile: the inner +(2:BIGINT, 3:BIGINT) widens both literals, but the outer operand stays plain 1 (INTEGER), whereas selectExpressionWithoutFrom's 1 + 1 widens both to 1:BIGINT. If the outer + in the sum reduction goes through the same widening operator, I'd expect +(1:BIGINT, +(2:BIGINT, 3:BIGINT)). Can you confirm whether the outer add is intentionally not widened (and that a single + mixing INTEGER + BIGINT operands is expected)?

[ahkcs]

It is widened — the bare 1 is a rendering artifact. Dumping the tree shows outer.op0 = 1:BIGINT (type=BIGINT); every operand is BIGINT, no mixed add. Calcite's RexCall digest drops a literal's :BIGINT suffix when its sibling has the same type (here +(2,3) is BIGINT), so 1:BIGINT prints as 1.

[github-actions]

Persistent review updated to latest commit 6d53547

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f272d30

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect method reference class The method referencesLambdaParameter is defined as a static method in the outer class but references AbstractBuilder::referencesLambdaParameter in the recursive call. This will cause a compilation error because the method is not in AbstractBuilder. Change the method reference to PPLFuncImpTable::referencesLambdaParameter to fix the incorrect class reference. 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 RexCall call) { - return call.getOperands().stream().anyMatch(AbstractBuilder::referencesLambdaParameter); + return call.getOperands().stream().anyMatch(PPLFuncImpTable::referencesLambdaParameter); } return false; } Suggestion importance[1-10]: 10 __ Why: The method reference AbstractBuilder::referencesLambdaParameter is incorrect because referencesLambdaParameter is a static method in the outer class PPLFuncImpTable, not in AbstractBuilder. This will cause a compilation error. The correct reference should be PPLFuncImpTable::referencesLambdaParameter.	High

---

Previous suggestions

Suggestions up to commit 32d27cd

Category	Suggestion	Impact
General	Preserve precision in DECIMAL conversion The DECIMAL case converts via doubleValue() which can lose precision for large integers or exact decimal values. Consider using BigDecimal.valueOf(num.longValue()) for integer types or a more precise conversion path to preserve numeric accuracy. 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 Integer || num instanceof Long) ? + BigDecimal.valueOf(num.longValue()) : BigDecimal.valueOf(num.doubleValue()); default -> value; }; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that converting to DECIMAL via doubleValue() can lose precision for large integers. The improved code checks if the number is an integer type before conversion, which would preserve precision. However, the impact is moderate since this affects edge cases with large integer values in array operations.	Low

Suggestions up to commit 6ba846b

Category	Suggestion	Impact
Possible issue	Handle RexLambda nodes in subquery detection The containsSubQuery method does not handle RexLambda nodes, which can also contain subqueries in their operands. Since the PR introduces RexLambdaRef imports and the codebase uses lambda expressions, failing to traverse into RexLambda nodes could miss nested subqueries, leading to incorrect type coercion decisions. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [662-670] private static boolean containsSubQuery(RexNode node) { if (node instanceof RexSubQuery) { return true; } if (node instanceof RexCall call) { return call.getOperands().stream().anyMatch(PPLFuncImpTable::containsSubQuery); } + if (node instanceof RexLambda lambda) { + return containsSubQuery(lambda.getExpression()); + } return false; } Suggestion importance[1-10]: 7 __ Why: The containsSubQuery method should traverse RexLambda nodes to detect nested subqueries within lambda expressions. Missing this could lead to incorrect type coercion decisions when subqueries appear in lambda bodies.	Medium
	Traverse RexLambda nodes for parameter references The referencesLambdaParameter method does not traverse into RexLambda nodes to check their body expressions. If a lambda contains nested lambdas with parameter references, this method will miss them, potentially allowing incorrect widening casts inside lambda bodies that should be skipped. 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 RexCall call) { return call.getOperands().stream().anyMatch(AbstractBuilder::referencesLambdaParameter); } + if (node instanceof RexLambda lambda) { + return referencesLambdaParameter(lambda.getExpression()); + } return false; } Suggestion importance[1-10]: 7 __ Why: The referencesLambdaParameter method should check nested RexLambda nodes to avoid incorrectly widening arithmetic inside lambda bodies that reference parameters. This prevents breaking lambda parameter resolution.	Medium
General	Add explicit null check before coercion The coerce method does not handle null values explicitly before casting to Number. If value is null, the instanceof Number check passes it through, but the method signature suggests it should handle nulls safely. Add an explicit null check to prevent potential NullPointerException in downstream code that assumes non-null 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)) { + if (value == null || 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()); default -> value; }; } Suggestion importance[1-10]: 3 __ Why: The explicit null check before instanceof Number is redundant since the existing condition already handles null values correctly by returning them unchanged. The suggestion adds minimal value.	Low

Suggestions up to commit a2e7706

Category	Suggestion	Impact
General	Verify scalar subquery before widening The method only checks if either operand contains a subquery but doesn't verify that the comparison actually involves a scalar subquery. Non-scalar subqueries (e.g., IN with a multi-row result) should not trigger this widening logic, as they have different semantics. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [633-640] private static RexNode[] coerceNumericComparisonOperands( RexBuilder builder, BuiltinFunctionName functionName, RexNode... args) { if (args.length != 2 || !BuiltinFunctionName.COMPARATORS.contains(functionName)) { return args; } - if (!containsSubQuery(args[0]) && !containsSubQuery(args[1])) { + boolean hasScalarSubquery = (args[0] instanceof RexSubQuery && ((RexSubQuery) args[0]).getKind() == SqlKind.SCALAR_QUERY) + || (args[1] instanceof RexSubQuery && ((RexSubQuery) args[1]).getKind() == SqlKind.SCALAR_QUERY); + if (!hasScalarSubquery) { return args; } ... } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a valid concern that the method doesn't distinguish between scalar and non-scalar subqueries. The improved code adds a check for SqlKind.SCALAR_QUERY, which would make the logic more precise. However, the existing containsSubQuery helper may already handle this correctly through Calcite's decorrelation logic, so the impact is moderate.	Medium
	Preserve precision in DECIMAL coercion The DECIMAL case uses doubleValue() which can lose precision for large integers. For numeric types that aren't already BigDecimal, consider using BigDecimal.valueOf(num.longValue()) for integer types or checking the actual runtime type to preserve precision. 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 Float || num instanceof Double) ? + BigDecimal.valueOf(num.doubleValue()) : + BigDecimal.valueOf(num.longValue()); default -> value; }; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential precision loss when coercing to DECIMAL using doubleValue() for integer types. However, the impact is moderate since this affects edge cases with large integers, and the improved code adds complexity. The suggestion is valid but not critical.	Low
	Handle nested lambda expressions The method recursively checks for lambda parameters but doesn't handle RexLambda nodes themselves. If a lambda expression is nested within the arithmetic operation, the check should also inspect the lambda body to avoid missing lambda parameter references. 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]: 5 __ Why: The suggestion points out that RexLambda nodes are not explicitly handled in the recursive check. While this is a valid observation, the existing code already handles RexCall which would include lambda expressions as operands. The improvement is minor and may not be necessary given the current implementation context.	Low

Suggestions up to commit 82ed0c1

Category	Suggestion	Impact
General	Preserve integer precision in DECIMAL coercion The DECIMAL case uses doubleValue() which can lose precision for large integers. When coercing integer types to DECIMAL, use BigDecimal.valueOf(num.longValue()) to preserve exact integer values without floating-point rounding errors. 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 Byte || num instanceof Short || num instanceof Integer || num instanceof Long) + ? BigDecimal.valueOf(num.longValue()) + : BigDecimal.valueOf(num.doubleValue()); default -> value; }; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a precision loss issue when coercing large integers to DECIMAL via doubleValue(). Using longValue() for integer types preserves exact values. However, the improved code could be simplified and the impact is moderate since DECIMAL coercion is a less common path.	Medium
	Document subquery-specific coercion rationale The method only coerces when a subquery is present, but the join key type mismatch can occur in other scenarios. Consider broadening the condition or documenting why this specific case requires special handling while others don't. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [633-640] private static RexNode[] coerceNumericComparisonOperands( RexBuilder builder, BuiltinFunctionName functionName, RexNode... args) { if (args.length != 2 || !BuiltinFunctionName.COMPARATORS.contains(functionName)) { return args; } + // Only coerce for subquery comparisons where decorrelation creates join keys if (!containsSubQuery(args[0]) && !containsSubQuery(args[1])) { return args; } ... } Suggestion importance[1-10]: 3 __ Why: The suggestion asks for additional documentation, but the existing comment at lines 617-632 already explains why subquery comparisons require special handling (decorrelation creates join keys with mismatched types). The improved code only adds a redundant comment without substantive improvement.	Low

Suggestions up to commit 6d53547

Category	Suggestion	Impact
General	Skip redundant casts for matching types The cast operation may fail or produce unexpected results if the source type is already wider than the target. Add a check to skip casting when args[i] already has the promoted type to avoid redundant operations and potential issues. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [761-772] private static RexNode[] widenIntegerOperands(RexBuilder builder, RexNode... args) { SqlTypeName promoted = promotedIntegerType(args); if (promoted == null) { return args; } RexNode[] widened = new RexNode[args.length]; for (int i = 0; i < args.length; i++) { - RelDataType target = TYPE_FACTORY.createSqlType(promoted, args[i].getType().isNullable()); - widened[i] = builder.makeCast(target, args[i]); + if (args[i].getType().getSqlTypeName() == promoted) { + widened[i] = args[i]; + } else { + RelDataType target = TYPE_FACTORY.createSqlType(promoted, args[i].getType().isNullable()); + widened[i] = builder.makeCast(target, args[i]); + } } return widened; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that redundant casts could be avoided when the operand already has the promoted type. However, this is a minor optimization that doesn't fix a bug or significantly improve performance. The current implementation is correct and the suggested optimization would only marginally improve efficiency in specific cases.	Low

[github-actions]

Persistent review updated to latest commit 2ea4a2d

[github-actions]

Persistent review updated to latest commit 82ed0c1

[github-actions]

Persistent review updated to latest commit a2e7706

[github-actions]

Persistent review updated to latest commit 6ba846b

[github-actions]

Persistent review updated to latest commit 32d27cd

[github-actions]

Persistent review updated to latest commit f272d30