Adding IT tests to cover null and missing values. Improve UnnestOpera…

…tor robustness.

core/src/main/java/org/opensearch/sql/analysis/NestedAnalyzer.java

@@ -16,6 +16,7 @@
 import org.opensearch.sql.ast.expression.UnresolvedExpression;
 import org.opensearch.sql.expression.NamedExpression;
 import org.opensearch.sql.expression.ReferenceExpression;
+import org.opensearch.sql.expression.function.BuiltinFunctionName;
 import org.opensearch.sql.planner.logical.LogicalNested;
 import org.opensearch.sql.planner.logical.LogicalPlan;
 
@@ -41,7 +42,7 @@ public LogicalPlan visitAlias(Alias node, AnalysisContext context) {
 
   @Override
   public LogicalPlan visitFunction(Function node, AnalysisContext context) {
-    if (node.getFuncName().equalsIgnoreCase("nested")) {
+    if (node.getFuncName().equalsIgnoreCase(BuiltinFunctionName.NESTED.name())) {
 
       List<UnresolvedExpression> expressions = node.getFuncArgs();
       ReferenceExpression nestedField =

core/src/main/java/org/opensearch/sql/expression/nested/NestedFunction.java

@@ -33,6 +33,6 @@ public ExprValue valueOf(Environment<Expression, ExprValue> valueEnv) {
 
   @Override
   public ExprType type() {
-    return  this.arguments.get(0).type();
+    return this.arguments.get(0).type();
   }
 }

core/src/main/java/org/opensearch/sql/planner/physical/UnnestOperator.java

@@ -108,6 +108,10 @@ public ExprValue next() {
         result = flatten(nonNestedField, inputValue, result, false);
       }
 
+      if (result.isEmpty()) {
+        return new ExprTupleValue(new LinkedHashMap<>());
+      }
+
       flattenedResult = result.listIterator();
     }
     return new ExprTupleValue(new LinkedHashMap<>(flattenedResult.next()));
@@ -132,16 +136,25 @@ public void generateNonNestedFieldsMap(ExprValue inputMap) {
         ExprValue currentObj = inputField.getValue();
         while (!nestingComplete) {
           if (currentObj instanceof ExprTupleValue) {
-            var it = currentObj.tupleValue().entrySet().iterator().next();
-            currentObj = it.getValue();
-            nonNestedField += "." + it.getKey();
+            var it = currentObj.tupleValue().entrySet().iterator();
+            if (it.hasNext()) {
+              var next = it.next();
+              currentObj = next.getValue();
+              nonNestedField += "." + next.getKey();
+            } else {
+              nestingComplete = true;
+              nonNestedField = null;
+            }
           } else if (currentObj instanceof ExprCollectionValue) {
             currentObj = currentObj.collectionValue().get(0);
           } else {
             nestingComplete = true;
           }
         }
-        this.nonNestedFields.add(nonNestedField);
+
+        if (nonNestedField != null) {
+          this.nonNestedFields.add(nonNestedField);
+        }
       }
     }
   }
@@ -262,7 +275,7 @@ private void getNested(
       } else {
         currentObj = null;
       }
-    } else {
+    } else if (currentObj instanceof ExprCollectionValue) {
       ExprValue arrayObj = currentObj;
       if (supportArrays) {
         for (int x = 0; x < arrayObj.collectionValue().size(); x++) {
@@ -275,6 +288,8 @@ private void getNested(
         getNested(field, nestedField, row, ret, currentObj, supportArrays);
         currentObj = null;
       }
+    } else {
+      currentObj = null;
     }
 
     // Return final nested result

core/src/test/java/org/opensearch/sql/planner/physical/UnnestOperatorTest.java

@@ -7,6 +7,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.when;
 import static org.opensearch.sql.data.model.ExprValueUtils.collectionValue;
 import static org.opensearch.sql.data.model.ExprValueUtils.tupleValue;
@@ -72,6 +73,24 @@ class UnnestOperatorTest extends PhysicalPlanTestBase {
       )
   );
 
+  private final ExprValue missingTupleData = tupleValue(
+      ImmutableMap.of(
+          "tuple",
+          tupleValue(
+              ImmutableMap.of()
+          )
+      )
+  );
+
+  private final ExprValue missingArrayData = tupleValue(
+      ImmutableMap.of(
+          "missing",
+          collectionValue(
+              List.of("value")
+          )
+      )
+  );
+
   @Test
   public void nested_one_nested_field() {
     when(inputPlan.hasNext()).thenReturn(true, false);
@@ -165,4 +184,36 @@ public void non_nested_field_tests() {
         )
     );
   }
+
+  @Test
+  public void nested_missing_tuple_field() {
+    when(inputPlan.hasNext()).thenReturn(true, false);
+    when(inputPlan.next())
+        .thenReturn(missingTupleData);
+    Set<String> fields = Set.of("message.val");
+    Map<String, List<String>> groupedFieldsByPath =
+        Map.of("message", List.of("message.val"));
+    assertTrue(
+        execute(new UnnestOperator(inputPlan, fields, groupedFieldsByPath))
+            .get(0)
+            .tupleValue()
+            .size() == 0
+    );
+  }
+
+  @Test
+  public void nested_missing_array_field() {
+    when(inputPlan.hasNext()).thenReturn(true, false);
+    when(inputPlan.next())
+        .thenReturn(missingArrayData);
+    Set<String> fields = Set.of("missing.data");
+    Map<String, List<String>> groupedFieldsByPath =
+        Map.of("message", List.of("message.data"));
+    assertTrue(
+        execute(new UnnestOperator(inputPlan, fields, groupedFieldsByPath))
+            .get(0)
+            .tupleValue()
+            .size() == 0
+    );
+  }
 }

docs/user/dql/functions.rst

@@ -4300,17 +4300,16 @@ Another example to show how to set custom values for the optional parameters::
     +-------------------------------------------+
 
 NESTED
-------------
+------
 
 Description
 >>>>>>>>>>>
 
-``nested(field | [field, path] | [path, condition])``
+``nested(field | [field, path])``
 
 The ``nested`` function maps to the ``nested`` query used in search engine. It returns nested field types in documents that match the provided specified field(s).
-The ``field`` and ``path`` parameters are valid parameters for a nested function used in the ``SELECT`` clause.
 
-Example with only ``field`` and ``path`` parameters::
+Example with ``field`` and ``path`` parameters::
 
     os> SELECT nested(message.info, message) FROM nested;
     fetched rows / total rows = 2/2

integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java

@@ -601,10 +601,14 @@ public enum Index {
         "datasource",
         getMappingFile("datasources_index_mappings.json"),
         "src/test/resources/datasources.json"),
-    MULTI_NESTED(TestsConstants.TEST_INDEX_MULTI_NESTED,
+    MULTI_NESTED(TestsConstants.TEST_INDEX_MULTI_NESTED_TYPE,
         "multi_nested",
         getMappingFile("multi_nested.json"),
-        "src/test/resources/multi_nested_objects.json");
+        "src/test/resources/multi_nested_objects.json"),
+    NESTED_WITH_NULLS(TestsConstants.TEST_INDEX_NESTED_WITH_NULLS,
+        "multi_nested",
+        getNestedTypeIndexMapping(),
+        "src/test/resources/nested_with_nulls.json");
 
     private final String name;
     private final String type;

integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java

@@ -58,7 +58,7 @@ public class TestsConstants {
   public final static String TEST_INDEX_CALCS = TEST_INDEX + "_calcs";
   public final static String TEST_INDEX_WILDCARD = TEST_INDEX + "_wildcard";
   public final static String TEST_INDEX_MULTI_NESTED_TYPE = TEST_INDEX + "_multi_nested";
-  public final static String TEST_INDEX_MULTI_NESTED= TEST_INDEX + "_multi_nested";
+  public final static String TEST_INDEX_NESTED_WITH_NULLS = TEST_INDEX + "_nested_with_nulls";
   public final static String DATASOURCES = ".ql-datasources";
 
   public final static String DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";

integ-test/src/test/java/org/opensearch/sql/sql/NestedIT.java

@@ -5,10 +5,10 @@
 
 package org.opensearch.sql.sql;
 
-import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_EMPLOYEE_NESTED;
 import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_MULTI_NESTED_TYPE;
 import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_NESTED_TYPE;
 import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_NESTED_TYPE_WITHOUT_ARRAYS;
+import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_NESTED_WITH_NULLS;
 import static org.opensearch.sql.util.MatcherUtils.rows;
 import static org.opensearch.sql.util.MatcherUtils.schema;
 import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;
@@ -20,6 +20,7 @@
 import org.json.JSONObject;
 import org.junit.Test;
 import org.junit.jupiter.api.Disabled;
+import org.opensearch.sql.data.model.ExprNullValue;
 import org.opensearch.sql.legacy.SQLIntegTestCase;
 
 public class NestedIT extends SQLIntegTestCase {
@@ -29,6 +30,7 @@ public void init() throws IOException {
     loadIndex(Index.NESTED);
     loadIndex(Index.NESTED_WITHOUT_ARRAYS);
     loadIndex(Index.EMPLOYEE_NESTED);
+    loadIndex(Index.NESTED_WITH_NULLS);
   }
 
   @Test
@@ -115,6 +117,26 @@ public void nested_function_with_array_of_multi_nested_field_test() {
         rows("yy"));
   }
 
+  @Test
+  public void nested_function_with_null_and_missing_fields_test() {
+    String query = "SELECT nested(message.info), nested(comment.data) FROM "
+        + TEST_INDEX_NESTED_WITH_NULLS;
+    JSONObject result = executeJdbcRequest(query);
+
+    assertEquals(10, result.getInt("total"));
+    verifyDataRows(result,
+        rows(null, "hh"),
+        rows("b", "aa"),
+        rows("c", "aa"),
+        rows("c", "ab"),
+        rows("a", "ab"),
+        rows("zz", new JSONArray(List.of("aa", "bb"))),
+        rows("zz", new JSONArray(List.of("aa", "bb"))),
+        rows(null, "ee"),
+        rows("a", "ab"),
+        rows("rr", new JSONArray(List.of("asdf", "sdfg"))));
+  }
+
   @Test
   public void nested_function_multiple_fields_with_matched_and_mismatched_paths_test() {
     String query =

integ-test/src/test/resources/nested_with_nulls.json

@@ -0,0 +1,24 @@
+{"index":{"_id":"1"}}
+{"message":{"author":"e","dayOfWeek":5},"comment":{"data":"hh","likes":5},"myNum":7,"someField":"a"}
+{"index":{"_id":"2"}}
+{"message":{"info":"b","author":"f","dayOfWeek":2},"comment":{"data":"aa","likes":2},"myNum":2,"someField":"a"}
+{"index":{"_id":"3"}}
+{"message":{"info":"c","author":"g","dayOfWeek":1},"comment":{"data":"aa","likes":3},"myNum":3,"someField":"a"}
+{"index":{"_id":"4"}}
+{"message":[{"info":"c","author":"h","dayOfWeek":4},{"info":"a","author":"i","dayOfWeek":5}],"comment":{"data":"ab","likes":1},"myNum":4,"someField":"b"}
+{"index":{"_id":"5"}}
+{"message": [{"info":"zz","author":"zz","dayOfWeek":6}],"comment":{"data":["aa","bb"],"likes":10},"myNum":[3,4],"someField":"a"}
+{"index":{"_id":"7"}}
+{"message":[{"info":"zz", "author":"z\"z", "dayOfWeek":6}], "comment":{"data":["aa","bb"], "likes":10}, "myNum":[3,4], "someField":"a"}
+{"index":{"_id":"8"}}
+{"message":{"info":null,"author":"e","dayOfWeek":7},"comment":{"data":"ee","likes":6},"myNum":6,"someField":"a"}
+{"index":{"_id":"9"}}
+{"message":{"info":"a","author":"e","dayOfWeek":1},"comment":{"data":"ab","likes":3},"myNum":1,"someField":"b"}
+{"index":{"_id":"10"}}
+{"message":[{"info":"rr", "author":"this \"value\" contains quotes", "dayOfWeek":3}], "comment":{"data":["asdf","sdfg"], "likes":56}, "myNum":[1,2,4], "someField":"ert"}
+{"index":{"_id":"11"}}
+{"comment":{"data":"jj","likes":1},"myNum":8,"someField":"a"}
+{"index":{"_id":"12"}}
+{"message":null,"comment":{"data":"kk","likes":0},"myNum":9,"someField":"a"}
+{"index":{"_id":"13"}}
+{}