Skip to content

fix: [bug] narrow BIGINT to INTEGER for int-domain function arguments (#5660) - #5728

Open
AjimelecGonzalez wants to merge 1 commit into
opensearch-project:mainfrom
AjimelecGonzalez:fix/mvindex
Open

fix: [bug] narrow BIGINT to INTEGER for int-domain function arguments (#5660)#5728
AjimelecGonzalez wants to merge 1 commit into
opensearch-project:mainfrom
AjimelecGonzalez:fix/mvindex

Conversation

@AjimelecGonzalez

@AjimelecGonzalez AjimelecGonzalez commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Description

PPL queries that pass integer arithmetic as an argument to functions requiring Java int parameters fail when Calcite is enabled:

    mvindex(arr, 1 + 1)      -> CompileException: arrayItemOptional(List, long, ...)
	left('abcdef', 1 + 1)    -> Unable to implement: SqlFunctions.left(String, long)
	round(123.456, 1 + 0)    -> SqlFunctions.sround(BigDecimal, long)

Root cause: PPL widens INTEGER arithmetic to BIGINT for overflow safety (#5603), so expressions like 1 + 1 produce BIGINT. Many Calcite runtime methods (ITEM, LEFT, RIGHT, ROUND, TRUNCATE, SUBSTRING, CONV, SHA2, etc.) take Java int parameters. Since SqlTypeFamily.INTEGER contains BIGINT, the call passes type checking but fails at code generation because the JVM cannot auto-narrow long to int.

The bug surfaces on:
- Local execution: Calcite EnumerableCalc codegen -> Unable to implement

Fix:
- PPLFuncImpTable.resolve: for functions whose implementations require int, narrow BIGINT arguments back to INTEGER at the plan layer.

Overflow safety is preserved: the arithmetic itself still computes in BIGINT; only the final value handed to an int-domain parameter is narrowed. Arithmetic operators, comparisons, cast(x as long), aggregations, and long-field arithmetic are left untouched.

Also fixes the pre-existing case where an explicit cast(x as long) is passed to these functions.

Testing:

  • Unit tests in RelJsonSerializerTest covering the serialization-layer type preservation.
  • Integration tests in CalciteArrayFunctionIT, CalciteTextFunctionIT, and CalciteMathematicalFunctionIT covering mvindex, left, right, substring, round, truncate, conv, and sha2 with
    arithmetic and cast(x as long) arguments.
  • Regression tests confirming arithmetic with doc-value fields, aggregations, cast(x as long), and long-field arithmetic still return the correct BIGINT types.

Related Issues

Resolves #5660

Related to #5603 (introduced the integer arithmetic widening that exposed this)

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

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 0d3b11a)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 0d3b11a
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add signature parameter bounds validation

Add bounds checking before accessing signature parameter types to prevent potential
IndexOutOfBoundsException. The method narrows arguments based on positions in
INT_PARAM_POSITIONS but doesn't verify these positions are valid for the matched
signature's parameter list.

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [765-775]

 for (int pos : intPositions) {
-  if (pos < args.length && args[pos].getType().getSqlTypeName() == SqlTypeName.BIGINT) {
+  if (pos < args.length && pos < signature.getParamTypes().size() 
+      && args[pos].getType().getSqlTypeName() == SqlTypeName.BIGINT) {
     if (narrowed == null) {
       narrowed = args.clone();
     }
     RelDataType intType =
         TYPE_FACTORY.createTypeWithNullability(
             TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER), args[pos].getType().isNullable());
     narrowed[pos] = builder.makeCast(intType, args[pos]);
   }
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential IndexOutOfBoundsException risk when accessing signature.getParamTypes(). Adding bounds checking (pos < signature.getParamTypes().size()) would prevent runtime errors if INT_PARAM_POSITIONS contains positions beyond the signature's parameter list.

Medium
General
Verify signature expects INTEGER type

Validate that the signature's parameter at pos actually expects INTEGER type before
narrowing. The current logic assumes all positions in INT_PARAM_POSITIONS always
need narrowing, but doesn't verify the signature's expected type matches this
assumption.

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [754-766]

 private static RexNode[] narrowBigintArgs(
     RexBuilder builder,
     BuiltinFunctionName functionName,
     CalciteFuncSignature signature,
     RexNode... args) {
   // Only narrow the int-domain control positions of functions known to require Java int params.
   int[] intPositions = INT_PARAM_POSITIONS.get(functionName);
   if (intPositions == null) {
     return args;
   }
   RexNode[] narrowed = null;
   for (int pos : intPositions) {
-    if (pos < args.length && args[pos].getType().getSqlTypeName() == SqlTypeName.BIGINT) {
+    if (pos < args.length && pos < signature.getParamTypes().size()
+        && args[pos].getType().getSqlTypeName() == SqlTypeName.BIGINT
+        && signature.getParamTypes().get(pos).getSqlTypeName() == SqlTypeName.INTEGER) {
Suggestion importance[1-10]: 6

__

Why: The suggestion adds validation to ensure the signature's parameter type at pos is actually INTEGER before narrowing. This makes the logic more defensive and explicit, though the current implementation relies on INT_PARAM_POSITIONS being correctly maintained to match signature expectations.

Low

Previous suggestions

Suggestions up to commit c1295f6
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add bounds check for parameter position

The loop checks if any argument needs casting but doesn't validate that
parameterTypes has enough positions for all arguments. If args.length exceeds the
size of any parameter type combination, accessing position i in
expectsIntegerAtPosition could cause index issues. Add a bounds check before calling
expectsIntegerAtPosition.

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [771-777]

 for (int i = 0; i < args.length; i++) {
   if (args[i].getType().getSqlTypeName() == SqlTypeName.BIGINT
+      && i < parameterTypes.stream().mapToInt(List::size).min().orElse(0)
       && expectsIntegerAtPosition(parameterTypes, i)) {
     needsCast = true;
     break;
   }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion identifies a potential bounds issue, but expectsIntegerAtPosition already handles bounds checking by returning false when position >= combination.size() (line 850-852). The proposed fix adds unnecessary complexity with a stream operation that doesn't improve safety.

Low
General
Verify original types before narrowing

The narrowing logic is applied to all non-arithmetic/non-comparison calls, but the
method narrowOperandsToOriginalType unconditionally narrows all BIGINT operands to
INTEGER. This could break functions that legitimately accept BIGINT parameters.
Consider checking if the original call operand types were narrower before applying
the cast.

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/serde/RexStandardizer.java [92-94]

 if (!allowNumericTypeWiden) {
-  standardizedOperands = narrowOperandsToOriginalType(call, standardizedOperands, helper);
+  standardizedOperands = narrowOperandsToOriginalType(call, standardizedOperands, call.operands, helper);
 }
Suggestion importance[1-10]: 2

__

Why: The suggestion raises a concern about unconditional narrowing, but the improved_code doesn't show how to implement the verification. The current logic is intentional per the PR's design: non-arithmetic/non-comparison functions with BIGINT operands are narrowed because they require int parameters in Calcite runtime methods. The suggestion lacks a concrete implementation.

Low

…opensearch-project#5660)

PPL queries that pass integer arithmetic as an argument to functions requiring Java int parameters fail when Calcite is enabled:

	mvindex(arr, 1 + 1)      -> CompileException: arrayItemOptional(List, long, ...)
	left('abcdef', 1 + 1)    -> Unable to implement: SqlFunctions.left(String, long)
	round(123.456, 1 + 0)    -> SqlFunctions.sround(BigDecimal, long)

Root cause: PPL widens INTEGER arithmetic to BIGINT for overflow safety (opensearch-project#5603), so expressions like `1 + 1` produce BIGINT. Many Calcite runtime methods (ITEM, LEFT, RIGHT, ROUND, TRUNCATE, SUBSTRING, CONV, SHA2, etc.) take Java int parameters. Since SqlTypeFamily.INTEGER contains BIGINT, the call passes type checking but fails at code generation because the JVM cannot auto-narrow long to int.

The bug surfaces on:
	- Local execution: Calcite EnumerableCalc codegen -> Unable to implement

Fix:
	- PPLFuncImpTable.resolve: for a known set of functions, narrow BIGINT arguments to INTEGER at the specific int-domain "control" positions (indices, lengths, precision, radix, bit-length, mode) via a per-function position map. Value/data positions are never narrowed, so e.g. round(bigint_value, 2) keeps its BIGINT first operand.

Overflow safety is preserved: the arithmetic itself still computes in BIGINT; only the final value handed to an int-domain parameter is narrowed. Arithmetic operators, comparisons, cast(x as long), aggregations, and long-field arithmetic are left untouched.

Also fixes the pre-existing case where an explicit cast(x as long) is passed to these functions.

Issue: opensearch-project#5660

Signed-off-by: Ajimelec Gonzalez <ajimelec@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0d3b11a

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] PPL query with mvindex() fails when plugins.calcite.pushdown.enabled=true

1 participant