From 8e7455670e40a0b47b92ce8e4b161614224cb17b Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 5 Feb 2025 17:53:57 -0800 Subject: [PATCH] Cleanup, modify to return `null` for an empty array. Signed-off-by: currantw --- .../org/opensearch/sql/analysis/Analyzer.java | 5 +---- .../org/opensearch/sql/ast/tree/Expand.java | 4 ++-- .../sql/planner/physical/ExpandOperator.java | 8 ++++++- .../org/opensearch/sql/utils/PathUtils.java | 5 +++-- .../planner/logical/LogicalExpandTest.java | 18 +++++++++++---- .../planner/physical/ExpandOperatorTest.java | 3 ++- .../opensearch/sql/utils/PathUtilsTest.java | 22 +++++++++---------- .../ppl/utils/PPLQueryDataAnonymizerTest.java | 4 +++- 8 files changed, 43 insertions(+), 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 index fdcfbc1ca3..ac0fa054c9 100644 --- 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 index 70fa3666b7..77aaac885d 100644 --- 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 operation. */ +/** AST node representing an {@code expand } 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 index ca620dffff..c7a3f59373 100644 --- 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 expandExprValueAtPath(ExprValue rootExprValue, St return new LinkedList<>(Collections.singletonList(rootExprValue)); } - return targetExprValue.collectionValue().stream() + List 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 index d8ba8615d1..95ed46cd37 100644 --- 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 pathComponents = splitPath(path); + List 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 pathComponents = splitPath(path); + List 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 index c34816eed7..427844cc0b 100644 --- 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 index 0c8c2094ea..c8e54aea97 100644 --- 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 index 4923d21e14..f83601ff2d 100644 --- 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 index 5a2a07f098..2f92ceb854 100644 --- 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