From 2b92f73c10b0128f1ef9e9f99462e6a30d6c3e53 Mon Sep 17 00:00:00 2001 From: penghuo Date: Wed, 27 Jan 2021 20:17:38 -0800 Subject: [PATCH 01/11] support object access in sql/ppl --- .../sql/analysis/ExpressionAnalyzer.java | 4 +- .../sql/analysis/QualifierAnalyzer.java | 17 ++++- .../sql/ast/dsl/AstDSL.java | 30 ++++---- .../sql/ast/expression/Field.java | 23 +++--- .../sql/data/model/ExprTupleValue.java | 8 +- .../sql/data/model/ExprValue.java | 10 +++ .../sql/expression/ReferenceExpression.java | 38 ++++++++++ .../bindingtuple/LazyBindingTuple.java | 7 +- .../sql/utils/ExpressionUtils.java | 2 + .../sql/analysis/ExpressionAnalyzerTest.java | 22 +++--- .../sql/analysis/QualifierAnalyzerTest.java | 14 +++- .../sql/data/model/ExprNumberValueTest.java | 9 +++ .../expression/ReferenceExpressionTest.java | 63 +++++++++++++++- .../sql/legacy/ObjectFieldSelectIT.java | 3 + .../sql/ppl/ObjectFieldOperateIT.java | 74 +++++++++++++++++++ ppl/src/main/antlr/OpenDistroPPLParser.g4 | 6 +- .../sql/ppl/parser/AstBuilder.java | 4 +- .../sql/ppl/parser/AstExpressionBuilder.java | 21 ++++-- .../sql/ppl/parser/AstBuilderTest.java | 21 ++++++ .../sql/sql/parser/AstSortBuilder.java | 1 + 20 files changed, 313 insertions(+), 64 deletions(-) create mode 100644 integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzer.java index 4738d25b74..6d123b45a8 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzer.java @@ -259,9 +259,9 @@ private Expression visitIdentifier(String ident, AnalysisContext context) { return ref; } + // Array type is not supporte yet. private boolean isTypeNotSupported(ExprType type) { - return "struct".equalsIgnoreCase(type.typeName()) - || "array".equalsIgnoreCase(type.typeName()); + return "array".equalsIgnoreCase(type.typeName()); } } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzer.java index cbb0541c89..9b732c6c78 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzer.java @@ -51,12 +51,25 @@ public String unqualified(QualifiedName fullName) { private boolean isQualifierIndexOrAlias(QualifiedName fullName) { Optional qualifier = fullName.first(); if (qualifier.isPresent()) { + if (isFieldName(qualifier.get())) { + return false; + } resolveQualifierSymbol(fullName, qualifier.get()); return true; } return false; } + private boolean isFieldName(String qualifier) { + try { + // Resolve the qualifier in Namespace.FIELD_NAME + context.peek().resolve(new Symbol(Namespace.FIELD_NAME, qualifier)); + return true; + } catch (SemanticCheckException e2) { + return false; + } + } + private void resolveQualifierSymbol(QualifiedName fullName, String qualifier) { try { context.peek().resolve(new Symbol(Namespace.INDEX_NAME, qualifier)); @@ -64,8 +77,8 @@ private void resolveQualifierSymbol(QualifiedName fullName, String qualifier) { // Throw syntax check intentionally to indicate fall back to old engine. // Need change to semantic check exception in future. throw new SyntaxCheckException(String.format( - "The qualifier [%s] of qualified name [%s] must be an index name or its alias", - qualifier, fullName)); + "The qualifier [%s] of qualified name [%s] must be an field name, index name or its " + + "alias", qualifier, fullName)); } } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java index dffcd4c0f1..a4979a9d88 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java @@ -116,7 +116,7 @@ public UnresolvedPlan values(List... values) { return new Values(Arrays.asList(values)); } - public static UnresolvedExpression qualifiedName(String... parts) { + public static QualifiedName qualifiedName(String... parts) { return new QualifiedName(Arrays.asList(parts)); } @@ -178,7 +178,7 @@ public static Literal nullLiteral() { } public static Map map(String origin, String target) { - return new Map(new Field(origin), new Field(target)); + return new Map(field(origin), field(target)); } public static Map map(UnresolvedExpression origin, UnresolvedExpression target) { @@ -280,28 +280,28 @@ public AllFields allFields() { return AllFields.of(); } - public Field field(UnresolvedExpression field) { - return new Field((QualifiedName) field); - } - - public Field field(String field) { + public Field field(QualifiedName field) { return new Field(field); } - public Field field(UnresolvedExpression field, Argument... fieldArgs) { - return new Field(field, Arrays.asList(fieldArgs)); + public Field field(String field) { + return new Field(qualifiedName(field)); } public Field field(String field, Argument... fieldArgs) { - return new Field(field, Arrays.asList(fieldArgs)); - } - - public Field field(UnresolvedExpression field, List fieldArgs) { - return new Field(field, fieldArgs); + return field(field, Arrays.asList(fieldArgs)); } +// +// public Field field(String field, Argument... fieldArgs) { +// return new Field(field, Arrays.asList(fieldArgs)); +// } +// +// public Field field(UnresolvedExpression field, List fieldArgs) { +// return new Field(field, fieldArgs); +// } public Field field(String field, List fieldArgs) { - return new Field(field, fieldArgs); + return new Field(qualifiedName(field), fieldArgs); } public Alias alias(String name, UnresolvedExpression expr) { diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java index 198b1c6d94..d0340c2ca2 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java @@ -27,21 +27,24 @@ @Getter @ToString @EqualsAndHashCode(callSuper = false) -@AllArgsConstructor public class Field extends UnresolvedExpression { - private UnresolvedExpression field; - private List fieldArgs = Collections.emptyList(); - public Field(QualifiedName field) { - this.field = field; - } + private final UnresolvedExpression field; - public Field(String field) { - this.field = new QualifiedName(field); + private final List fieldArgs; + + /** + * Constructor of Field + */ + public Field(UnresolvedExpression field) { + this(field, Collections.emptyList()); } - public Field(String field, List fieldArgs) { - this.field = new QualifiedName(field); + /** + * Constructor of Field + */ + public Field(UnresolvedExpression field, List fieldArgs) { + this.field = field; this.fieldArgs = fieldArgs; } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprTupleValue.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprTupleValue.java index fb8cd658a5..05ba8412b6 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprTupleValue.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprTupleValue.java @@ -64,8 +64,7 @@ public String toString() { @Override public BindingTuple bindingTuples() { - return new LazyBindingTuple( - bindingName -> valueMap.getOrDefault(bindingName, ExprMissingValue.of())); + return new LazyBindingTuple(() -> this); } @Override @@ -73,6 +72,11 @@ public Map tupleValue() { return valueMap; } + @Override + public ExprValue keyValue(String key) { + return valueMap.getOrDefault(key, ExprMissingValue.of()); + } + /** * Override the equals method. * @return true for equal, otherwise false. diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java index 7b98c0b4cf..8bb91eb346 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java @@ -196,4 +196,14 @@ default List collectionValue() { throw new ExpressionEvaluationException( "invalid to get collectionValue from value of type " + type()); } + + /** + * Get the value specified by key from {@link ExprTupleValue}. + * This method only be implemented in {@link ExprTupleValue}. + */ + default ExprValue keyValue(String key) { + throw new ExpressionEvaluationException( + String.format("invalid to get keyValue by key: %s from value of type: %s", key, + type())); + } } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java index ba3a8f4e87..bfaeb2809c 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java @@ -15,9 +15,14 @@ package com.amazon.opendistroforelasticsearch.sql.expression; +import static com.amazon.opendistroforelasticsearch.sql.utils.ExpressionUtils.PATH_SEP; + +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprTupleValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; import com.amazon.opendistroforelasticsearch.sql.data.type.ExprType; import com.amazon.opendistroforelasticsearch.sql.expression.env.Environment; +import java.util.Arrays; +import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.RequiredArgsConstructor; @@ -28,8 +33,23 @@ public class ReferenceExpression implements Expression { @Getter private final String attr; + @Getter + private final List paths; + private final ExprType type; + /** + * Constructor of ReferenceExpression. + * @param ref the field name. e.g. addr.state/addr. + * @param type type. + */ + public ReferenceExpression(String ref, ExprType type) { + this.attr = ref; + // Todo. the define of paths need to be redefined after adding multiple index/variable support. + this.paths = Arrays.asList(ref.split("\\.")); + this.type = type; + } + @Override public ExprValue valueOf(Environment env) { return env.resolve(this); @@ -49,4 +69,22 @@ public T accept(ExpressionNodeVisitor visitor, C context) { public String toString() { return attr; } + + /** + * Resolve the ExprValue from {@link ExprTupleValue}. + * @param value {@link ExprTupleValue}. + * @return {@link ExprTupleValue}. + */ + public ExprValue resolve(ExprTupleValue value) { + return resolve(value, paths); + } + + private ExprValue resolve(ExprValue value, List paths) { + final ExprValue wholePathValue = value.keyValue(String.join(PATH_SEP, paths)); + if (!wholePathValue.isMissing() || paths.size() == 1) { + return wholePathValue; + } else { + return resolve(value.keyValue(paths.get(0)), paths.subList(1, paths.size())); + } + } } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/storage/bindingtuple/LazyBindingTuple.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/storage/bindingtuple/LazyBindingTuple.java index f5e29594b9..97a16323f1 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/storage/bindingtuple/LazyBindingTuple.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/storage/bindingtuple/LazyBindingTuple.java @@ -15,9 +15,10 @@ package com.amazon.opendistroforelasticsearch.sql.storage.bindingtuple; +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprTupleValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; -import java.util.function.Function; +import java.util.function.Supplier; import lombok.RequiredArgsConstructor; /** @@ -25,10 +26,10 @@ */ @RequiredArgsConstructor public class LazyBindingTuple extends BindingTuple { - private final Function lazyBinding; + private final Supplier lazyBinding; @Override public ExprValue resolve(ReferenceExpression ref) { - return lazyBinding.apply(ref.getAttr()); + return ref.resolve(lazyBinding.get()); } } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/ExpressionUtils.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/ExpressionUtils.java index dcb8b10239..9b92b06d0f 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/ExpressionUtils.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/ExpressionUtils.java @@ -26,6 +26,8 @@ @UtilityClass public class ExpressionUtils { + public static String PATH_SEP = "."; + /** * Format the list of {@link Expression}. */ diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzerTest.java index c3ecc4c8be..7e6fa063ea 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/ExpressionAnalyzerTest.java @@ -194,13 +194,20 @@ public void qualified_name_with_qualifier() { qualifiedName("index_alias", "integer_value") ); - analysisContext.peek().define(new Symbol(Namespace.FIELD_NAME, "nested_field"), STRUCT); + analysisContext.peek().define(new Symbol(Namespace.FIELD_NAME, "object_field"), STRUCT); + analysisContext.peek().define(new Symbol(Namespace.FIELD_NAME, "object_field.integer_value"), + INTEGER); + assertAnalyzeEqual( + DSL.ref("object_field.integer_value", INTEGER), + qualifiedName("object_field", "integer_value") + ); + SyntaxCheckException exception = assertThrows(SyntaxCheckException.class, () -> analyze(qualifiedName("nested_field", "integer_value"))); assertEquals( "The qualifier [nested_field] of qualified name [nested_field.integer_value] " - + "must be an index name or its alias", + + "must be an field name, index name or its alias", exception.getMessage() ); analysisContext.pop(); @@ -237,17 +244,6 @@ public void case_clause() { AstDSL.stringLiteral("test")))); } - @Test - public void skip_struct_data_type() { - SyntaxCheckException exception = - assertThrows(SyntaxCheckException.class, - () -> analyze(qualifiedName("struct_value"))); - assertEquals( - "Identifier [struct_value] of type [STRUCT] is not supported yet", - exception.getMessage() - ); - } - @Test public void skip_array_data_type() { SyntaxCheckException exception = diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzerTest.java index 4b7014309f..eb8638816a 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/QualifierAnalyzerTest.java @@ -52,11 +52,11 @@ void should_return_original_name_if_no_qualifier() { @Test void should_report_error_if_qualifier_is_not_index() { - runInScope(new Symbol(Namespace.FIELD_NAME, "a"), ARRAY, () -> { + runInScope(new Symbol(Namespace.FIELD_NAME, "aIndex"), ARRAY, () -> { SyntaxCheckException error = assertThrows(SyntaxCheckException.class, () -> qualifierAnalyzer.unqualified("a", "integer_value")); assertEquals("The qualifier [a] of qualified name [a.integer_value] " - + "must be an index name or its alias", error.getMessage()); + + "must be an field name, index name or its alias", error.getMessage()); }); } @@ -65,7 +65,8 @@ void should_report_error_if_qualifier_is_not_exist() { SyntaxCheckException error = assertThrows(SyntaxCheckException.class, () -> qualifierAnalyzer.unqualified("a", "integer_value")); assertEquals( - "The qualifier [a] of qualified name [a.integer_value] must be an index name or its alias", + "The qualifier [a] of qualified name [a.integer_value] must be an field name, index name " + + "or its alias", error.getMessage()); } @@ -76,6 +77,13 @@ void should_return_qualified_name_if_qualifier_is_index() { ); } + @Test + void should_return_qualified_name_if_qualifier_is_field() { + runInScope(new Symbol(Namespace.FIELD_NAME, "a"), STRUCT, () -> + assertEquals("a.integer_value", qualifierAnalyzer.unqualified("a", "integer_value")) + ); + } + @Test void should_report_error_if_more_parts_in_qualified_name() { runInScope(new Symbol(Namespace.INDEX_NAME, "a"), STRUCT, () -> diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java index fe33301818..1fd36eeaac 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java @@ -31,4 +31,13 @@ public void getShortValueFromIncompatibleExprValue() { .assertThrows(ExpressionEvaluationException.class, () -> booleanValue.shortValue()); assertEquals("invalid to get shortValue from value of type BOOLEAN", exception.getMessage()); } + + @Test + public void key_value() { + final ExprIntegerValue value = new ExprIntegerValue(1); + + ExpressionEvaluationException exception = Assertions + .assertThrows(ExpressionEvaluationException.class, () -> value.keyValue("path")); + assertEquals("invalid to get keyValue by key: path from value of type: INTEGER", exception.getMessage()); + } } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java index a5d2b934f1..4766c2f53f 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java @@ -35,11 +35,15 @@ import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.LONG; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.STRING; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.STRUCT; +import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprStringValue; +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprTupleValue; +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValueUtils; import com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType; -import com.amazon.opendistroforelasticsearch.sql.exception.ExpressionEvaluationException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.DisplayNameGeneration; @@ -77,4 +81,59 @@ public void resolve_type() { assertEquals(ExprCoreType.STRUCT, DSL.ref("struct_value", STRUCT).type()); assertEquals(ExprCoreType.ARRAY, DSL.ref("array_value", ARRAY).type()); } + + @Test + public void path_as_whole_has_highest_priority() { + ReferenceExpression expr = new ReferenceExpression("name.nick", STRING); + ExprValue actualValue = expr.resolve(tuple()); + + assertEquals(STRING, actualValue.type()); + assertEquals("bob", actualValue.stringValue()); + } + + @Test + public void one_path_value() { + ReferenceExpression expr = ref("name", STRING); + ExprValue actualValue = expr.resolve(tuple()); + + assertEquals(STRING, actualValue.type()); + assertEquals("bob smith", actualValue.stringValue()); + } + + @Test + public void multiple_path_value() { + ReferenceExpression expr = new ReferenceExpression("address.state", STRING); + ExprValue actualValue = expr.resolve(tuple()); + + assertEquals(STRING, actualValue.type()); + assertEquals("WA", actualValue.stringValue()); + } + + @Test + public void not_exist_path() { + ReferenceExpression expr = new ReferenceExpression("missing_field", STRING); + ExprValue actualValue = expr.resolve(tuple()); + + assertTrue(actualValue.isMissing()); + } + + /** + * { + * "name": "bob smith" + * "name.nick": "bob", + * "address": { + * "state": "WA", + * "city": "seattle" + * } + * } + */ + private ExprTupleValue tuple() { + ExprValue address = + ExprValueUtils.tupleValue(ImmutableMap.of("state", "WA", "city", "seattle")); + ExprTupleValue tuple = ExprTupleValue.fromExprValueMap(ImmutableMap.of( + "name", new ExprStringValue("bob smith"), + "name.nick", new ExprStringValue("bob"), + "address", address)); + return tuple; + } } \ No newline at end of file diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java index a437179f98..67f6fca890 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java @@ -25,6 +25,7 @@ import com.amazon.opendistroforelasticsearch.sql.legacy.utils.StringUtils; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; /** @@ -94,6 +95,8 @@ public void testSelectNestedFieldItself() { ); } + @Ignore("Issue track the multiple values for a field. Actually we should not expected return a " + + "array for object field") @Test public void testSelectObjectFieldOfArrayValuesItself() { JSONObject response = new JSONObject(query("SELECT accounts FROM %s")); diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java new file mode 100644 index 0000000000..a6ac70c7d6 --- /dev/null +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java @@ -0,0 +1,74 @@ +package com.amazon.opendistroforelasticsearch.sql.ppl; + +import static com.amazon.opendistroforelasticsearch.sql.legacy.SQLIntegTestCase.Index.DEEP_NESTED; +import static com.amazon.opendistroforelasticsearch.sql.legacy.TestsConstants.TEST_INDEX_DEEP_NESTED; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.rows; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.schema; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.verifyDataRows; +import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.verifySchema; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.Test; + +public class ObjectFieldOperateIT extends PPLIntegTestCase { + + @Override + public void init() throws IOException { + loadIndex(DEEP_NESTED); + } + + @Test + public void select_object_field() throws IOException { + JSONObject result = executeQuery( + String.format("source=%s | " + + "fields city.name, city.location.latitude", + TEST_INDEX_DEEP_NESTED)); + verifySchema(result, + schema("city.name", "string"), + schema("city.location.latitude", "double")); + verifyDataRows(result, + rows("Seattle", 10.5)); + } + + @Test + public void compare_object_field_in_where() throws IOException { + JSONObject result = executeQuery( + String.format("source=%s " + + "| where city.name = 'Seattle' " + + "| fields city.name, city.location.latitude", + TEST_INDEX_DEEP_NESTED)); + verifySchema(result, + schema("city.name", "string"), + schema("city.location.latitude", "double")); + verifyDataRows(result, + rows("Seattle", 10.5)); + } + + @Test + public void group_object_field_in_stats() throws IOException { + JSONObject result = executeQuery( + String.format("source=%s " + + "| stats count() by city.name", + TEST_INDEX_DEEP_NESTED)); + verifySchema(result, + schema("count()", "integer"), + schema("city.name", "string")); + verifyDataRows(result, + rows(1, "Seattle")); + } + + @Test + public void sort_by_object_field() throws IOException { + JSONObject result = executeQuery( + String.format("source=%s " + + "| sort city.name" + + "| fields city.name, city.location.latitude", + TEST_INDEX_DEEP_NESTED)); + verifySchema(result, + schema("city.name", "string"), + schema("city.location.latitude", "double")); + verifyDataRows(result, + rows("Seattle", 10.5)); + } +} diff --git a/ppl/src/main/antlr/OpenDistroPPLParser.g4 b/ppl/src/main/antlr/OpenDistroPPLParser.g4 index dafc05c7e2..a4fe7f0938 100644 --- a/ppl/src/main/antlr/OpenDistroPPLParser.g4 +++ b/ppl/src/main/antlr/OpenDistroPPLParser.g4 @@ -42,7 +42,7 @@ whereCommand ; fieldsCommand - : FIELDS (PLUS | MINUS)? wcFieldList + : FIELDS (PLUS | MINUS)? fieldList ; renameCommand @@ -312,11 +312,11 @@ valueList ; qualifiedName - : ident #identsAsQualifiedName + : ident (DOT ident)* #identsAsQualifiedName ; wcQualifiedName - : wildcard #identsAsWildcardQualifiedName + : wildcard (DOT wildcard)* #identsAsWildcardQualifiedName ; ident diff --git a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java index 8a8c011190..6e56c80509 100644 --- a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java +++ b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java @@ -119,8 +119,8 @@ public UnresolvedPlan visitWhereCommand(WhereCommandContext ctx) { @Override public UnresolvedPlan visitFieldsCommand(FieldsCommandContext ctx) { return new Project( - ctx.wcFieldList() - .wcFieldExpression() + ctx.fieldList() + .fieldExpression() .stream() .map(this::visitExpression) .collect(Collectors.toList()), diff --git a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilder.java b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilder.java index 0cb16a4121..4d1bf4a23c 100644 --- a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilder.java +++ b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilder.java @@ -15,6 +15,7 @@ package com.amazon.opendistroforelasticsearch.sql.ppl.parser; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.qualifiedName; import static com.amazon.opendistroforelasticsearch.sql.expression.function.BuiltinFunctionName.IS_NOT_NULL; import static com.amazon.opendistroforelasticsearch.sql.expression.function.BuiltinFunctionName.IS_NULL; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.BinaryArithmeticContext; @@ -67,9 +68,11 @@ import com.google.common.collect.ImmutableMap; import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.stream.Collectors; import org.antlr.v4.runtime.ParserRuleContext; +import org.antlr.v4.runtime.RuleContext; /** * Class of building AST Expression nodes. @@ -171,7 +174,7 @@ public UnresolvedExpression visitWcFieldExpression(WcFieldExpressionContext ctx) @Override public UnresolvedExpression visitSortField(SortFieldContext ctx) { return new Field( - ctx.sortFieldExpression().fieldExpression().getText(), + qualifiedName(ctx.sortFieldExpression().fieldExpression().getText()), ArgumentFactory.getArgumentList(ctx) ); } @@ -227,7 +230,7 @@ public UnresolvedExpression visitEvalFunctionCall(EvalFunctionCallContext ctx) { @Override public UnresolvedExpression visitTableSource(TableSourceContext ctx) { - return visitIdentifier(ctx); + return visitIdentifiers(Arrays.asList(ctx)); } /** @@ -235,13 +238,13 @@ public UnresolvedExpression visitTableSource(TableSourceContext ctx) { */ @Override public UnresolvedExpression visitIdentsAsQualifiedName(IdentsAsQualifiedNameContext ctx) { - return visitIdentifier(ctx.ident()); + return visitIdentifiers(ctx.ident()); } @Override public UnresolvedExpression visitIdentsAsWildcardQualifiedName( IdentsAsWildcardQualifiedNameContext ctx) { - return visitIdentifier(ctx.wildcard()); + return visitIdentifiers(ctx.wildcard()); } @Override @@ -270,9 +273,13 @@ public UnresolvedExpression visitBooleanLiteral(BooleanLiteralContext ctx) { return new Literal(Boolean.valueOf(ctx.getText()), DataType.BOOLEAN); } - private UnresolvedExpression visitIdentifier(ParserRuleContext ctx) { - return new QualifiedName(Collections.singletonList( - StringUtils.unquoteIdentifier(ctx.getText()))); + private QualifiedName visitIdentifiers(List ctx) { + return new QualifiedName( + ctx.stream() + .map(RuleContext::getText) + .map(StringUtils::unquoteIdentifier) + .collect(Collectors.toList()) + ); } } diff --git a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java index 11b035153a..82bd27aa0e 100644 --- a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java @@ -38,6 +38,7 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.map; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.nullLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.projectWithArg; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.qualifiedName; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.rareTopN; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.relation; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.rename; @@ -112,6 +113,16 @@ public void testWhereCommand() { ); } + @Test + public void testWhereCommandWithQualifiedName() { + assertEqual("search source=t | where a.v=1", + filter( + relation("t"), + compare("=", field(qualifiedName("a", "v")), intLiteral(1)) + ) + ); + } + @Test public void testFieldsCommandWithoutArguments() { assertEqual("source=t | fields f, g", @@ -142,6 +153,16 @@ public void testFieldsCommandWithExcludeArguments() { )); } + @Test + public void testSearchCommandWithQualifiedName() { + assertEqual("source=t | fields f.v, g.v", + projectWithArg( + relation("t"), + defaultFieldsArgs(), + field(qualifiedName("f", "v")), field(qualifiedName("g", "v")) + )); + } + @Test public void testRenameCommand() { assertEqual("source=t | rename f as g", diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java index 0af37f6405..1dfd615eaf 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java @@ -17,6 +17,7 @@ package com.amazon.opendistroforelasticsearch.sql.sql.parser; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.booleanLiteral; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.qualifiedName; import static com.amazon.opendistroforelasticsearch.sql.ast.expression.DataType.INTEGER; import static com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort.NullOrder.NULL_FIRST; import static com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort.SortOrder.DESC; From db83e5ab57aafb2d8e8e78334d96aabcd145316a Mon Sep 17 00:00:00 2001 From: penghuo Date: Wed, 27 Jan 2021 21:24:14 -0800 Subject: [PATCH 02/11] update doc --- docs/category.json | 1 + docs/experiment/ppl/general/datatypes.rst | 183 ++++++++++++++++++ docs/user/beyond/partiql.rst | 19 +- .../sql/legacy/ObjectFieldSelectIT.java | 4 +- 4 files changed, 190 insertions(+), 17 deletions(-) diff --git a/docs/category.json b/docs/category.json index d570fdd449..fe695f89df 100644 --- a/docs/category.json +++ b/docs/category.json @@ -15,6 +15,7 @@ "experiment/ppl/cmd/stats.rst", "experiment/ppl/cmd/where.rst", "experiment/ppl/general/identifiers.rst", + "experiment/ppl/general/datatypes.rst", "experiment/ppl/functions/math.rst", "experiment/ppl/functions/datetime.rst", "experiment/ppl/functions/string.rst", diff --git a/docs/experiment/ppl/general/datatypes.rst b/docs/experiment/ppl/general/datatypes.rst index f5e0841bf4..ee53df01f5 100644 --- a/docs/experiment/ppl/general/datatypes.rst +++ b/docs/experiment/ppl/general/datatypes.rst @@ -224,3 +224,186 @@ String Data Types A string is a sequence of characters enclosed in either single or double quotes. For example, both 'text' and "text" will be treated as string literal. + +Query Struct Data Types +======================= + +In PPL, the Struct Data Types corresponding to the `Object field type in Elasticsearch `_. The "." is used as the path selector when access the inner attribute of the struct data. + +Example: People +--------------- + +There are three fields in test index ``people``: 1) deep nested object field ``city``; 2) object field of array value ``account``; 3) nested field ``projects``:: + + { + "mappings": { + "properties": { + "city": { + "properties": { + "name": { + "type": "keyword" + }, + "location": { + "properties": { + "latitude": { + "type": "double" + } + } + } + } + }, + "account": { + "properties": { + "id": { + "type": "keyword" + } + } + }, + "projects": { + "type": "nested", + "properties": { + "name": { + "type": "keyword" + } + } + } + } + } + } + +Example: Employees +------------------ + +Here is the mapping for test index ``employees_nested``. Note that field ``projects`` is a nested field:: + + { + "mappings": { + "properties": { + "id": { + "type": "long" + }, + "name": { + "type": "text", + "fields": { + "keyword": { + "type": "keyword", + "ignore_above": 256 + } + } + }, + "projects": { + "type": "nested", + "properties": { + "name": { + "type": "text", + "fields": { + "keyword": { + "type": "keyword" + } + }, + "fielddata": true + }, + "started_year": { + "type": "long" + } + } + }, + "title": { + "type": "text", + "fields": { + "keyword": { + "type": "keyword", + "ignore_above": 256 + } + } + } + } + } + } + + +Result set:: + + { + "employees_nested" : [ + { + "id" : 3, + "name" : "Bob Smith", + "title" : null, + "projects" : [ + { + "name" : "AWS Redshift Spectrum querying", + "started_year" : 1990 + }, + { + "name" : "AWS Redshift security", + "started_year" : 1999 + }, + { + "name" : "AWS Aurora security", + "started_year" : 2015 + } + ] + }, + { + "id" : 4, + "name" : "Susan Smith", + "title" : "Dev Mgr", + "projects" : [ ] + }, + { + "id" : 6, + "name" : "Jane Smith", + "title" : "Software Eng 2", + "projects" : [ + { + "name" : "AWS Redshift security", + "started_year" : 1998 + }, + { + "name" : "AWS Hello security", + "started_year" : 2015, + "address" : [ + { + "city" : "Dallas", + "state" : "TX" + } + ] + } + ] + } + ] + } + + +Example 1: Select struct inner attribute +---------------------------------------- + +The example show fetch city (top level), city.name (second level), city.location.latitude (deeper level) struct type data from people results. + +PPL query:: + + od> source=people | fields city, city.name, city.location.latitude; + fetched rows / total rows = 1/1 + +-----------------------------------------------------+-------------+--------------------------+ + | city | city.name | city.location.latitude | + |-----------------------------------------------------+-------------+--------------------------| + | {'name': 'Seattle', 'location': {'latitude': 10.5}} | Seattle | 10.5 | + +-----------------------------------------------------+-------------+--------------------------+ + + +Example 2: Group by struct inner attribute +------------------------------------------ + +The example show group by object field inner attribute. + +PPL query:: + + od> source=people | stats count() by city.name; + fetched rows / total rows = 1/1 + +-----------+-------------+ + | count() | city.name | + |-----------+-------------| + | 1 | Seattle | + +-----------+-------------+ + diff --git a/docs/user/beyond/partiql.rst b/docs/user/beyond/partiql.rst index adec1a64db..8954cfdd38 100644 --- a/docs/user/beyond/partiql.rst +++ b/docs/user/beyond/partiql.rst @@ -187,11 +187,12 @@ Before looking into how nested object field (tuple values) be queried, we need t | | | | | object or JSON array. | +-------------------------+---------------+-----------------------+---------------------------------------------+-------------------------+ | Selecting second level | Yes | No | Yes | | -| | | (null returned) | (or null returned if not in PartiQL syntax) | | +| | | (exception may) | (or null returned if not in PartiQL syntax) | | +| | | be thrown) | | | +-------------------------+---------------+-----------------------+---------------------------------------------+ PartiQL specification | | Selecting deeper levels | Yes | No | No | is followed | -| | | (null returned) | (exception may | | -| | | | be thrown) | | +| | | (exception may | (exception may | | +| | | be thrown) | be thrown) | | +-------------------------+---------------+-----------------------+---------------------------------------------+-------------------------+ Example 1: Selecting Top Level @@ -220,18 +221,6 @@ Selecting at deeper levels for object fields of regular value returns inner fiel | {'latitude': 10.5} | 10.5 | +--------------------+--------------------------+ -Example 3: Selecting Field of Array Value ------------------------------------------ - -Select deeper level for object fields of array value which returns ``NULL``. For example, because inner field ``accounts.id`` has three values instead of a tuple in this document, null is returned. Similarly, selecting inner field ``projects.name`` directly in nested field returns null:: - - od> SELECT accounts.id, projects.name FROM people; - fetched rows / total rows = 1/1 - +---------------+-----------------+ - | accounts.id | projects.name | - |---------------+-----------------| - | null | null | - +---------------+-----------------+ For selecting second level for nested fields, please read on and find more details in the following sections. diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java index 67f6fca890..9d510d42e8 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/ObjectFieldSelectIT.java @@ -95,8 +95,8 @@ public void testSelectNestedFieldItself() { ); } - @Ignore("Issue track the multiple values for a field. Actually we should not expected return a " - + "array for object field") + @Ignore("Issue 1017 track the multiple values for a field. Actually we should not expected " + + "return a array for object field") @Test public void testSelectObjectFieldOfArrayValuesItself() { JSONObject response = new JSONObject(query("SELECT accounts FROM %s")); From e53d345126991e1d44dfee077632179a584f7022 Mon Sep 17 00:00:00 2001 From: penghuo Date: Wed, 27 Jan 2021 21:31:58 -0800 Subject: [PATCH 03/11] update doc --- docs/experiment/ppl/general/datatypes.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/experiment/ppl/general/datatypes.rst b/docs/experiment/ppl/general/datatypes.rst index ee53df01f5..c9ee529287 100644 --- a/docs/experiment/ppl/general/datatypes.rst +++ b/docs/experiment/ppl/general/datatypes.rst @@ -406,4 +406,3 @@ PPL query:: |-----------+-------------| | 1 | Seattle | +-----------+-------------+ - From e83a8734afc1c5e7ddc4138828e68238cd5037e3 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 08:21:42 -0800 Subject: [PATCH 04/11] fix build --- .../sql/ast/dsl/AstDSL.java | 22 +++++++++---------- .../sql/ast/expression/Field.java | 4 ++-- .../sql/data/model/ExprNumberValueTest.java | 3 ++- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java index a4979a9d88..502b2ffc4d 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java @@ -280,28 +280,28 @@ public AllFields allFields() { return AllFields.of(); } - public Field field(QualifiedName field) { + public Field field(UnresolvedExpression field) { return new Field(field); } + public Field field(UnresolvedExpression field, Argument... fieldArgs) { + return field(field, Arrays.asList(fieldArgs)); + } + public Field field(String field) { - return new Field(qualifiedName(field)); + return field(qualifiedName(field)); } public Field field(String field, Argument... fieldArgs) { return field(field, Arrays.asList(fieldArgs)); } -// -// public Field field(String field, Argument... fieldArgs) { -// return new Field(field, Arrays.asList(fieldArgs)); -// } -// -// public Field field(UnresolvedExpression field, List fieldArgs) { -// return new Field(field, fieldArgs); -// } + + public Field field(UnresolvedExpression field, List fieldArgs) { + return new Field(field, fieldArgs); + } public Field field(String field, List fieldArgs) { - return new Field(qualifiedName(field), fieldArgs); + return field(qualifiedName(field), fieldArgs); } public Alias alias(String name, UnresolvedExpression expr) { diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java index d0340c2ca2..b92b4003cf 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Field.java @@ -34,14 +34,14 @@ public class Field extends UnresolvedExpression { private final List fieldArgs; /** - * Constructor of Field + * Constructor of Field. */ public Field(UnresolvedExpression field) { this(field, Collections.emptyList()); } /** - * Constructor of Field + * Constructor of Field. */ public Field(UnresolvedExpression field, List fieldArgs) { this.field = field; diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java index 1fd36eeaac..3ca144807a 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java @@ -38,6 +38,7 @@ public void key_value() { ExpressionEvaluationException exception = Assertions .assertThrows(ExpressionEvaluationException.class, () -> value.keyValue("path")); - assertEquals("invalid to get keyValue by key: path from value of type: INTEGER", exception.getMessage()); + assertEquals("invalid to get keyValue by key: path from value of type: INTEGER", + exception.getMessage()); } } From bc8fff0e0936d762e6b8526bfc8f448338a743a9 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 08:53:43 -0800 Subject: [PATCH 05/11] fix license --- .../sql/ppl/ObjectFieldOperateIT.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java index a6ac70c7d6..12d7eb71ec 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ObjectFieldOperateIT.java @@ -1,3 +1,19 @@ +/* + * Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + package com.amazon.opendistroforelasticsearch.sql.ppl; import static com.amazon.opendistroforelasticsearch.sql.legacy.SQLIntegTestCase.Index.DEEP_NESTED; From 78a34875819d7b3781d8b3b58569748a0b4d6325 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 12:08:52 -0800 Subject: [PATCH 06/11] update --- .../sql/data/model/ExprValue.java | 4 +--- .../sql/data/model/ExprNumberValueTest.java | 8 ++------ .../sql/legacy/PrettyFormatResponseIT.java | 2 ++ 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java index 8bb91eb346..0faa314857 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java @@ -202,8 +202,6 @@ default List collectionValue() { * This method only be implemented in {@link ExprTupleValue}. */ default ExprValue keyValue(String key) { - throw new ExpressionEvaluationException( - String.format("invalid to get keyValue by key: %s from value of type: %s", key, - type())); + return ExprMissingValue.of(); } } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java index 3ca144807a..d1e20d8a17 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprNumberValueTest.java @@ -18,6 +18,7 @@ package com.amazon.opendistroforelasticsearch.sql.data.model; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.amazon.opendistroforelasticsearch.sql.exception.ExpressionEvaluationException; import org.junit.jupiter.api.Assertions; @@ -34,11 +35,6 @@ public void getShortValueFromIncompatibleExprValue() { @Test public void key_value() { - final ExprIntegerValue value = new ExprIntegerValue(1); - - ExpressionEvaluationException exception = Assertions - .assertThrows(ExpressionEvaluationException.class, () -> value.keyValue("path")); - assertEquals("invalid to get keyValue by key: path from value of type: INTEGER", - exception.getMessage()); + assertTrue(new ExprIntegerValue(1).keyValue("path").isMissing()); } } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/PrettyFormatResponseIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/PrettyFormatResponseIT.java index 7ade383945..bbe47c946f 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/PrettyFormatResponseIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/PrettyFormatResponseIT.java @@ -136,6 +136,8 @@ public void selectWrongField() throws IOException { assertThat(getDataRows(response).length(), equalTo(RESPONSE_DEFAULT_MAX_SIZE)); } + @Ignore("Issue 1019, Breaking Change, the keyword should not been allowed, semantic exception " + + "is expected") @Test public void selectKeyword() throws IOException { JSONObject response = executeQuery( From fe2f0679d95e838d8ad23be71d6ac36284e523c6 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 13:36:50 -0800 Subject: [PATCH 07/11] add docs --- .../sql/expression/ReferenceExpression.java | 26 ++++++++++++++++++- .../expression/ReferenceExpressionTest.java | 17 ++++++++---- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java index bfaeb2809c..9c04f6870c 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java @@ -71,7 +71,31 @@ public String toString() { } /** - * Resolve the ExprValue from {@link ExprTupleValue}. + * Resolve the ExprValue from {@link ExprTupleValue} using paths. + * + * Considering the following sample data. + * { + * "name": "bob smith" + * "project.year": 1990, + * "project": { + * "year": "2020" + * } + * "address": { + * "state": "WA", + * "city": "seattle" + * } + * } + * The paths could be + * 1. top level, e.g. "name", which will be resolved as "bob smith" + * 2. multiple paths, e.g. "name.address.state", which will be resolved as "WA" + * 3. special case, the "." is the path separator, but it is possible that the path include + * ".", for handling this use case, we define the resolve rule as bellow, e.g. "project.year" is + * resolved as 1990 instead of 2020. + * + * Resolved Rule + * 1. Resolve the full name by combine the paths("x"."y"."z") as whole ("x.y.z"). + * 2. Resolve the path recursively through ExprValue. + * * @param value {@link ExprTupleValue}. * @return {@link ExprTupleValue}. */ diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java index 4766c2f53f..551aa77894 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java @@ -39,6 +39,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.amazon.opendistroforelasticsearch.sql.data.model.ExprIntegerValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprStringValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprTupleValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; @@ -84,11 +85,11 @@ public void resolve_type() { @Test public void path_as_whole_has_highest_priority() { - ReferenceExpression expr = new ReferenceExpression("name.nick", STRING); + ReferenceExpression expr = new ReferenceExpression("project.year", INTEGER); ExprValue actualValue = expr.resolve(tuple()); - assertEquals(STRING, actualValue.type()); - assertEquals("bob", actualValue.stringValue()); + assertEquals(INTEGER, actualValue.type()); + assertEquals(1990, actualValue.integerValue()); } @Test @@ -120,7 +121,10 @@ public void not_exist_path() { /** * { * "name": "bob smith" - * "name.nick": "bob", + * "project.year": 1990, + * "project": { + * "year": 2020 + * }, * "address": { * "state": "WA", * "city": "seattle" @@ -130,9 +134,12 @@ public void not_exist_path() { private ExprTupleValue tuple() { ExprValue address = ExprValueUtils.tupleValue(ImmutableMap.of("state", "WA", "city", "seattle")); + ExprValue project = + ExprValueUtils.tupleValue(ImmutableMap.of("year", 2020)); ExprTupleValue tuple = ExprTupleValue.fromExprValueMap(ImmutableMap.of( "name", new ExprStringValue("bob smith"), - "name.nick", new ExprStringValue("bob"), + "project.year", new ExprIntegerValue(1990), + "project", project, "address", address)); return tuple; } From 274019776d17a6ff4de001c69c440ce1487a6a48 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 13:43:02 -0800 Subject: [PATCH 08/11] update --- .../sql/expression/ReferenceExpression.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java index 9c04f6870c..084af70a8d 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java @@ -71,8 +71,7 @@ public String toString() { } /** - * Resolve the ExprValue from {@link ExprTupleValue} using paths. - * + * Resolve the ExprValue from {@link ExprTupleValue} using paths.* * Considering the following sample data. * { * "name": "bob smith" @@ -91,7 +90,6 @@ public String toString() { * 3. special case, the "." is the path separator, but it is possible that the path include * ".", for handling this use case, we define the resolve rule as bellow, e.g. "project.year" is * resolved as 1990 instead of 2020. - * * Resolved Rule * 1. Resolve the full name by combine the paths("x"."y"."z") as whole ("x.y.z"). * 2. Resolve the path recursively through ExprValue. From d6a6503db66a109bc8f86e4b47a79eb776077225 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 18:31:55 -0800 Subject: [PATCH 09/11] update --- .../sql/expression/ReferenceExpression.java | 13 ++++++-- .../expression/ReferenceExpressionTest.java | 30 +++++++++++++++++-- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java index 084af70a8d..3f2de5671b 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java @@ -71,7 +71,7 @@ public String toString() { } /** - * Resolve the ExprValue from {@link ExprTupleValue} using paths.* + * Resolve the ExprValue from {@link ExprTupleValue} using paths. * Considering the following sample data. * { * "name": "bob smith" @@ -81,7 +81,11 @@ public String toString() { * } * "address": { * "state": "WA", - * "city": "seattle" + * "city": "seattle", + * "project.year": 1990 + * } + * "address.local": { + * "state": "WA", * } * } * The paths could be @@ -89,7 +93,10 @@ public String toString() { * 2. multiple paths, e.g. "name.address.state", which will be resolved as "WA" * 3. special case, the "." is the path separator, but it is possible that the path include * ".", for handling this use case, we define the resolve rule as bellow, e.g. "project.year" is - * resolved as 1990 instead of 2020. + * resolved as 1990 instead of 2020. Note. This logic only applied top level none object field. + * e.g. "address.local.state" been resolved to Missing. but "address.project.year" could been + * resolved as 1990. + * * Resolved Rule * 1. Resolve the full name by combine the paths("x"."y"."z") as whole ("x.y.z"). * 2. Resolve the path recursively through ExprValue. diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java index 551aa77894..6faafcb0c7 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpressionTest.java @@ -118,6 +118,23 @@ public void not_exist_path() { assertTrue(actualValue.isMissing()); } + @Test + public void object_field_contain_dot() { + ReferenceExpression expr = new ReferenceExpression("address.local.state", STRING); + ExprValue actualValue = expr.resolve(tuple()); + + assertTrue(actualValue.isMissing()); + } + + @Test + public void innner_none_object_field_contain_dot() { + ReferenceExpression expr = new ReferenceExpression("address.project.year", INTEGER); + ExprValue actualValue = expr.resolve(tuple()); + + assertEquals(INTEGER, actualValue.type()); + assertEquals(1990, actualValue.integerValue()); + } + /** * { * "name": "bob smith" @@ -128,19 +145,28 @@ public void not_exist_path() { * "address": { * "state": "WA", * "city": "seattle" + * "project.year": 1990 + * }, + * "address.local": { + * "state": "WA", * } * } */ private ExprTupleValue tuple() { ExprValue address = - ExprValueUtils.tupleValue(ImmutableMap.of("state", "WA", "city", "seattle")); + ExprValueUtils.tupleValue(ImmutableMap.of("state", "WA", "city", "seattle", "project" + + ".year", 1990)); ExprValue project = ExprValueUtils.tupleValue(ImmutableMap.of("year", 2020)); + ExprValue addressLocal = + ExprValueUtils.tupleValue(ImmutableMap.of("state", "WA")); ExprTupleValue tuple = ExprTupleValue.fromExprValueMap(ImmutableMap.of( "name", new ExprStringValue("bob smith"), "project.year", new ExprIntegerValue(1990), "project", project, - "address", address)); + "address", address, + "address.local", addressLocal + )); return tuple; } } \ No newline at end of file From cdedb2e505e0eba713e4b6b2823d47cc41c64622 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 18:35:56 -0800 Subject: [PATCH 10/11] update --- .../sql/expression/ReferenceExpression.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java index 3f2de5671b..4171a2f08d 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java @@ -96,8 +96,8 @@ public String toString() { * resolved as 1990 instead of 2020. Note. This logic only applied top level none object field. * e.g. "address.local.state" been resolved to Missing. but "address.project.year" could been * resolved as 1990. - * - * Resolved Rule + *

+ * Resolve Rule * 1. Resolve the full name by combine the paths("x"."y"."z") as whole ("x.y.z"). * 2. Resolve the path recursively through ExprValue. * From 8004a8061dc0fb4124d3fa84cccc250023fac018 Mon Sep 17 00:00:00 2001 From: penghuo Date: Thu, 28 Jan 2021 18:41:28 -0800 Subject: [PATCH 11/11] update doc --- .../sql/expression/ReferenceExpression.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java index 4171a2f08d..aa0a6e7b94 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/ReferenceExpression.java @@ -96,8 +96,8 @@ public String toString() { * resolved as 1990 instead of 2020. Note. This logic only applied top level none object field. * e.g. "address.local.state" been resolved to Missing. but "address.project.year" could been * resolved as 1990. - *

- * Resolve Rule + * + *

Resolve Rule * 1. Resolve the full name by combine the paths("x"."y"."z") as whole ("x.y.z"). * 2. Resolve the path recursively through ExprValue. *