Use leastRestrictive for mvappend element-type widening

[ahkcs]

Description

MVAppendFunctionImpl.updateMostGeneralType used strict Object.equals to compare each operand's component type against the running "most general" type, falling back to Calcite's ANY on any mismatch. Object.equals returns false for type pairs that differ only in nullability tag — e.g. array(1, 2) synthesizes INTEGER NULLABLE for its component while literal 3 is INTEGER NOT NULL — and for straightforwardly-widenable numerics like INT + DECIMAL.

The Calcite engine's enumerable runtime tolerates ANY because MVAppendImplementor.implement processes elements through Object[] — the declared element type is unused at execution time. The analytics-engine route is stricter: substrait can't serialize ANY, so isthmus throws UnsupportedOperationException: Unable to convert the type ANY during substrait conversion.

Two changes:

1. updateMostGeneralType widens via RelDataTypeFactory.leastRestrictive — the same routine SqlLibraryOperators.ARRAY uses for its return-type inference. For genuinely incompatible operand types (INT + VARCHAR, …) leastRestrictive returns null; fall back to ANY there to preserve the existing in-process Calcite engine Object[] runtime semantics that the mvappend(1, 'text', 2.5)-style tests rely on. Promote DECIMAL → DOUBLE on the way through: RowResponseCodec maps DECIMAL cells to FloatingPoint(DOUBLE) anyway, and an explicit DECIMAL element type triggers Calcite's element coercion to BigDecimal, which the JSON formatter renders inconsistently across paths.

2. MVAppendImplementor.implement pre-casts each scalar operand to the call's element Java class via EnumUtils.convert. Without this, Avatica's AbstractCursor.ArrayAccessor dispatches the per-element accessor by the declared SQL type — e.g. DoubleAccessor.getDouble does (Double) value — and would throw a runtime ClassCastException on an Integer cell when the call's element type widens to DOUBLE. Array operands pass through; their element-type alignment is the planner's responsibility.

A previous revision of this PR went straight to leastRestrictive widening and triggered a runtime corruption (mvappend(1, 2.5) → [0, 2.5]) for exactly the Avatica-cast reason above. The pre-cast in MVAppendImplementor resolves that without reintroducing the regression.

Test plan

• ./gradlew :core:test --tests "*MVAppend*" → green.
• ./gradlew :integ-test:integTest --tests "org.opensearch.sql.calcite.remote.CalciteMVAppendFunctionIT" (Calcite engine path) → 15/15 pass.
• ./gradlew :integ-test:integTest --tests "org.opensearch.sql.calcite.remote.CalciteArrayFunctionIT" → 60/60 pass (no regression on the sister IT).

Analytics-engine compatibility

This change is a companion to opensearch-project/OpenSearch#21554, which onboards mvappend to the DataFusion route. Pass-rate projection for CalciteMVAppendFunctionIT once the backend-lucene + parquet-fixture infrastructure is in place (currently tracked separately under helper-managed-index migration):

#	Test	Operand types	Analytics-engine	Why
1	testMvappendWithMultipleElements	INT, INT, INT	✅	uniform ARRAY<INT>
2	testMvappendWithSingleElement	INT	✅	uniform ARRAY<INT>
3	testMvappendWithArrayFlattening	ARRAY<INT>, ARRAY<INT>	✅	uniform ARRAY<INT>
4	testMvappendWithMixedArrayAndScalar	ARRAY<INT>, INT, INT	✅	nullability-only mismatch → bridged by leastRestrictive
5	testMvappendWithStringValues	VARCHAR, VARCHAR	✅	uniform ARRAY<VARCHAR>
6	testMvappendWithRealFields	VARCHAR, VARCHAR	✅	uniform ARRAY<VARCHAR>
7	testMvappendWithNestedArrays	3× ARRAY<VARCHAR>	✅	uniform ARRAY<VARCHAR>
8	testMvappendWithNumericArrays	2× ARRAY<DOUBLE>, DOUBLE	✅	uniform ARRAY<DOUBLE>
9	testMvappendInWhereClause	VARCHAR, VARCHAR	✅	passes the project, fails the WHERE — see below
10	testMvappendWithComplexExpression	ARRAY<INT>, ARRAY<INT>, INT	✅	nullability-only mismatch → bridged by leastRestrictive
11	testMvappendWithIntAndDouble	INT, DECIMAL	✅	NEW: widens to DOUBLE via the DECIMAL → DOUBLE promotion + operand pre-cast in this PR
12	testMvappendWithMixedTypes	INT, VARCHAR, DECIMAL	❌	genuinely heterogeneous → ARRAY<ANY> (substrait can't encode)
13	testMvappendWithFieldsAndLiterals	INT, VARCHAR, VARCHAR	❌	INT + VARCHAR → ARRAY<ANY>
14	testMvappendWithEmptyArray	ARRAY<VARCHAR> (#5421 default), INT, INT	❌	VARCHAR + INT → ARRAY<ANY>. The empty-array array() is bound to empty_arr via eval; at MVAppend's type-inference site we see the column reference, not the literal, so no per-call detection helps.
15	testMvappendWithNull	VARCHAR, INT, INT	❌	VARCHAR + INT → ARRAY<ANY> (nullif(1, 1) is INTEGER-typed, not NULL-typed)

Projected: 11/15 on the analytics-engine route after this fix, up from 10/15. The deeper investigation behind this projection — why each remaining failure isn't fixable without architectural changes (Arrow has no Union-array stdlib, the analytics-engine planner's filter rule doesn't track per-leaf-call types, and array()'s element-type default is constrained by substrait-serializability) — is summarized in the corresponding section of opensearch-project/OpenSearch#21554.

The 4 remaining failures are an architectural limitation, not a regression introduced here:

• Calcite's ANY is a JVM Object[] pass-through with no substrait/Arrow/DataFusion equivalent.
• Arrow has Union arrays but datafusion-functions-array doesn't operate on them.
• Stringifying server-side would defeat the assertions, which expect typed Java values back (Integer 1, not "1").
• Aggressive numeric widening (INT + VARCHAR → ANY) can't be salvaged without either changing PPL's mvappend contract from heterogeneous-Object[] to a uniform stringified type (breaking user expectations) or shipping Arrow Union arrays through the wire format (multi-PR work).

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 6cf2b65)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The boxedJavaClass method returns Object.class for unhandled SQL types (e.g., DATE, TIMESTAMP, BINARY), but EnumUtils.convert at line 127 may fail or produce incorrect results when converting to Object.class for these types. This can cause runtime errors or data corruption when mvappend is called with date/time or binary operands. private static Class<?> boxedJavaClass(SqlTypeName sqlType) { return switch (sqlType) { case BOOLEAN -> Boolean.class; case TINYINT -> Byte.class; case SMALLINT -> Short.class; case INTEGER -> Integer.class; case BIGINT -> Long.class; case FLOAT, REAL -> Float.class; case DOUBLE -> Double.class; case DECIMAL -> BigDecimal.class; case CHAR, VARCHAR -> String.class; default -> Object.class; }; } Type Safety When leastRestrictive returns null (line 97), the code falls back to ANY type but still attempts type coercion at line 127 with elementClass = Object.class. The condition at line 124 checks elementClass == Object.class and skips conversion, but this silently bypasses type safety for genuinely incompatible types like INT + VARCHAR. The runtime may then fail when Avatica's accessor expects homogeneous types but receives mixed Integer and String objects. RelDataType least = typeFactory.leastRestrictive(java.util.List.of(current, candidate)); if (least == null) { return typeFactory.createSqlType(SqlTypeName.ANY); } if (least.getSqlTypeName() == SqlTypeName.DECIMAL) { return typeFactory.createTypeWithNullability( typeFactory.createSqlType(SqlTypeName.DOUBLE), true); } return least; } public static class MVAppendImplementor implements NotNullImplementor { @Override public Expression implement( RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) { // Pre-cast each scalar operand to the call's element Java class so the result list is // homogeneously typed. Avatica's {@code AbstractCursor.ArrayAccessor} dispatches the // per-element accessor by the declared SQL type — e.g. {@code DoubleAccessor.getDouble} // does {@code (Double) value} — and would throw a runtime ClassCastException on an // {@code Integer} cell when the call's element type widens to DOUBLE. Array operands // pass through; their element-type alignment is the planner's responsibility. RelDataType elementType = call.getType().getComponentType(); Class<?> elementClass = elementType == null ? Object.class : boxedJavaClass(elementType.getSqlTypeName()); List<Expression> coerced = new ArrayList<>(translatedOperands.size()); for (int i = 0; i < translatedOperands.size(); i++) { Expression op = translatedOperands.get(i); RelDataType opType = call.getOperands().get(i).getType(); if (opType.getComponentType() != null || elementClass == Object.class) { coerced.add(op); } else { coerced.add(EnumUtils.convert(op, elementClass)); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 6cf2b65

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add null safety for SQL type The elementType null check may be insufficient. If elementType.getSqlTypeName() returns null or an unmapped type, boxedJavaClass returns Object.class, but this could mask type resolution issues. Consider validating the SQL type name explicitly. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java [118-119] -Class<?> elementClass = - elementType == null ? Object.class : boxedJavaClass(elementType.getSqlTypeName()); +SqlTypeName sqlTypeName = elementType == null ? null : elementType.getSqlTypeName(); +Class<?> elementClass = (elementType == null || sqlTypeName == null) + ? Object.class + : boxedJavaClass(sqlTypeName); Suggestion importance[1-10]: 5 __ Why: The suggestion adds defensive null checking for getSqlTypeName(), which could improve robustness. However, the boxedJavaClass method already handles unmapped types by returning Object.class in the default case, so this is a minor defensive improvement rather than fixing a critical bug.	Low
	Remove unnecessary fully qualified name Using the fully qualified java.util.List is unnecessary since List is already imported. This creates inconsistency with other usages in the file and reduces code readability. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java [96] -RelDataType least = typeFactory.leastRestrictive(java.util.List.of(current, candidate)); +RelDataType least = typeFactory.leastRestrictive(List.of(current, candidate)); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that java.util.List is redundant since List is already imported at line 12. However, this is a minor style improvement with minimal impact on functionality.	Low

---

Previous suggestions

Suggestions up to commit 5fc778f

Category	Suggestion	Impact
General	Use leastRestrictive for type widening Consider using SqlTypeUtil.leastRestrictive instead of manually checking nullability equality and creating nullable types. This aligns with the PR title and provides a more robust type widening mechanism that handles edge cases consistently with Calcite's type system. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java [95-97] -if (SqlTypeUtil.equalSansNullability(typeFactory, current, candidate)) { - return typeFactory.createTypeWithNullability(current, true); +RelDataType leastRestrictive = SqlTypeUtil.leastRestrictive(typeFactory, List.of(current, candidate)); +if (leastRestrictive != null) { + return leastRestrictive; } Suggestion importance[1-10]: 4 __ Why: While SqlTypeUtil.leastRestrictive could provide more robust type widening, the PR's explicit comment explains that the current approach intentionally avoids widening genuinely different types (INT + DOUBLE, INT + VARCHAR) to preserve heterogeneous values. Using leastRestrictive might contradict this design decision by performing unwanted type coercion.	Low

Suggestions up to commit 5ab670b

Category	Suggestion	Impact
General	Add null check for candidate parameter Consider validating that current and candidate are non-null before calling leastRestrictive. While the method checks current == null earlier, candidate could theoretically be null, which might cause unexpected behavior in leastRestrictive. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java [91-92] +if (candidate == null) { + return current; +} RelDataType least = typeFactory.leastRestrictive(java.util.List.of(current, candidate)); return least != null ? least : typeFactory.createSqlType(SqlTypeName.ANY); Suggestion importance[1-10]: 5 __ Why: The suggestion adds defensive null-checking for candidate, which could improve robustness. However, this is a minor improvement since the method likely receives non-null values from its callers in practice, and the score is moderate because it's primarily a defensive programming practice rather than fixing a critical bug.	Low

[github-actions]

Persistent review updated to latest commit 5fc778f

[github-actions]

Persistent review updated to latest commit 6cf2b65