Cleanup, modify to return null for an empty array.

Signed-off-by: currantw <[email protected]>
Bit-Quill · Feb 6, 2025 · 8e74556 · 8e74556
1 parent 9fd0635
commit 8e74556
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 26 deletions.
diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java
@@ -450,10 +450,7 @@ public LogicalPlan visitEval(Eval node, AnalysisContext context) {
     return new LogicalEval(child, expressionsBuilder.build());
   }
 
-  /**
-   * Builds and returns a {@link org.opensearch.sql.planner.logical.logicalExpand} corresponding to
-   * the given expand node.
-   */
+  /** Builds and returns a {@link LogicalExpand} corresponding to the given expand node. */
   @Override
   public LogicalPlan visitExpand(Expand node, AnalysisContext context) {
     LogicalPlan child = node.getChild().getFirst().accept(this, context);

diff --git a/core/src/main/java/org/opensearch/sql/ast/tree/Expand.java b/core/src/main/java/org/opensearch/sql/ast/tree/Expand.java
@@ -13,13 +13,13 @@
 import org.opensearch.sql.ast.AbstractNodeVisitor;
 import org.opensearch.sql.ast.expression.Field;
 
-/** AST node representing an expand <field> operation. */
+/** AST node representing an {@code expand <field>} operation. */
 @Getter
 @ToString
 @RequiredArgsConstructor
 public class Expand extends UnresolvedPlan {
-  private UnresolvedPlan child;
 
+  private UnresolvedPlan child;
   @Getter private final Field field;
 
   @Override

diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/ExpandOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/ExpandOperator.java
@@ -16,6 +16,7 @@
 import lombok.RequiredArgsConstructor;
 import lombok.ToString;
 import org.opensearch.sql.data.model.ExprValue;
+import org.opensearch.sql.data.model.ExprValueUtils;
 import org.opensearch.sql.expression.ReferenceExpression;
 import org.opensearch.sql.utils.PathUtils;
 
@@ -67,7 +68,12 @@ private static List<ExprValue> expandExprValueAtPath(ExprValue rootExprValue, St
       return new LinkedList<>(Collections.singletonList(rootExprValue));
     }
 
-    return targetExprValue.collectionValue().stream()
+    List<ExprValue> expandedExprValues = targetExprValue.collectionValue();
+    if (expandedExprValues.isEmpty()) {
+      expandedExprValues = List.of(ExprValueUtils.nullValue());
+    }
+
+    return expandedExprValues.stream()
         .map(v -> PathUtils.setExprValueAtPath(rootExprValue, path, v))
         .collect(Collectors.toCollection(LinkedList::new));
   }

diff --git a/core/src/main/java/org/opensearch/sql/utils/PathUtils.java b/core/src/main/java/org/opensearch/sql/utils/PathUtils.java
@@ -17,6 +17,7 @@
 import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.exception.SemanticCheckException;
 
+/** Utility methods for handling {@link ExprValue} paths. */
 @UtilityClass
 public class PathUtils {
 
@@ -34,8 +35,8 @@ public boolean containsExprValueAtPath(ExprValue root, String path) {
    * PathUtils#containsExprValueAtPath}.
    */
   public ExprValue getExprValueAtPath(ExprValue root, String path) {
-    List<String> pathComponents = splitPath(path);
 
+    List<String> pathComponents = splitPath(path);
     if (!containsExprValueForPathComponents(root, pathComponents)) {
       return null;
     }
@@ -49,8 +50,8 @@ public ExprValue getExprValueAtPath(ExprValue root, String path) {
    * {@link PathUtils#containsExprValueAtPath}.
    */
   public ExprValue setExprValueAtPath(ExprValue root, String path, ExprValue newValue) {
-    List<String> pathComponents = splitPath(path);
 
+    List<String> pathComponents = splitPath(path);
     if (!containsExprValueForPathComponents(root, pathComponents)) {
       throw new SemanticCheckException(String.format("Field path '%s' does not exist.", path));
     }

diff --git a/core/src/test/java/org/opensearch/sql/planner/logical/LogicalExpandTest.java b/core/src/test/java/org/opensearch/sql/planner/logical/LogicalExpandTest.java
@@ -8,6 +8,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.opensearch.sql.data.type.ExprCoreType.ARRAY;
+import static org.opensearch.sql.data.type.ExprCoreType.INTEGER;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -23,20 +24,29 @@ class LogicalExpandTest extends AnalyzerTestBase {
 
   private static final String TABLE_NAME = "schema";
 
+  @Test
+  void testExpandScalar() {
+    LogicalPlan expected =
+        LogicalPlanDSL.expand(
+            LogicalPlanDSL.relation(TABLE_NAME, table), DSL.ref("integer_value", INTEGER));
+    LogicalPlan actual =
+        analyze(AstDSL.expand(AstDSL.relation(TABLE_NAME), AstDSL.field("integer_value")));
+    assertEquals(expected, actual);
+  }
+
   @Test
   void testExpandArray() {
     LogicalPlan expected =
         LogicalPlanDSL.expand(
             LogicalPlanDSL.relation(TABLE_NAME, table), DSL.ref("array_value", ARRAY));
-    UnresolvedPlan unresolved =
-        AstDSL.expand(AstDSL.relation(TABLE_NAME), AstDSL.field("array_value"));
-    assertEquals(expected, analyze(unresolved));
+    LogicalPlan actual =
+        analyze(AstDSL.expand(AstDSL.relation(TABLE_NAME), AstDSL.field("array_value")));
+    assertEquals(expected, actual);
   }
 
   @Test
   void testExpandInvalidFieldName() {
     UnresolvedPlan unresolved = AstDSL.expand(AstDSL.relation(TABLE_NAME), AstDSL.field("invalid"));
-
     String msg = assertThrows(SemanticCheckException.class, () -> analyze(unresolved)).getMessage();
     assertEquals("can't resolve Symbol(namespace=FIELD_NAME, name=invalid) in type env", msg);
   }

diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/ExpandOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/ExpandOperatorTest.java
@@ -66,7 +66,8 @@ void testArrayEmpty() {
     mockInput(inputRow);
 
     actualRows = execute(expand(inputPlan, DSL.ref("array_empty", ARRAY)));
-    expectedRows = List.of();
+    expectedRows =
+        List.of(ExprValueUtils.tupleValue(Map.of("array_empty", ExprValueUtils.nullValue())));
 
     assertEquals(expectedRows, actualRows);
   }

diff --git a/core/src/test/java/org/opensearch/sql/utils/PathUtilsTest.java b/core/src/test/java/org/opensearch/sql/utils/PathUtilsTest.java
@@ -42,31 +42,31 @@ class PathUtilsTest {
               Map.entry("struct_missing", missingValue)));
 
   @Test
-  void testContainsExprValueForPathComponents() {
+  void testContainsExprValueForPath() {
     assertTrue(PathUtils.containsExprValueAtPath(input, "field"));
     assertTrue(PathUtils.containsExprValueAtPath(input, "struct1.field"));
     assertTrue(PathUtils.containsExprValueAtPath(input, "struct2.struct1.field"));
 
     assertFalse(PathUtils.containsExprValueAtPath(input, "field_invalid"));
-    assertFalse(PathUtils.containsExprValueAtPath(input, "struct_null.field"));
-    assertFalse(PathUtils.containsExprValueAtPath(input, "struct_missing.field"));
-    assertFalse(PathUtils.containsExprValueAtPath(input, "field.field"));
+    assertFalse(PathUtils.containsExprValueAtPath(input, "struct_null.field_invalid"));
+    assertFalse(PathUtils.containsExprValueAtPath(input, "struct_missing.field_invalid"));
+    assertFalse(PathUtils.containsExprValueAtPath(input, "struct_invalid.field_invalid"));
   }
 
   @Test
-  void testGetExprValueForPathComponents() {
+  void testGetExprValueForPath() {
     assertEquals(value, PathUtils.getExprValueAtPath(input, "field"));
     assertEquals(value, PathUtils.getExprValueAtPath(input, "struct1.field"));
     assertEquals(value, PathUtils.getExprValueAtPath(input, "struct2.struct1.field"));
 
     assertNull(PathUtils.getExprValueAtPath(input, "field_invalid"));
-    assertNull(PathUtils.getExprValueAtPath(input, "struct_null.field"));
-    assertNull(PathUtils.getExprValueAtPath(input, "struct_missing.field"));
-    assertNull(PathUtils.getExprValueAtPath(input, "field.field"));
+    assertNull(PathUtils.getExprValueAtPath(input, "struct_null.field_invalid"));
+    assertNull(PathUtils.getExprValueAtPath(input, "struct_missing.field_invalid"));
+    assertNull(PathUtils.getExprValueAtPath(input, "struct_invalid.field_invalid"));
   }
 
   @Test
-  void testSetExprValueForPathComponents() {
+  void testSetExprValueForPath() {
     ExprValue expected;
     ExprValue actual;
 
@@ -128,7 +128,7 @@ void testSetExprValueForPathComponents() {
     ex =
         assertThrows(
             SemanticCheckException.class,
-            () -> PathUtils.setExprValueAtPath(input, "field.field_invalid", newValue));
-    assertEquals("Field path 'field.field_invalid' does not exist.", ex.getMessage());
+            () -> PathUtils.setExprValueAtPath(input, "struct_invalid.field_invalid", newValue));
+    assertEquals("Field path 'struct_invalid.field_invalid' does not exist.", ex.getMessage());
   }
 }
diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java
@@ -98,7 +98,9 @@ public void testTrendlineCommand() {
 
   @Test
   public void testExpandCommand() {
-    assertEquals("source=t | expand field_name", anonymize("source=t | expand field_name"));
+    String expected = "source=t | expand field_name";
+    String actual = anonymize("source=t | expand field_name");
+    assertEquals(expected, actual);
   }
 
   @Test