Fix LIST/VALUES aggregate type-mismatch

[vinaykpud]

Description

Fixes two bugs that combined to make stats list(<field>) / stats values(<field>) unusable on the analytics-engine path:

1. Type-mismatch AssertionError for every element type (31 failures in CalciteMultiValueStatsIT).
2. unsupported object class [B specifically for list(ip_value) / list(binary_value) (and the values() equivalents).

Bug 1 — nullability mismatch

PplAggregateCallRewriter built explicitReturnType for LIST/VALUES via createArrayType(elemType, -1) — the 2-arg overload defaults to NOT NULL. LOCAL_ARRAY_AGG_OP's inferReturnType is TO_ARRAY.andThen(FORCE_NULLABLE) — nullable. Calcite's Aggregate.typeMatchesInferred asserted on the mismatch.

Fix: build the explicit return type with createTypeWithNullability(arrayType, true) so it aligns with the operator's inference. Aggregate output is inherently nullable on empty input, so this matches the semantics too.

Bug 2 — byte[] rendering for IP / binary list elements

After bug 1 was fixed, IP/binary list queries returned HTTP 400 unsupported object class [B. LIST / VALUES are registered with STRING_ARRAY return type, but DataFusion's array_agg on an IpType / BinaryType column accumulates raw byte[] payloads — which the SQL plugin's response converter cannot render.

Fix: insert a LogicalProject above the aggregate that lifts every non-VARCHAR scalar operand of a LIST/VALUES call into a VARCHAR column. IpType routes to the existing ip_to_string Rust UDF, BinaryType to binary_to_base64, other types use a plain CAST(... AS VARCHAR). The aggregator then produces ARRAY<VARCHAR>, so byte[] elements never reach the response boundary.

Direct UDF dispatch (rather than a plain CAST for IP/binary): IpBinaryCastFunctionAdapter runs in an earlier pass, so a CAST emitted at this stage would fall through to DataFusion's Latin-1 cast kernel and corrupt the bytes.

Testing

Verified locally on Variant B (parquet+lucene+SQL plugin, plugins.calcite.enabled=true):

Type	Before bug 1 fix	After both fixes
boolean, byte, short, integer, long, float, double, keyword, text, date, date_nanos	HTTP 500 AssertionError	HTTP 200 with ARRAY<VARCHAR>
ip	HTTP 500 AssertionError	HTTP 200 → ["127.0.0.1"]
binary	HTTP 500 AssertionError	HTTP 200 → ["U29tZSBiaW5hcnkgYmxvYg=="]

ListAggregateMultiTypeIT (new) covers all 13 supported scalar types end-to-end. The 31 CalciteMultiValueStatsIT cases this category covers also pass.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 97ca5f2)

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 collectListValuesScalarOperands method skips ARRAY operands by checking argType.getComponentType() != null, but it does not verify that argIdx is within bounds of origFields. If a LIST/VALUES call references an out-of-bounds column index (e.g., due to upstream corruption or a malformed query plan), origFields.get(argIdx) throws IndexOutOfBoundsException. This crashes the rewriter instead of gracefully handling or reporting the invalid state. List<RelDataTypeField> origFields = agg.getInput().getRowType().getFieldList(); int origFieldCount = origFields.size(); Map<Integer, Integer> castMap = new LinkedHashMap<>(); for (AggregateCall call : agg.getAggCallList()) { if (!isListOrValuesCall(call) || call.getArgList().isEmpty()) { continue; } int argIdx = call.getArgList().get(0); RelDataType argType = origFields.get(argIdx).getType(); if (argType.getComponentType() != null || argType.getSqlTypeName() == SqlTypeName.VARCHAR) { continue; } castMap.putIfAbsent(argIdx, origFieldCount + castMap.size()); } return castMap; Possible Issue In rewireListValuesCalls, the code retrieves call.getArgList().get(0) without checking if call.getArgList() is non-empty. Although isListOrValuesCall(call) && !call.getArgList().isEmpty() guards the map lookup, if castMap.get(...) returns null (operand not lifted), the code falls through to rewired.add(call) without issue. However, if a LIST/VALUES call somehow has an empty argument list and isListOrValuesCall returns true, newIdx assignment would attempt .get(0) on an empty list, causing IndexOutOfBoundsException. The guard !call.getArgList().isEmpty() in the ternary prevents this only if the condition is evaluated; if future refactoring removes that check or reorders logic, the crash becomes possible. Integer newIdx = isListOrValuesCall(call) && !call.getArgList().isEmpty() ? castMap.get(call.getArgList().get(0)) : null; if (newIdx == null) {

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to c8a8250

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate operand index bounds The logic assumes castMap.get() returns null when the key is absent, but if a LIST/VALUES call references an operand that wasn't lifted (e.g., already VARCHAR or ARRAY), castMap.get() returns null and the call is left unchanged. However, this could mask cases where the operand index is out of bounds. Add a bounds check to prevent potential IndexOutOfBoundsException. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [183-187] +if (!call.getArgList().isEmpty()) { + int argIdx = call.getArgList().get(0); + if (argIdx >= lifted.getRowType().getFieldCount()) { + throw new IllegalStateException("Operand index out of bounds: " + argIdx); + } +} Integer newIdx = isListOrValuesCall(call) && !call.getArgList().isEmpty() ? castMap.get(call.getArgList().get(0)) : null; if (newIdx == null) { rewired.add(call); continue; } Suggestion importance[1-10]: 4 __ Why: The suggestion adds bounds checking for argIdx, but the logic already handles the case where castMap.get() returns null (operand not lifted). The bounds check could catch programming errors, but the existing null-check pattern already provides safety. The improvement is marginal since argIdx comes from call.getArgList() which should be validated by Calcite's aggregate construction.	Low
	Add null-safety validation Add null-safety checks for srcType and srcRef parameters before performing instanceof checks and operations. If either parameter is null, the method will throw a NullPointerException when attempting instanceof checks or passing null to rexBuilder methods. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [163-171] private static RexNode toVarchar(RelDataType srcType, RexNode srcRef, RelDataType varcharNullable, RexBuilder rexBuilder) { + if (srcType == null || srcRef == null) { + throw new IllegalArgumentException("srcType and srcRef must not be null"); + } if (srcType instanceof IpType) { return rexBuilder.makeCall(varcharNullable, IpBinaryCastFunctionAdapter.IP_TO_STRING_OP, List.of(srcRef)); } if (srcType instanceof BinaryType) { return rexBuilder.makeCall(varcharNullable, IpBinaryCastFunctionAdapter.BINARY_TO_BASE64_OP, List.of(srcRef)); } return rexBuilder.makeCast(varcharNullable, srcRef); } Suggestion importance[1-10]: 3 __ Why: While null-safety checks can improve robustness, this is a private static method called from controlled internal contexts where srcType and srcRef are derived from validated RelDataTypeField objects. The added null checks provide defensive programming but address a low-probability scenario given the call chain.	Low

---

Previous suggestions

Suggestions up to commit 311b3e8

Category	Suggestion	Impact
Possible issue	Add null-safety validation Add null-safety checks for srcType and srcRef parameters before performing instanceof checks and operations. If either parameter is null, the method will throw a NullPointerException when attempting instanceof checks or passing null to rexBuilder methods. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [163-171] private static RexNode toVarchar(RelDataType srcType, RexNode srcRef, RelDataType varcharNullable, RexBuilder rexBuilder) { + if (srcType == null || srcRef == null) { + throw new IllegalArgumentException("srcType and srcRef must not be null"); + } if (srcType instanceof IpType) { return rexBuilder.makeCall(varcharNullable, IpBinaryCastFunctionAdapter.IP_TO_STRING_OP, List.of(srcRef)); } if (srcType instanceof BinaryType) { return rexBuilder.makeCall(varcharNullable, IpBinaryCastFunctionAdapter.BINARY_TO_BASE64_OP, List.of(srcRef)); } return rexBuilder.makeCast(varcharNullable, srcRef); } Suggestion importance[1-10]: 3 __ Why: While null-safety is generally good practice, the toVarchar method is private and called only from buildLiftingProject, which always passes non-null values derived from origFields. The suggestion adds defensive checks that are unlikely to catch real issues in this controlled context.	Low

Suggestions up to commit d486638

Category	Suggestion	Impact
General	Extract type factory to variable Consider extracting the type factory to a local variable to avoid repeated method calls. This improves readability and potentially reduces overhead from multiple getCluster().getTypeFactory() invocations. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [179-183] // Match LOCAL_ARRAY_AGG_OP's nullable ARRAY inference; the 2-arg // createArrayType overload defaults to NOT NULL and trips Calcite's // typeMatchesInferred check. -RelDataType arrayType = agg.getCluster().getTypeFactory().createArrayType(arg0Type, -1); -explicitReturnType = agg.getCluster().getTypeFactory().createTypeWithNullability(arrayType, true); +RelDataTypeFactory typeFactory = agg.getCluster().getTypeFactory(); +RelDataType arrayType = typeFactory.createArrayType(arg0Type, -1); +explicitReturnType = typeFactory.createTypeWithNullability(arrayType, true); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a minor optimization opportunity by extracting getCluster().getTypeFactory() to a local variable. While this improves readability and avoids repeated method calls, the impact is minimal since the method is only called twice in close proximity. The suggestion is valid but offers marginal improvement.	Low

Suggestions up to commit 5f0b3be

Category	Suggestion	Impact
General	Cache type factory reference Consider caching the type factory reference to avoid repeated method calls. This improves code readability and potentially reduces overhead from multiple getCluster().getTypeFactory() invocations. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [182-183] -RelDataType arrayType = agg.getCluster().getTypeFactory().createArrayType(arg0Type, -1); -explicitReturnType = agg.getCluster().getTypeFactory().createTypeWithNullability(arrayType, true); +RelDataTypeFactory typeFactory = agg.getCluster().getTypeFactory(); +RelDataType arrayType = typeFactory.createArrayType(arg0Type, -1); +explicitReturnType = typeFactory.createTypeWithNullability(arrayType, true); Suggestion importance[1-10]: 4 __ Why: While caching the typeFactory reference improves readability and may reduce overhead, this is a minor optimization. The suggestion is valid but offers marginal improvement in a code section that's only executed during query planning, not in a hot path.	Low

Suggestions up to commit 71fe3f4

Category	Suggestion	Impact
General	Cache type factory reference Consider caching the type factory in a local variable to avoid multiple method calls. This improves code readability and potentially reduces overhead from repeated getCluster().getTypeFactory() invocations. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [182-183] -RelDataType arrayType = agg.getCluster().getTypeFactory().createArrayType(arg0Type, -1); -explicitReturnType = agg.getCluster().getTypeFactory().createTypeWithNullability(arrayType, true); +RelDataTypeFactory typeFactory = agg.getCluster().getTypeFactory(); +RelDataType arrayType = typeFactory.createArrayType(arg0Type, -1); +explicitReturnType = typeFactory.createTypeWithNullability(arrayType, true); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies an opportunity to reduce redundant method calls by caching getCluster().getTypeFactory(). However, this is a minor optimization that improves readability slightly but has minimal performance impact, as the method calls are lightweight and only executed twice.	Low

Suggestions up to commit 9713eda

Category	Suggestion	Impact
General	Extract type factory to variable Consider extracting the type factory to a local variable to avoid repeated method calls and improve code readability. This reduces duplication and makes the code more maintainable. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PplAggregateCallRewriter.java [182-183] -RelDataType arrayType = agg.getCluster().getTypeFactory().createArrayType(arg0Type, -1); -explicitReturnType = agg.getCluster().getTypeFactory().createTypeWithNullability(arrayType, true); +RelDataTypeFactory typeFactory = agg.getCluster().getTypeFactory(); +RelDataType arrayType = typeFactory.createArrayType(arg0Type, -1); +explicitReturnType = typeFactory.createTypeWithNullability(arrayType, true); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a minor code improvement opportunity by extracting agg.getCluster().getTypeFactory() to a local variable. While this reduces duplication and slightly improves readability, the impact is minimal since the method is only called twice in adjacent lines, making this a low-priority optimization.	Low

[github-actions]

Persistent review updated to latest commit b68dc61

[github-actions]

Persistent review updated to latest commit 71fe3f4

[github-actions]

Persistent review updated to latest commit 5f0b3be

[github-actions]

Persistent review updated to latest commit d486638

[github-actions]

✅ Gradle check result for d486638: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.48%. Comparing base (e5ec89a) to head (97ca5f2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21997      +/-   ##
============================================
+ Coverage     73.45%   73.48%   +0.03%     
- Complexity    75554    75591      +37     
============================================
  Files          6037     6038       +1     
  Lines        342795   342820      +25     
  Branches      49313    49314       +1     
============================================
+ Hits         251797   251935     +138     
+ Misses        71034    70863     -171     
- Partials      19964    20022      +58     

☔ 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.

[github-actions]

Persistent review updated to latest commit 311b3e8

[github-actions]

Persistent review updated to latest commit c8a8250

[github-actions]

Persistent review updated to latest commit 97ca5f2

[github-actions]

✅ Gradle check result for 97ca5f2: SUCCESS