[BugFix] Return 400 for mvjoin and nomv on arrays of non-string elements (#5898)

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java

@@ -320,6 +320,7 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.opensearch.sql.calcite.CalcitePlanContext;
+import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
 import org.opensearch.sql.calcite.utils.PPLOperandTypes;
 import org.opensearch.sql.calcite.utils.PlanUtils;
 import org.opensearch.sql.calcite.utils.UserDefinedFunctionUtils;
@@ -909,6 +910,27 @@ private static RexNode[] widenIntegerOperands(RexBuilder builder, RexNode... arg
       return widened;
     }
 
+    /**
+     * Rejects arrays whose element type is known and is not a string. Character types (which
+     * include the VARCHAR-backed date, time and timestamp types), {@code ANY} and {@code NULL}
+     * element types are accepted.
+     *
+     * @throws IllegalArgumentException so that the caller reports a client error (400)
+     */
+    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;
+      }
+      throw new IllegalArgumentException(
+          String.format(
+              "MVJOIN only supports string arrays, but got an array of %s",
+              OpenSearchTypeFactory.convertRelDataTypeToExprType(elementType)));
+    }
+
     private static boolean referencesLambdaParameter(RexNode node) {
       if (node instanceof RexLambdaRef) {
         return true;
@@ -1247,8 +1269,19 @@ void populate() {
                   builder.makeCast(TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, true), source),
           PPLTypeChecker.family(SqlTypeFamily.ANY));
 
-      // Register MVJOIN to use Calcite's ARRAY_JOIN
-      registerOperator(MVJOIN, SqlLibraryOperators.ARRAY_JOIN);
+      // Register MVJOIN to use Calcite's ARRAY_JOIN. The family type checker compares only the
+      // top-level ARRAY family, so the element type is checked here, during analysis, instead of
+      // failing at run time inside SqlFunctions.arrayToString.
+      register(
+          MVJOIN,
+          (final RexBuilder builder, final RexNode... args) -> {
+            requireStringArrayElements(args[0].getType());
+            return builder.makeCall(SqlLibraryOperators.ARRAY_JOIN, args);
+          },
+          wrapSqlOperandTypeChecker(
+              SqlLibraryOperators.ARRAY_JOIN.getOperandTypeChecker(),
+              SqlLibraryOperators.ARRAY_JOIN.getName(),
+              false));
 
       // Register SPLIT with custom logic for empty delimiter
       // Case 1: Delimiter is not empty string, use SPLIT

docs/user/ppl/cmd/nomv.md

@@ -57,6 +57,7 @@ The `nomv` command has the following limitations:
 - The `nomv` command is only available when the Apache Calcite query engine is enabled.
 - The newline delimiter (`\n`) is fixed and cannot be customized. For custom delimiters, use the [`mvjoin`](../functions/collection.md/#mvjoin) function directly in an [`eval`](./eval.md) expression.
 - `NULL` values within the array are automatically filtered out and do not appear in the output.
+- The array elements must be strings. Arrays with elements of other types are rejected with an error, so cast the elements to strings first (for example, with `cast(... as string)`).
 
 ## Related commands
 

docs/user/ppl/functions/collection.md

@@ -307,7 +307,7 @@ fetched rows / total rows = 1/1
 
 **Usage**: `mvjoin(array, delimiter)`
 
-Joins string array elements into a single string, separated by the specified delimiter. `NULL` elements are excluded from the output. Only string arrays are supported.
+Joins string array elements into a single string, separated by the specified delimiter. `NULL` elements are excluded from the output. Only string arrays are supported: arrays whose elements are not strings are rejected with an error. To join other types, cast the elements to strings first, for example with `cast(... as string)`.
 
 **Parameters**:
 
@@ -358,6 +358,26 @@ fetched rows / total rows = 1/1
 | Amber, Duke |
 +-------------+
 ```
+
+The following example casts numbers to strings before joining them:
+
+```ppl
+source=people
+| eval result = mvjoin(array(cast(1 as string), cast(2 as string), cast(3 as string)), ',')
+| fields result
+| head 1
+```
+
+The query returns the following results:
+
+```text
+fetched rows / total rows = 1/1
++--------+
+| result |
+|--------|
+| 1,2,3  |
++--------+
+```
 
 ## MVAPPEND
 

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteArrayFunctionIT.java

@@ -433,6 +433,71 @@ public void testMvjoinWithMultipleRealFields() throws IOException {
         firstRow.getString(3));
   }
 
+  @Test
+  public void testMvjoinRejectsIntegerArray() {
+    verifyMvjoinRejectsNonStringArray("array(age)", "INTEGER");
+  }
+
+  @Test
+  public void testMvjoinRejectsLongArray() {
+    verifyMvjoinRejectsNonStringArray("array(balance)", "LONG");
+  }
+
+  @Test
+  public void testMvjoinRejectsBooleanArray() {
+    verifyMvjoinRejectsNonStringArray("array(male)", "BOOLEAN");
+  }
+
+  @Test
+  public void testMvjoinAcceptsDateArray() throws IOException {
+    // birthdate is a date field. Date, time and timestamp values are string-backed, so they stay
+    // accepted. The rendered value is only checked by its date part to stay format-agnostic.
+    final JSONObject actual =
+        executeQuery(
+            String.format(
+                "source=%s | where account_number = 1 | eval result = mvjoin(array(birthdate), ',')"
+                    + " | fields result",
+                TEST_INDEX_BANK));
+
+    verifySchema(actual, schema("result", "string"));
+    final JSONArray dataRows = actual.getJSONArray("datarows");
+    assertEquals(1, dataRows.length());
+    assertTrue(dataRows.getJSONArray(0).getString(0).startsWith("2017-10-23"));
+  }
+
+  @Test
+  public void testMvjoinAcceptsCastNumberToStringArray() throws IOException {
+    final JSONObject actual =
+        executeQuery(
+            String.format(
+                "source=%s | where account_number = 1 | eval result = mvjoin(array(cast(age as"
+                    + " string)), ',') | fields result",
+                TEST_INDEX_BANK));
+
+    verifySchema(actual, schema("result", "string"));
+    verifyDataRows(actual, rows("32"));
+  }
+
+  private void verifyMvjoinRejectsNonStringArray(
+      final String arrayExpression, final String expectedElementType) {
+    final Class<? extends Exception> expectedException =
+        isStandaloneTest() ? RuntimeException.class : ResponseException.class;
+    final Exception e =
+        assertThrows(
+            expectedException,
+            () ->
+                executeQuery(
+                    String.format(
+                        "source=%s | eval result = mvjoin(%s, ',') | fields result | head 1",
+                        TEST_INDEX_BANK, arrayExpression)));
+
+    verifyErrorMessageContains(
+        e, "MVJOIN only supports string arrays, but got an array of " + expectedElementType);
+    if (e instanceof ResponseException) {
+      assertEquals(400, ((ResponseException) e).getResponse().getStatusLine().getStatusCode());
+    }
+  }
+
   @Test
   public void testMvindexSingleElementPositive() throws IOException {
     JSONObject actual =

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteNoMvCommandIT.java

@@ -255,13 +255,14 @@ public void testNoMvWithNullAtBeginningAndEnd() throws IOException {
 
   @Test
   public void testNoMvWithAllNulls() throws IOException {
-    String q =
+    // age is an integer field, so it is cast to string first: nomv rejects numeric arrays.
+    final String q =
         "source="
             + TEST_INDEX_BANK_WITH_NULL_VALUES
-            + " | where account_number = 25 | eval arr = array(age, age, age) | nomv arr | fields"
-            + " account_number, arr";
+            + " | where account_number = 25 | eval age_str = cast(age as string) | eval arr ="
+            + " array(age_str, age_str, age_str) | nomv arr | fields account_number, arr";
 
-    JSONObject result = executeQuery(q);
+    final JSONObject result = executeQuery(q);
 
     verifySchema(result, schema("account_number", null, "bigint"), schema("arr", null, "string"));
 
@@ -270,19 +271,41 @@ public void testNoMvWithAllNulls() throws IOException {
 
   @Test
   public void testNoMvArrayWithAllNulls() throws IOException {
-    String q =
+    // age is an integer field, so it is cast to string first: nomv rejects numeric arrays.
+    final String q =
         "source="
             + TEST_INDEX_BANK_WITH_NULL_VALUES
-            + " | where account_number = 25 | eval arr = array(age, age, age) | nomv arr | fields"
-            + " account_number, arr";
+            + " | where account_number = 25 | eval arr = array(cast(age as string), cast(age as"
+            + " string), cast(age as string)) | nomv arr | fields account_number, arr";
 
-    JSONObject result = executeQuery(q);
+    final JSONObject result = executeQuery(q);
 
     verifySchema(result, schema("account_number", null, "bigint"), schema("arr", null, "string"));
 
     verifyDataRows(result, rows(25, ""));
   }
 
+  @Test
+  public void testNoMvNumericArrayError() {
+    final ResponseException ex =
+        Assertions.assertThrows(
+            ResponseException.class,
+            () ->
+                executeQuery(
+                    "source="
+                        + TEST_INDEX_BANK
+                        + " | eval arr = array(age) | nomv arr | head 1 | fields arr"));
+
+    final int status = ex.getResponse().getStatusLine().getStatusCode();
+    Assertions.assertEquals(400, status, "Expected 400 for an array of non-string elements");
+
+    final String msg = ex.getMessage();
+    Assertions.assertTrue(
+        msg.contains("MVJOIN only supports string arrays, but got an array of INTEGER"), msg);
+    Assertions.assertTrue(msg.contains("ExpressionEvaluationException"), msg);
+    Assertions.assertTrue(msg.contains("analyzing"), msg);
+  }
+
   @Test
   public void testNoMvMultipleRowsRowLocalBehavior() throws IOException {
     String q =