[AjimelecGonzalez]

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. Some runtime methods take a Java int with no long version. These include Calcite built-ins such as ITEM, LEFT, RIGHT, ROUND, TRUNCATE, RAND and LOCATE, and PPL functions such as CONV, SHA2 and WEEK. Since SqlTypeFamily.INTEGER contains BIGINT, the call passes type checking but fails at code generation because Java cannot pass a long to an int parameter.

Fix:

• Calcite built-ins: narrow during function resolution, only where needed. After the matched implementation builds its call, PPLFuncImpTable#resolve checks the call's operator. The new CalciteIntParameters reads the Java methods Calcite uses for that operator (from RexImpTable). A BIGINT argument is cast to INTEGER only when some version of the method takes an int at that position and none takes a long.
• No list to maintain. The decision comes from the real Java methods, not from a hand-written list or from type families, so new functions are handled automatically. There is no special register method; every function still uses registerOperator.
• Fallback. If Calcite doesn't expose the method (today only ITEM), the signature that matched the call decides. So ITEM(array, index) is narrowed and ITEM(map, key) is not. CalciteIntParameters reads Calcite's implementor table reflectively; if that ever fails (for example after a Calcite upgrade), it falls back to the same signature rule, and BigintArgumentCodegenTest fails the build if any function stops compiling.
• PPL functions take long instead of int. CONV, SHA2, WEEK, SYSDATE, TONUMBER, REX_EXTRACT, REX_EXTRACT_MULTI and SPAN build their method calls themselves at code generation time, so there is no registered method to inspect. They now accept long directly and convert with UserDefinedFunctionUtils#toIntArg where an int is needed. This needs no narrowing and changes nothing for valid inputs.

Narrowed to INTEGER	Kept as BIGINT
ITEM index (arrays) / mvindex	SUBSTRING / SUBSTR (Calcite has long versions)
ARRAY_SLICE start, length / mvindex range	the value in ROUND / TRUNCATE
LEFT, RIGHT length	ITEM map keys
ROUND, TRUNCATE decimal places	all PPL functions (they take long), e.g. conv(4294967296, 10, 16) still returns 100000000
RAND seed	TIMESTAMPADD amount, ADDDATE / SUBDATE days
LOCATE / POSITION start
REGEXP_REPLACE_5 position, occurrence

Overflow safety is kept: the math still runs in BIGINT, and only the final value passed to an int argument is narrowed. Math operators, comparisons, aggregations and math on long fields are not changed. If a value is too big for an int, like left(s, 3000000000), the query fails with Value 3000000000 out of range instead of a code generation error. It never wraps to a wrong value.

This also fixes passing cast(x as long) to these functions, which failed before #5603 too.

Testing:

• UDFIntParameterTest: fails the build if any PPL function method takes an int, and tests toIntArg.
• BigintArgumentCodegenTest: goes through every registered function and compiles each signature with INTEGER arguments, then with BIGINT. It fails if only the BIGINT version breaks, so new functions are checked automatically.
• PPLFuncImpTableNarrowBigintTest:
  • What is narrowed: ITEM arrays, ARRAY_SLICE, LEFT, LOCATE, TRUNCATE.
  • What is kept: ITEM map keys, SUBSTRING, the ROUND value, all PPL function arguments, the TIMESTAMPADD amount, the ADDDATE days.
  • Also: nullability is kept, and the caller's argument array is not modified.
• Integration tests (CalciteArrayFunctionIT, CalciteTextFunctionIT, CalciteMathematicalFunctionIT, CalcitePPLBuiltinFunctionIT):
  • Math arguments work for mvindex, left, right, substring, locate, round, truncate, rand, conv, sha2, week, tonumber and sysdate; cast(x as long) arguments work for mvindex, left, right and round.
  • conv(4294967296, 10, 16) returns 100000000, and adddate(date('2020-01-01'), 1 + 1) returns 2020-01-03.
  • substring with a BIGINT above the int range returns the same results as before this change.
• Also verified manually with pushdown enabled: stats, sort, where and eventstats using these functions with arithmetic all pass.

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]

PR Reviewer Guide 🔍

(Review updated until commit 4893e91)

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 narrowing logic does not validate that the BIGINT value fits within INTEGER range before casting. If a user passes a BIGINT literal or expression that exceeds Integer.MAX_VALUE (e.g., 3000000000L) to a function expecting an int parameter, the cast will silently overflow, producing an incorrect negative value. This can occur with functions like ITEM, ROUND, or SUBSTRING when the index/precision argument is a large BIGINT. The test 'testAddDateWithBigintDays' shows a value of '1 + 1' is safe, but a user could supply 'cast(3000000000 as long)' as an index, which would overflow. protected static FunctionImp narrowIntArgs(FunctionImp functionImp, int... intArgPositions) { return (RexBuilder builder, RexNode... args) -> { RexNode[] narrowed = args; for (int pos : intArgPositions) { if (pos >= args.length || args[pos].getType().getSqlTypeName() != SqlTypeName.BIGINT) { continue; } if (narrowed == args) { narrowed = args.clone(); } RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, args[pos].getType().isNullable()); narrowed[pos] = builder.makeCast(intType, args[pos]); } return functionImp.resolve(builder, narrowed); }; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 19d66ce

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate narrowable array length matches operands The method doesn't validate that narrowable.length matches operands.size() before iterating. If CalciteIntParameters.narrowablePositions or signatureIntegerPositions returns an array of incorrect length, the loop could access out-of-bounds indices or skip positions. Add a length check to prevent potential ArrayIndexOutOfBoundsException. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [704-729] private static RexNode narrowIntArgs( RexBuilder builder, @Nullable PPLTypeChecker typeChecker, RexNode[] args, RexNode node) { if (!(node instanceof RexCall call) || call.getOperator() instanceof SqlUserDefinedFunction) { return node; } List<RexNode> operands = call.getOperands(); boolean[] narrowable = CalciteIntParameters.narrowablePositions(call.getOperator(), operands.size()) .orElseGet(() -> signatureIntegerPositions(typeChecker, args, call)); - if (narrowable == null) { + if (narrowable == null || narrowable.length != operands.size()) { return node; } - List<RexNode> narrowed = null; - for (int pos = 0; pos < operands.size(); pos++) { - RexNode operand = operands.get(pos); - if (!narrowable[pos] || operand.getType().getSqlTypeName() != SqlTypeName.BIGINT) { - continue; - } - if (narrowed == null) { - narrowed = new ArrayList<>(operands); - } - RelDataType intType = - TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, operand.getType().isNullable()); - narrowed.set(pos, builder.makeCast(intType, operand)); - } - return narrowed == null ? node : call.clone(call.getType(), narrowed); + ... } Suggestion importance[1-10]: 7 __ Why: This is a valid defensive programming suggestion. While CalciteIntParameters.narrowablePositions is called with operands.size() as the arity parameter, and signatureIntegerPositions creates an array of args.length, there's no guarantee these match operands.size() in all cases. Adding a length check prevents potential ArrayIndexOutOfBoundsException and makes the code more robust.	Medium
General	Use trySetAccessible for safer reflection The reflective field access with setAccessible(true) may fail in restricted environments or future Java versions with stricter module access controls. The code silently catches these failures, which could lead to incorrect narrowing decisions. Consider logging when reflection fails or documenting this limitation. core/src/main/java/org/opensearch/sql/expression/function/CalciteIntParameters.java [89-126] private static List<Method> implementationMethods(SqlOperator operator) { Object implementor; try { implementor = RexImpTable.INSTANCE.get(operator); } catch (RuntimeException e) { return List.of(); } ... for (Class<?> type = implementor.getClass(); type != null && type.getName().startsWith("org.apache.calcite."); type = type.getSuperclass()) { for (Field field : type.getDeclaredFields()) { if (Modifier.isStatic(field.getModifiers())) { continue; } Object value; try { - field.setAccessible(true); + if (!field.trySetAccessible()) { + continue; + } value = field.get(implementor); } catch (ReflectiveOperationException | RuntimeException e) { continue; } ... } } return methods; } Suggestion importance[1-10]: 5 __ Why: Using trySetAccessible() is a safer alternative to setAccessible(true) in modern Java, as it returns false instead of throwing an exception when access cannot be granted. However, the current code already handles failures by catching exceptions and continuing, so the impact is moderate. The suggestion improves code quality but doesn't fix a critical issue.	Low
	Validate overload return types match The method collects all overloads with matching name and arity but doesn't verify they're actually valid for the operator. This could include unrelated methods from the same class that happen to share the name. Consider filtering overloads to ensure they match the operator's expected signature or return type before analyzing parameter types. core/src/main/java/org/opensearch/sql/expression/function/CalciteIntParameters.java [60-86] private static Optional<boolean[]> compute(Key key) { List<Method> overloads = new ArrayList<>(); for (Method method : implementationMethods(key.operator())) { for (Method candidate : method.getDeclaringClass().getMethods()) { if (candidate.getName().equals(method.getName()) && candidate.getParameterCount() == key.arity() - && !candidate.isVarArgs()) { + && !candidate.isVarArgs() + && candidate.getReturnType().equals(method.getReturnType())) { overloads.add(candidate); } } } - if (overloads.isEmpty()) { - return Optional.empty(); - } - boolean[] narrowable = new boolean[key.arity()]; - for (int pos = 0; pos < key.arity(); pos++) { - boolean acceptsLong = false; - boolean acceptsInt = false; - for (Method overload : overloads) { - Class<?> parameter = overload.getParameterTypes()[pos]; - acceptsLong |= ACCEPTS_LONG.contains(parameter); - acceptsInt |= parameter == int.class || parameter == Integer.class; - } - narrowable[pos] = acceptsInt && !acceptsLong; - } - return Optional.of(narrowable); + ... } Suggestion importance[1-10]: 3 __ Why: The suggestion to filter overloads by return type is reasonable but may be overly restrictive. Calcite operators can have overloads with different return types (e.g., ROUND can return different numeric types). The current approach of matching by name and arity is standard for method resolution, and the suggestion doesn't demonstrate a concrete bug.	Low

---

Previous suggestions

Suggestions up to commit d277c86

Category	Suggestion	Impact
General	Cache reflection field for better performance Reflective access to the protected composition field may fail in restricted environments or future Calcite versions. The method silently returns null on failure, which could lead to incorrect behavior. Consider caching the field reference or handling failures more explicitly. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [789-798] +private static final Field COMPOSITION_FIELD; +static { + Field field = null; + try { + field = CompositeOperandTypeChecker.class.getDeclaredField("composition"); + field.setAccessible(true); + } catch (ReflectiveOperationException | InaccessibleObjectException | SecurityException e) { + // Log warning: cannot access composition field + } + COMPOSITION_FIELD = field; +} + private static CompositeOperandTypeChecker.@Nullable Composition compositionOf( CompositeOperandTypeChecker typeChecker) { + if (COMPOSITION_FIELD == null) { + return null; + } try { - Field compositionField = CompositeOperandTypeChecker.class.getDeclaredField("composition"); - compositionField.setAccessible(true); - return (CompositeOperandTypeChecker.Composition) compositionField.get(typeChecker); + return (CompositeOperandTypeChecker.Composition) COMPOSITION_FIELD.get(typeChecker); } catch (ReflectiveOperationException | InaccessibleObjectException | SecurityException e) { return null; } } Suggestion importance[1-10]: 5 __ Why: Caching the Field reference would improve performance by avoiding repeated reflection lookups. However, the method is already called infrequently (only during type checking), so the performance gain is modest. The suggestion is correct but offers limited practical benefit.	Low
	Add logging for reflection failures Reflective access to internal Calcite classes may fail in restricted environments or future Java versions with stricter module access. The code silently catches exceptions and continues, which could mask critical failures. Consider logging warnings when reflection fails to aid debugging. core/src/main/java/org/opensearch/sql/expression/function/CalciteIntParameters.java [89-125] private static List<Method> implementationMethods(SqlOperator operator) { Object implementor; try { implementor = RexImpTable.INSTANCE.get(operator); } catch (RuntimeException e) { + // Log warning: failed to get implementor for operator return List.of(); } ... for (Class<?> type = implementor.getClass(); type != null && type.getName().startsWith("org.apache.calcite."); type = type.getSuperclass()) { for (Field field : type.getDeclaredFields()) { ... try { field.setAccessible(true); value = field.get(implementor); } catch (ReflectiveOperationException | RuntimeException e) { + // Log warning: failed to access field in Calcite implementor continue; } ... } } return methods; } Suggestion importance[1-10]: 4 __ Why: Adding logging for reflection failures would improve debuggability, but the current silent failure is intentional as a fallback mechanism. The suggestion is valid but has moderate impact since the code already handles failures gracefully by returning empty results.	Low
	Filter method candidates by declaring class The method lookup logic may include unintended overloads from parent classes or unrelated methods with the same name. Consider filtering candidates to only include methods from the same declaring class as the implementation method, or verify that the candidate is actually callable from the implementation context. core/src/main/java/org/opensearch/sql/expression/function/CalciteIntParameters.java [60-70] private static Optional<boolean[]> compute(Key key) { List<Method> overloads = new ArrayList<>(); for (Method method : implementationMethods(key.operator())) { - for (Method candidate : method.getDeclaringClass().getMethods()) { + Class<?> declaringClass = method.getDeclaringClass(); + for (Method candidate : declaringClass.getMethods()) { if (candidate.getName().equals(method.getName()) && candidate.getParameterCount() == key.arity() - && !candidate.isVarArgs()) { + && !candidate.isVarArgs() + && candidate.getDeclaringClass().equals(declaringClass)) { overloads.add(candidate); } } } if (overloads.isEmpty()) { return Optional.empty(); } ... } Suggestion importance[1-10]: 3 __ Why: The suggestion adds a redundant filter since getMethods() already returns methods from the declaring class and its superclasses. The additional check candidate.getDeclaringClass().equals(declaringClass) would incorrectly exclude inherited methods that may be valid overloads.	Low

Suggestions up to commit 4893e91

Category	Suggestion	Impact
Possible issue	Add null check for arguments The null check for args[pos] is missing before calling getType(). If a RexNode at the specified position is null, this will throw a NullPointerException. Add a null check to handle this edge case safely. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [882-898] protected static FunctionImp narrowIntArgs(FunctionImp functionImp, int... intArgPositions) { return (RexBuilder builder, RexNode... args) -> { RexNode[] narrowed = args; for (int pos : intArgPositions) { - if (pos >= args.length || args[pos].getType().getSqlTypeName() != SqlTypeName.BIGINT) { + if (pos >= args.length || args[pos] == null || args[pos].getType().getSqlTypeName() != SqlTypeName.BIGINT) { continue; } if (narrowed == args) { narrowed = args.clone(); } - ... + RelDataType intType = + TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, args[pos].getType().isNullable()); + narrowed[pos] = builder.makeCast(intType, args[pos]); } return functionImp.resolve(builder, narrowed); }; } Suggestion importance[1-10]: 7 __ Why: Adding a null check for args[pos] before calling getType() prevents a potential NullPointerException. This is a valid defensive programming practice, though the likelihood of null RexNode arguments in this context may be low based on the framework's design.	Medium
General	Add negative index validation The method does not validate that intArgPositions contains valid indices. If a negative position is provided, it will be silently skipped by the pos >= args.length check, but negative indices could indicate a programming error. Add validation to reject negative positions or document that negative values are intentionally ignored. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [882-898] protected static FunctionImp narrowIntArgs(FunctionImp functionImp, int... intArgPositions) { return (RexBuilder builder, RexNode... args) -> { RexNode[] narrowed = args; for (int pos : intArgPositions) { - if (pos >= args.length || args[pos].getType().getSqlTypeName() != SqlTypeName.BIGINT) { + if (pos < 0 || pos >= args.length || args[pos].getType().getSqlTypeName() != SqlTypeName.BIGINT) { continue; } if (narrowed == args) { narrowed = args.clone(); } RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, args[pos].getType().isNullable()); narrowed[pos] = builder.makeCast(intType, args[pos]); } return functionImp.resolve(builder, narrowed); }; } Suggestion importance[1-10]: 4 __ Why: Adding a check for pos < 0 improves robustness by catching potential programming errors. However, the impact is limited since negative positions would be silently skipped anyway, and the PR's test coverage doesn't indicate this is a real concern.	Low

Suggestions up to commit 4893e91

Category	Suggestion	Impact
General	Validate argument positions are non-negative The method does not validate that intArgPositions contains valid indices. If a negative position is provided, it will be silently skipped by the pos >= args.length check, but this could mask configuration errors. Add validation to ensure all positions are non-negative to catch registration mistakes early. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [882-898] protected static FunctionImp narrowIntArgs(FunctionImp functionImp, int... intArgPositions) { + for (int pos : intArgPositions) { + if (pos < 0) { + throw new IllegalArgumentException("Invalid argument position: " + pos); + } + } return (RexBuilder builder, RexNode... args) -> { RexNode[] narrowed = args; for (int pos : intArgPositions) { if (pos >= args.length || args[pos].getType().getSqlTypeName() != SqlTypeName.BIGINT) { continue; } if (narrowed == args) { narrowed = args.clone(); } RelDataType intType = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, args[pos].getType().isNullable()); narrowed[pos] = builder.makeCast(intType, args[pos]); } return functionImp.resolve(builder, narrowed); }; } Suggestion importance[1-10]: 5 __ Why: Adding validation for negative positions would help catch configuration errors during registration. However, the current implementation already handles invalid positions gracefully by skipping them, and the test declaredPositionBeyondArityIsIgnored shows this is intentional behavior. The validation would be a minor improvement for developer experience but not critical.	Low

Suggestions up to commit 2dde495

Category	Suggestion	Impact
Possible issue	Add null check for type The code accesses combination.get(position) without null-checking the returned RelDataType. If a null type is present in the combination list, this will cause a NullPointerException. Add a null check before calling getSqlTypeName(). core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [90-100] for (List<RelDataType> combination : parameterTypes) { if (position >= combination.size()) { continue; } - SqlTypeName typeName = combination.get(position).getSqlTypeName(); + RelDataType type = combination.get(position); + if (type == null) { + continue; + } + SqlTypeName typeName = type.getSqlTypeName(); if (typeName == SqlTypeName.INTEGER) { sawInteger = true; } else if (typeName == SqlTypeName.BIGINT || typeName == SqlTypeName.DOUBLE) { return false; } } Suggestion importance[1-10]: 7 __ Why: Adding a null check before calling getSqlTypeName() prevents a potential NullPointerException if the type list contains null elements. This is a defensive programming practice that improves code robustness, though the likelihood of null types in practice may be low.	Medium
General	Validate position parameter is non-negative The method does not validate that position is non-negative. A negative position could cause unexpected behavior when iterating through combinations. Add a guard to return false for negative positions. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [84-102] default boolean expectsIntegerFamilyAt(int position) { + if (position < 0) { + return false; + } List<List<RelDataType>> parameterTypes = getParameterTypes(); if (parameterTypes == null || parameterTypes.isEmpty()) { return false; } boolean sawInteger = false; for (List<RelDataType> combination : parameterTypes) { if (position >= combination.size()) { continue; } SqlTypeName typeName = combination.get(position).getSqlTypeName(); if (typeName == SqlTypeName.INTEGER) { sawInteger = true; } else if (typeName == SqlTypeName.BIGINT || typeName == SqlTypeName.DOUBLE) { return false; } } return sawInteger; } Suggestion importance[1-10]: 6 __ Why: Adding a guard for negative position values prevents potential unexpected behavior. While the current code would handle it gracefully (the loop condition position >= combination.size() would skip all combinations), explicit validation improves robustness and clarity.	Low
	Log exception for debugging purposes The catch block silently returns the original arguments when an IllegalArgumentException occurs, which may hide legitimate errors. Consider logging the exception at debug level to aid troubleshooting while still maintaining the fallback behavior. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [766-773] for (int pos = 0; pos < args.length; pos++) { if (args[pos].getType().getSqlTypeName() != SqlTypeName.BIGINT) { continue; } boolean expectsInteger; try { expectsInteger = typeChecker.expectsIntegerFamilyAt(pos); } catch (IllegalArgumentException e) { // A type checker whose composition is not purely family-based cannot enumerate per-position // families. We cannot reason about int-domain positions, so skip narrowing rather than // fail an otherwise-valid query. + LOG.debug("Cannot determine int-domain positions for type checker, skipping narrowing", e); return args; } ... } Suggestion importance[1-10]: 5 __ Why: Adding debug logging for the caught IllegalArgumentException would help troubleshooting without changing behavior. However, this is a minor enhancement since the fallback behavior is already well-documented in the comment.	Low

Suggestions up to commit 6b14555

Category	Suggestion	Impact
Possible issue	Add overflow check for BIGINT narrowing The cast from BIGINT to INTEGER may cause overflow for values outside the INTEGER range (-2^31 to 2^31-1). Consider adding overflow detection or bounds checking before the cast to prevent silent data corruption when BIGINT values exceed INTEGER limits. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [774-782] if (expectsInteger) { if (narrowed == null) { narrowed = args.clone(); } RelDataType intType = TYPE_FACTORY.createTypeWithNullability( TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER), args[pos].getType().isNullable()); + // Add overflow check for literal values + if (args[pos] instanceof RexLiteral) { + RexLiteral literal = (RexLiteral) args[pos]; + long value = ((Number) literal.getValue()).longValue(); + if (value < Integer.MIN_VALUE || value > Integer.MAX_VALUE) { + throw new IllegalArgumentException("Value " + value + " exceeds INTEGER range"); + } + } narrowed[pos] = builder.makeCast(intType, args[pos]); } Suggestion importance[1-10]: 3 __ Why: While overflow checking could be beneficial, the suggestion only handles RexLiteral cases and doesn't address runtime values. The PR's context suggests these are typically small integer values (indices, precision parameters) where overflow is unlikely. The suggestion adds complexity for a rare edge case without comprehensive coverage.	Low

[github-actions]

Persistent review updated to latest commit 0d3b11a

[dai-chen]

High level question: the root cause is we widen arithmetic expressions to avoid overflow "unconditionally", can we list the options we've explored besides current PR approach?

[dai-chen]

FYI, in case it's useful — I did a quick check of how other databases handle integer arithmetic overflow. There are two independent decisions (partially done in #5603), and they explain our current implementation challenge in Calcite's combination.

Option 1 — what type does INT + INT get?

Option	Engines	INT + INT	consequence
Keep narrow	PostgreSQL, Calcite, Spark, Trino, DuckDB, SQL Server	INT	overflow is possible at 32 bits, so Option 2 applies there
Widen one tier	MySQL, ClickHouse	BIGINT / Int64	overflow unreachable at 32 bits (int32×int32 = 2⁶² < 2⁶³); Option 2 applies at the 64-bit ceiling
One integer width	BigQuery (INT/SMALLINT/BIGINT are aliases for INT64)	INT64	no narrow tier exists; Option 2 applies at 64 bits

Note: widening only works because those engines' function libraries read every integer argument at 64 bits (MySQL val_int() → longlong, ClickHouse getInt, BigQuery INT64-only, Trino @SqlType(INTEGER) long) — unlike Calcite, whose left/right/position/arrayItemOptional/sround/struncate take primitive int with no long overload.

Option 2 — what happens when a value doesn't fit the type it was given?

Option	Engines	observed
Throw	PostgreSQL, DuckDB, Trino, Spark-ANSI, SQL Server, BigQuery, MySQL	PG 2147483647+1 → ERROR 22003 integer out of range (int4pl) · DuckDB → Out of Range Error: Overflow in addition of INT32 · MySQL 9223372036854775807+1 → ERROR 1690 (22003) BIGINT value is out of range
No check — result wraps	Calcite default, Spark legacy, DataFusion, ClickHouse	Calcite 2147483647+1 → -2147483648 · ClickHouse toInt64(9223372036854775807)+1 → -9223372036854775808
Per-call opt-out → NULL	BigQuery SAFE_ADD, Spark try_add, Trino/DuckDB TRY()	explicit escape hatch layered over one of the above — never a default

[github-actions]

Persistent review updated to latest commit f6d8acb

[AjimelecGonzalez]

High level question: the root cause is we widen arithmetic expressions to avoid overflow "unconditionally", can we list the options we've explored besides current PR approach?

As for the options explored:

Options explored:

1. Revert Widen narrow integer operands in PPL +/-/* to prevent overflow #5603 (don't widen) — reintroduces silent 32-bit overflow. Also doesn't fully fix it: the BIGINT-into-int consumer defect is pre-existing (mvindex(arr, cast(1 as long)) failed before Widen narrow integer operands in PPL +/-/* to prevent overflow #5603).
2. Fix per-function impl (e.g. in MVIndexFunctionImp) — tried in fix: cast mvindex index arithmetic to INTEGER type for Calcite ITEM compatibility (#5660) #5670; the plan-layer cast gets re-derived away, and it doesn't scale to every affected function.
3. Serializer-layer cast (RexStandardizer) — fix: [bug] PPL query with mvindex() fails when plugins.calcite.pushdown.enabled=true (#5660) #5689; fixes pushdown only, misses local EnumerableCalc execution.
4. Conditional widening (only widen when overflow is possible) — can't know operand magnitude at plan time for fields, so you'd widen conservatively anyway — same problem.
5. Narrow at the consumer boundary (this PR) — chosen; single chokepoint, covers all paths, keeps overflow-safe BIGINT arithmetic intact. Positions are derived from each operator's type checker (no hardcoded list appoarch need).

[github-actions]

Persistent review updated to latest commit f1957a4

[github-actions]

Persistent review updated to latest commit 6b14555

[codecov]

Codecov Report

❌ Patch coverage is 69.18239% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.66%. Comparing base (9249643) to head (19d66ce).

Files with missing lines	Patch %	Lines
...search/sql/expression/function/PPLTypeChecker.java	39.58%	22 Missing and 7 partials ⚠️
.../sql/expression/function/CalciteIntParameters.java	84.61%	4 Missing and 4 partials ⚠️
...arch/sql/expression/function/udf/SpanFunction.java	0.00%	6 Missing ⚠️
...ression/function/udf/datetime/SysdateFunction.java	0.00%	2 Missing ⚠️
...earch/sql/expression/function/PPLFuncImpTable.java	97.61%	0 Missing and 1 partial ⚠️
...expression/function/udf/CryptographicFunction.java	0.00%	1 Missing ⚠️
...expression/function/udf/datetime/WeekFunction.java	0.00%	1 Missing ⚠️
...sql/expression/function/udf/math/ConvFunction.java	0.00%	1 Missing ⚠️

❌ Your project check has failed because the head coverage (75.66%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5728      +/-   ##
============================================
+ Coverage     75.19%   75.66%   +0.46%     
- Complexity    10362    10429      +67     
============================================
  Files           962      963       +1     
  Lines         41098    41244     +146     
  Branches       4683     4726      +43     
============================================
+ Hits          30905    31207     +302     
+ Misses         8896     8727     -169     
- Partials       1297     1310      +13     

Flag	Coverage Δ
sql-engine	75.66% <69.18%> (+0.46%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 2dde495

[github-actions]

Persistent review updated to latest commit 4893e91

[github-actions]

Persistent review updated to latest commit 4893e91

[dai-chen]

1. Can we avoid separate register method because this requires future developers' awareness?
2. I recall in previous comment we align the narrowing can be down during function resolution instead of registration? Ref: fix: [bug] narrow BIGINT to INTEGER for int-domain function arguments (#5660) #5728 (comment)

[AjimelecGonzalez]

Thanks, both are addressed in the latest commit PR. The separate register method is gone and every function uses plain registerOperator again. Narrowing now happens in PPLFuncImpTable#resolve, as we agreed. I had moved it back to registration by mistake when replacing the type-family inference.

Now, there's also no list to maintain. For Calcite built-ins, the new CalciteIntParameters reads the Java methods Calcite actually calls and narrows a BIGINT only where no overload accepts a long. PPL UDFs now take long instead of int, so they need no narrowing. UDFIntParameterTest and BigintArgumentCodegenTest fail the build if a new function breaks either rule, so nobody has to remember anything.