[tanvir4se]

Description

mvjoin, and nomv (which rewrites to mvjoin), returned HTTP 500 while running the query when the array's elements were not strings, for example mvjoin(array(n), ',') on a number field. They now return HTTP 400 while analyzing, as the mvjoin docs already say ("Only string arrays are supported").

Root cause. MVJOIN is registered as Calcite's ARRAY_JOIN. The PPL wrapper around its type checker compares only the operand count and each operand's family, so ARRAY<BIGINT> passed as ARRAY, and SqlFunctions.arrayToString threw IllegalStateException at run time.

Fix. The MVJOIN implementation checks the array's element type before building the call. Strings and the string-backed date, time and timestamp types pass. Other known element types throw, which resolve() reports as an ExpressionEvaluationException (400). The check sits in the implementation, not the type checker, because the coercion fallback sees ARRAY<BIGINT> and ARRAY<ANY> as the same ARRAY and would build the call anyway.

Behaviour change. nomv on an array of a number field whose values are all null returned 200 with "" and now returns 400, because the element type decides. The affected tests now cast to string.

Known limitation. Arrays typed ARRAY<ANY>, from a bare nomv on a nested field or a mixed mvappend('a', n), are still accepted during analysis and still return 500 at run time. No follow-up issue is filed yet.

Before and after

Query (source=mv5898)	Before	After
mvjoin(array(k)), nomv on array(k) (keyword)	200	200
same for t (text)	200	200
same for d (date), result 2024-01-01 00:00:00	200	200
same for i (integer), l (long), db (double), bo (boolean)	500 executing	400 analyzing
same for ip, gp (geo_point), o (object), ne (nested)	500 executing	400 analyzing
mvjoin(l, ',') (scalar)	400, MVJOIN function expects ...	400, message unchanged
mvjoin(array('a', l), ',')	200 a,3	200 a,3
mvjoin(array(cast(l as string)), ',')	200 3	200 3
nomv ne (ARRAY, known limitation)	500	500
mvjoin(mvappend('a', l), ',') (ARRAY, known limitation)	500	500

Example after-fix message: Cannot resolve function: MVJOIN, arguments: [ARRAY,STRING], caused by: MVJOIN only supports string arrays, but got an array of LONG.

Open questions

These are still open on the issue. The PR uses the defaults below.

1. 400 rather than converting to text. Default: 400, matching the docs, the reporter, and the earlier mvjoin work. @ahkcs, please say if you would rather convert elements to text.
2. Where the check lives, and the message wording. Default: the check is in the MVJOIN implementation, with MVJOIN only supports string arrays, but got an array of <TYPE>. Happy to move it to a custom type checker, or to change the wording.
3. ARRAY<ANY> stays out of scope. Default: yes. Bare nomv on a nested field and mvjoin(mvappend('a', n), ',') still return 500. I can open a follow-up issue.

Related Issues

Resolves #5898

Check List

• New functionality includes testing.
• New functionality has been documented.
• Commits are signed per the DCO using --signoff or -s.

Could a maintainer please add the bugFix label?

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 🔍

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 Incomplete validation The element type check accepts SqlTypeName.ANY and SqlTypeName.NULL, but the PR description notes that arrays typed ARRAY<ANY> still cause a 500 at run time. This means the validation is incomplete for a known case. If ANY is accepted here but fails later, either reject it now or document why it must pass through. private static void requireStringArrayElements(final RelDataType arrayType) { final RelDataType elementType = arrayType.getComponentType(); if (elementType == null || SqlTypeUtil.isCharacter(elementType) || elementType.getSqlTypeName() == SqlTypeName.ANY || elementType.getSqlTypeName() == SqlTypeName.NULL) { return;

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null check for elementType.getSqlTypeName() before comparing it to SqlTypeName.ANY or SqlTypeName.NULL. If elementType is non-null but getSqlTypeName() returns null, the current code will throw a NullPointerException instead of the intended IllegalArgumentException. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [920-932] private static void requireStringArrayElements(final RelDataType arrayType) { final RelDataType elementType = arrayType.getComponentType(); if (elementType == null - || SqlTypeUtil.isCharacter(elementType) - || elementType.getSqlTypeName() == SqlTypeName.ANY - || elementType.getSqlTypeName() == SqlTypeName.NULL) { + || SqlTypeUtil.isCharacter(elementType)) { + return; + } + final SqlTypeName typeName = elementType.getSqlTypeName(); + if (typeName == null || typeName == SqlTypeName.ANY || typeName == SqlTypeName.NULL) { return; } throw new IllegalArgumentException( String.format( "MVJOIN only supports string arrays, but got an array of %s", OpenSearchTypeFactory.convertRelDataTypeToExprType(elementType))); } Suggestion importance[1-10]: 3 __ Why: While adding a null check for getSqlTypeName() is technically safer, it's unlikely that getSqlTypeName() would return null for a non-null elementType in the Calcite framework. The suggestion addresses a theoretical edge case but doesn't fix a critical bug or significantly improve the code's robustness in practice.	Low

[tanvir4se]

Decision Log

Root cause. MVJOIN is registered as Calcite's ARRAY_JOIN. PPLFamilyTypeCheckerWrapper.checkOperandTypes checks the operand count and each operand's type family. ARRAY<BIGINT> passes that check as ARRAY. Calcite's own ARRAY_JOIN checker does check the element type, but only in its SqlCallBinding overload, which the PPL wrapper does not call. At run time SqlFunctions.arrayToString throws IllegalStateException, and the response is HTTP 500.

Approach. The MVJOIN implementation calls requireStringArrayElements before builder.makeCall. Character element types pass. Date, time, timestamp and binary types report VARCHAR, so they pass too. ANY and NULL pass. Any other known element type throws IllegalArgumentException. resolve() wraps that as ExpressionEvaluationException, so the query returns HTTP 400 during analysis. The type checker is unchanged, so mvjoin(42, ',') keeps its current message.

Alternatives rejected. A custom PPLTypeChecker that checks the element type does not run when resolveWithCoercion builds the call, unless getParameterTypes() returns an empty list. The message would still say the call expected [ARRAY,STRING] and got [ARRAY,STRING], because PlanUtils.getActualSignature drops element types.

Calling Calcite's full checkOperandTypes from PPLFamilyTypeCheckerWrapper would change every operator that uses that wrapper. It also needs a synthetic SqlCallBinding.

Mapping the run-time IllegalStateException to HTTP 400 in SyncErrorReportRenderer would still fail the query during execution. The status would depend on the text of a Calcite exception.

RFC #4146 describes converting the elements to text. The mvjoin docs say only string arrays are supported, and @cnoramut suggested returning 400.

Pitfalls. When a type checker rejects the call, resolveWithCoercion uses CoercionUtils.castArguments. It compares ExprType values from convertRelDataTypeToExprType, which drops the element type. ARRAY<BIGINT> and ARRAY<VARCHAR> both look like ARRAY, so no cast is added and the call is built anyway.

Date, time and timestamp user-defined types report VARCHAR, so SqlTypeUtil.isCharacter accepts them.

nomv on a missing field reaches mvjoin as ARRAY<ANY>, so the guard has to accept ANY.

Things to watch. ARRAY<ANY> still reaches run time. A bare nomv on a nested field, and mvjoin(mvappend('a', n), ','), still return 500.

On the analytics-engine route, date and date_nanos fields are Calcite TIMESTAMP types (plain, or the DateOnlyType and TimeOnlyType markers), not the VARCHAR-backed types. SqlTypeUtil.isCharacter rejects them, so mvjoin on an array of dates may return 400 on that route.

nomv on an all-null number array returned 200 and now returns 400.

[tanvir4se]

PR Reviewer Guide 🔍

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

Incomplete validation
The element type check accepts SqlTypeName.ANY and SqlTypeName.NULL, but the PR description notes that arrays typed ARRAY<ANY> still cause a 500 at run time. This means the validation is incomplete for a known case. If ANY is accepted here but fails later, either reject it now or document why it must pass through.

private static void requireStringArrayElements(final RelDataType arrayType) {
  final RelDataType elementType = arrayType.getComponentType();
  if (elementType == null
      || SqlTypeUtil.isCharacter(elementType)
      || elementType.getSqlTypeName() == SqlTypeName.ANY
      || elementType.getSqlTypeName() == SqlTypeName.NULL) {
    return;

I am keeping ANY accepted in this pull request.

ANY means the element type is unknown during analysis. The guard returns 400 only when the element type is known and is not a string. Rejecting ANY would also break nomv on a missing field. array_compact on the missing field has type ARRAY<ANY>, and that query returns 200 today (testNoMvMissingField).

A bare nomv on a nested field, and mvjoin(mvappend('a', n), ','), are also ARRAY<ANY>, and they still return 500 at run time. That limitation is in the description, and I can open a follow-up issue for it.

[tanvir4se]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category **Suggestion                                                                                                                                    ** Impact
Possible issue
Add null safety check
Add null check for elementType.getSqlTypeName() before comparing it to SqlTypeName.ANY or SqlTypeName.NULL. If elementType is non-null but getSqlTypeName() returns null, the current code will throw a NullPointerException instead of the intended IllegalArgumentException.

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [920-932]

 private static void requireStringArrayElements(final RelDataType arrayType) {
   final RelDataType elementType = arrayType.getComponentType();
   if (elementType == null
-      || SqlTypeUtil.isCharacter(elementType)
-      || elementType.getSqlTypeName() == SqlTypeName.ANY
-      || elementType.getSqlTypeName() == SqlTypeName.NULL) {
+      || SqlTypeUtil.isCharacter(elementType)) {
+    return;
+  }
+  final SqlTypeName typeName = elementType.getSqlTypeName();
+  if (typeName == null || typeName == SqlTypeName.ANY || typeName == SqlTypeName.NULL) {
     return;
   }
   throw new IllegalArgumentException(
       String.format(
           "MVJOIN only supports string arrays, but got an array of %s",
           OpenSearchTypeFactory.convertRelDataTypeToExprType(elementType)));
 }

Suggestion importance[1-10]: 3
__

Why: While adding a null check for getSqlTypeName() is technically safer, it's unlikely that getSqlTypeName() would return null for a non-null elementType in the Calcite framework. The suggestion addresses a theoretical edge case but doesn't fix a critical bug or significantly improve the code's robustness in practice.

Low

I am leaving this check as it is.

Calcite documents RelDataType.getSqlTypeName() as never null, and the OpenSearch types return the type name of the Calcite type they wrap. A null type name also would not throw at these comparisons. SqlTypeUtil.isCharacter returns false for a null type name, and comparing null with == is false. If building the message did throw, resolve() catches it and reports ExpressionEvaluationException, so the response would still be 400.

The suggested change would let an element with a null type name pass analysis instead of rejecting it.

[tanvir4se]

@ahkcs @Swiddis, could one of you please approve workflows on this draft pull request? It also still needs the bugFix label. Thanks