From 869d8ab16fb332df187dc58404e5f6e1bad25a6f Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 16 Jul 2020 10:03:06 -0700 Subject: [PATCH 01/27] Change grammar --- .../sql/analysis/ExpressionAnalyzer.java | 6 ++++++ sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 | 4 ++++ sql/src/main/antlr/OpenDistroSQLParser.g4 | 1 + .../sql/sql/parser/AstExpressionBuilder.java | 6 ++++++ .../sql/sql/parser/AstBuilderTest.java | 7 ++++--- 5 files changed, 21 insertions(+), 3 deletions(-) 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 2f9710b7b6..02a5da41b5 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 @@ -27,6 +27,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.expression.Literal; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Not; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Or; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.QualifiedName; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedAttribute; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Xor; @@ -149,6 +150,11 @@ public Expression visitField(Field node, AnalysisContext context) { return visitIdentifier(attr, context); } + @Override + public Expression visitQualifiedName(QualifiedName node, AnalysisContext context) { + return visitIdentifier(node.toString(), context); + } + private Expression visitIdentifier(String ident, AnalysisContext context) { TypeEnvironment typeEnv = context.peek(); ReferenceExpression ref = DSL.ref(ident, diff --git a/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 b/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 index f29895e522..7c83d453e3 100644 --- a/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 +++ b/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 @@ -34,6 +34,10 @@ tableName : qualifiedName ; +columnName + : qualifiedName + ; + qualifiedName : ident (DOT ident)* ; diff --git a/sql/src/main/antlr/OpenDistroSQLParser.g4 b/sql/src/main/antlr/OpenDistroSQLParser.g4 index a6517c5572..13d4539730 100644 --- a/sql/src/main/antlr/OpenDistroSQLParser.g4 +++ b/sql/src/main/antlr/OpenDistroSQLParser.g4 @@ -151,6 +151,7 @@ predicate expressionAtom : constant #constantExpressionAtom + | columnName #fullColumnNameExpressionAtom | functionCall #functionCallExpressionAtom | LR_BRACKET expression RR_BRACKET #nestedExpressionAtom | left=expressionAtom mathOperator right=expressionAtom #mathExpressionAtom diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java index b3fab91c47..8a80e03684 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java @@ -29,6 +29,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.expression.QualifiedName; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser; +import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.ColumnNameContext; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.IdentContext; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.NestedExpressionAtomContext; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.QualifiedNameContext; @@ -48,6 +49,11 @@ public UnresolvedExpression visitTableName(TableNameContext ctx) { return new QualifiedName(visitQualifiedNameText(ctx)); } + @Override + public UnresolvedExpression visitColumnName(ColumnNameContext ctx) { + return new QualifiedName(visitQualifiedNameText(ctx)); + } + @Override public UnresolvedExpression visitIdent(IdentContext ctx) { return new QualifiedName(visitQualifiedNameText(ctx)); diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java index 5f047b22b5..6aef402e97 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java @@ -20,6 +20,7 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.doubleLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.intLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.project; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.qualifiedName; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.relation; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.stringLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.values; @@ -70,10 +71,10 @@ public void canBuildSelectAllFromIndex() { } @Test - public void buildSelectFieldsFromIndex() { // TODO: change to select fields later + public void buildSelectFieldsFromIndex() { assertEquals( - project(relation("test"), intLiteral(1)), - buildAST("SELECT 1 FROM test") + project(relation("test"), qualifiedName("age")), + buildAST("SELECT age FROM test") ); } From 448765807fd71d647f155e4abb0a83a25783630c Mon Sep 17 00:00:00 2001 From: Dai Date: Tue, 21 Jul 2020 15:53:31 -0700 Subject: [PATCH 02/27] Add UT --- .../sql/analysis/ExpressionAnalyzerTest.java | 8 ++++++++ docs/user/general/identifiers.rst | 18 +++++++++++++++++- .../resources/correctness/queries/select.txt | 2 +- 3 files changed, 26 insertions(+), 2 deletions(-) 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 b9ff216110..466e069d77 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 @@ -73,6 +73,14 @@ public void not() { ); } + @Test + public void qualified_name() { + assertAnalyzeEqual( + DSL.ref("integer_value", INTEGER), + AstDSL.qualifiedName("integer_value") + ); + } + @Test public void undefined_var_semantic_check_failed() { SemanticCheckException exception = assertThrows(SemanticCheckException.class, diff --git a/docs/user/general/identifiers.rst b/docs/user/general/identifiers.rst index 16b6a105c7..7bc9dddd36 100644 --- a/docs/user/general/identifiers.rst +++ b/docs/user/general/identifiers.rst @@ -103,6 +103,22 @@ For example, if you run ``SELECT * FROM ACCOUNTS``, it will end up with an index Identifier Qualifiers ===================== -For now, we do not support using Elasticsearch cluster name as catalog name to qualify an index name, such as ``my-cluster.logs``. +Description +----------- TODO: field name qualifiers + +Examples +-------- + +Here is an example for ... + + od> SELECT acc.age FROM accounts AS acc; + fetched rows / total rows = 4/4 + + + +Limitations +----------- + +For now, we do not support using Elasticsearch cluster name as catalog name to qualify an index name, such as ``my-cluster.logs``. diff --git a/integ-test/src/test/resources/correctness/queries/select.txt b/integ-test/src/test/resources/correctness/queries/select.txt index adb7f40782..70cca7fa4e 100644 --- a/integ-test/src/test/resources/correctness/queries/select.txt +++ b/integ-test/src/test/resources/correctness/queries/select.txt @@ -1,4 +1,4 @@ SELECT 1 + 2 FROM kibana_sample_data_flights -SELECT abs(-10) FROM kibana_sample_data_flights +SELECT ABS(-10) FROM kibana_sample_data_flights SELECT DistanceMiles FROM kibana_sample_data_flights SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500 From 280dcd2d6854b5de340737811b258f3c63ecf251 Mon Sep 17 00:00:00 2001 From: Dai Date: Tue, 21 Jul 2020 18:07:13 -0700 Subject: [PATCH 03/27] Change grammar and add UT for field alias --- .../antlr/OpenDistroSQLIdentifierParser.g4 | 4 +++ sql/src/main/antlr/OpenDistroSQLParser.g4 | 2 +- .../sql/sql/parser/AstBuilderTest.java | 33 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 b/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 index 7c83d453e3..131722c4e4 100644 --- a/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 +++ b/sql/src/main/antlr/OpenDistroSQLIdentifierParser.g4 @@ -38,6 +38,10 @@ columnName : qualifiedName ; +alias + : ident + ; + qualifiedName : ident (DOT ident)* ; diff --git a/sql/src/main/antlr/OpenDistroSQLParser.g4 b/sql/src/main/antlr/OpenDistroSQLParser.g4 index 13d4539730..edc94651da 100644 --- a/sql/src/main/antlr/OpenDistroSQLParser.g4 +++ b/sql/src/main/antlr/OpenDistroSQLParser.g4 @@ -72,7 +72,7 @@ selectElements ; selectElement - : expression #selectExpressionElement + : expression (AS? alias)? #selectExpressionElement ; fromClause diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java index 6aef402e97..cd37097f13 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java @@ -18,10 +18,13 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.booleanLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.doubleLiteral; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.function; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.intLiteral; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.map; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.project; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.qualifiedName; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.relation; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.rename; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.stringLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.values; import static java.util.Collections.emptyList; @@ -32,6 +35,7 @@ import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.SQLSyntaxParser; import org.antlr.v4.runtime.tree.ParseTree; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; class AstBuilderTest { @@ -78,6 +82,35 @@ public void buildSelectFieldsFromIndex() { ); } + @Test + public void can_build_select_fields_with_alias() { + assertEquals( + rename( + relation("test"), + map( + qualifiedName("age"), + qualifiedName("a") + ) + ), + buildAST("SELECT age AS a FROM test") + ); + } + + @Disabled + @Test + public void can_build_select_function_call_with_alias() { + assertEquals( + rename( + relation("test"), + map( + function("ABS", qualifiedName("age")), + qualifiedName("a") + ) + ), + buildAST("SELECT ABS(age) AS a FROM test") + ); + } + private UnresolvedPlan buildAST(String query) { ParseTree parseTree = parser.parse(query); return parseTree.accept(astBuilder); From da7edda23bdca33f7f710f53d11fa3af01e4d408 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 08:02:44 -0700 Subject: [PATCH 04/27] Support alias by named expression --- .../sql/analysis/ExpressionAnalyzer.java | 7 +++ .../sql/ast/AbstractNodeVisitor.java | 5 +++ .../sql/ast/expression/Alias.java | 37 ++++++++++++++++ .../sql/expression/NamedExpression.java | 44 +++++++++++++++++++ .../sql/planner/physical/ProjectOperator.java | 3 +- sql/src/main/antlr/OpenDistroSQLParser.g4 | 4 +- .../sql/sql/parser/AstBuilder.java | 19 +++++++- 7 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java create mode 100644 core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.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 02a5da41b5..f6f0702f81 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 @@ -19,6 +19,7 @@ import com.amazon.opendistroforelasticsearch.sql.analysis.symbol.Symbol; import com.amazon.opendistroforelasticsearch.sql.ast.AbstractNodeVisitor; import com.amazon.opendistroforelasticsearch.sql.ast.expression.AggregateFunction; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.And; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Compare; import com.amazon.opendistroforelasticsearch.sql.ast.expression.EqualTo; @@ -35,6 +36,7 @@ import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException; import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.amazon.opendistroforelasticsearch.sql.expression.function.BuiltinFunctionName; @@ -155,6 +157,11 @@ public Expression visitQualifiedName(QualifiedName node, AnalysisContext context return visitIdentifier(node.toString(), context); } + @Override + public Expression visitAlias(Alias node, AnalysisContext context) { + return new NamedExpression(node.getName(), node.getDelegate().accept(this, context)); + } + private Expression visitIdentifier(String ident, AnalysisContext context) { TypeEnvironment typeEnv = context.peek(); ReferenceExpression ref = DSL.ref(ident, diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/AbstractNodeVisitor.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/AbstractNodeVisitor.java index cb962c241a..f91212023a 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/AbstractNodeVisitor.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/AbstractNodeVisitor.java @@ -16,6 +16,7 @@ package com.amazon.opendistroforelasticsearch.sql.ast; import com.amazon.opendistroforelasticsearch.sql.ast.expression.AggregateFunction; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.And; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; import com.amazon.opendistroforelasticsearch.sql.ast.expression.AttributeList; @@ -178,4 +179,8 @@ public T visitDedupe(Dedupe node, C context) { public T visitValues(Values node, C context) { return visitChildren(node, context); } + + public T visitAlias(Alias node, C context) { + return visitChildren(node, context); + } } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java new file mode 100644 index 0000000000..2f4f7ad7bd --- /dev/null +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java @@ -0,0 +1,37 @@ +/* + * Copyright 2020 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.ast.expression; + +import com.amazon.opendistroforelasticsearch.sql.ast.AbstractNodeVisitor; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.ToString; + +@Getter +@ToString +@EqualsAndHashCode(callSuper = false) +@RequiredArgsConstructor +public class Alias extends UnresolvedExpression { + private final String name; + private final UnresolvedExpression delegate; + + @Override + public T accept(AbstractNodeVisitor nodeVisitor, C context) { + return nodeVisitor.visitAlias(this, context); + } +} diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java new file mode 100644 index 0000000000..efb3d61b83 --- /dev/null +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java @@ -0,0 +1,44 @@ +/* + * Copyright 2020 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.expression; + +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 lombok.Getter; +import lombok.RequiredArgsConstructor; + +/** + * Named expression that represents expression with name or alias. + */ +@RequiredArgsConstructor +public class NamedExpression implements Expression { + @Getter + private final String name; + private final Expression delegation; + + @Override + public ExprValue valueOf(Environment valueEnv) { + return delegation.valueOf(valueEnv); + } + + @Override + public ExprType type() { + return delegation.type(); + } + +} diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java index 04aa049e57..7f1c14ace6 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java @@ -18,6 +18,7 @@ import com.amazon.opendistroforelasticsearch.sql.data.model.ExprTupleValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMap.Builder; import java.util.Collections; @@ -62,7 +63,7 @@ public ExprValue next() { ExprValue exprValue = expr.valueOf(inputValue.bindingTuples()); // missing value is ignored. if (!exprValue.isMissing()) { - mapBuilder.put(expr.toString(), exprValue); + mapBuilder.put(((NamedExpression) expr).getName(), exprValue); } } return ExprTupleValue.fromExprValueMap(mapBuilder.build()); diff --git a/sql/src/main/antlr/OpenDistroSQLParser.g4 b/sql/src/main/antlr/OpenDistroSQLParser.g4 index dc208b98fc..1a5aa19321 100644 --- a/sql/src/main/antlr/OpenDistroSQLParser.g4 +++ b/sql/src/main/antlr/OpenDistroSQLParser.g4 @@ -68,11 +68,11 @@ selectClause ; selectElements - : (star=STAR | selectElement) (',' selectElement)* + : (star=STAR | selectElement) (COMMA selectElement)* ; selectElement - : expression (AS? alias)? #selectExpressionElement + : expression (AS? alias)? ; fromClause diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index 0774bf769f..1cd48bb03b 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -16,10 +16,13 @@ package com.amazon.opendistroforelasticsearch.sql.sql.parser; +import static com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils.unquoteIdentifier; import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.FromClauseContext; import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.SelectClauseContext; +import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.SelectElementContext; import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.SimpleSelectContext; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; import com.amazon.opendistroforelasticsearch.sql.ast.tree.Project; import com.amazon.opendistroforelasticsearch.sql.ast.tree.Relation; @@ -71,9 +74,9 @@ public UnresolvedPlan visitSelectClause(SelectClauseContext ctx) { return SELECT_ALL; } - List selectElements = ctx.selectElements().children; + List selectElements = ctx.selectElements().selectElement(); return new Project(selectElements.stream() - .map(this::visitAstExpression) + .map(this::visitSelectItem) .filter(Objects::nonNull) .collect(Collectors.toList())); } @@ -92,4 +95,16 @@ private UnresolvedExpression visitAstExpression(ParseTree tree) { return expressionBuilder.visit(tree); } + private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { + UnresolvedExpression delegate = visitAstExpression(ctx.expression()); + if (delegate == null) { + return null; + } + + String name = (ctx.alias() == null) + ? ctx.expression().getText() + : unquoteIdentifier(ctx.alias().getText()); + return new Alias(name, delegate); + } + } From eeaa9587af82cb3fe2a1076cd61c6fc340e7f9fe Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 14:25:12 -0700 Subject: [PATCH 05/27] Add UT --- .../sql/ast/dsl/AstDSL.java | 6 +- .../sql/sql/SQLService.java | 2 +- .../sql/sql/parser/AstBuilder.java | 29 ++++++-- .../sql/sql/antlr/SQLSyntaxParserTest.java | 20 ++++++ .../sql/sql/parser/AstBuilderTest.java | 69 +++++++++++-------- 5 files changed, 91 insertions(+), 35 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 4c62c119d4..5a9b10a22a 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 @@ -16,6 +16,7 @@ package com.amazon.opendistroforelasticsearch.sql.ast.dsl; import com.amazon.opendistroforelasticsearch.sql.ast.expression.AggregateFunction; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.And; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Compare; @@ -43,7 +44,6 @@ import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; import com.amazon.opendistroforelasticsearch.sql.ast.tree.UnresolvedPlan; import com.amazon.opendistroforelasticsearch.sql.ast.tree.Values; -import com.google.common.collect.ImmutableList; import java.util.Arrays; import java.util.List; import lombok.experimental.UtilityClass; @@ -226,6 +226,10 @@ public static Field field(String field, List fieldArgs) { return new Field(field, fieldArgs); } + public Alias alias(String name, UnresolvedExpression expr) { + return new Alias(name, expr); + } + public static List exprList(UnresolvedExpression... exprList) { return Arrays.asList(exprList); } diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java index 599d02bddb..36b275fc2f 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java @@ -95,7 +95,7 @@ public void execute(UnresolvedPlan ast, ResponseListener listener */ public UnresolvedPlan parse(String query) { ParseTree cst = parser.parse(query); - return cst.accept(new AstBuilder()); + return cst.accept(new AstBuilder(query)); } /** diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index 1cd48bb03b..e5217fd4da 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -36,6 +36,7 @@ import java.util.List; import java.util.Objects; import java.util.stream.Collectors; +import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.ParseTree; /** @@ -45,6 +46,19 @@ public class AstBuilder extends OpenDistroSQLParserBaseVisitor { private static final Project SELECT_ALL = null; + /** + * SQL query to get original token name because token.getText() returns text + * without whitespaces or other characters discarded by lexer. + */ + private String query; + + public AstBuilder(String query) { + this.query = query; + } + + public AstBuilder() { + } + private final AstExpressionBuilder expressionBuilder = new AstExpressionBuilder(); @Override @@ -101,10 +115,17 @@ private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { return null; } - String name = (ctx.alias() == null) - ? ctx.expression().getText() - : unquoteIdentifier(ctx.alias().getText()); - return new Alias(name, delegate); + String alias = getAlias(ctx); + return new Alias(unquoteIdentifier(alias), delegate); + } + + private String getAlias(SelectElementContext ctx) { + if (ctx.alias() == null) { + Token start = ctx.expression().getStart(); + Token stop = ctx.expression().getStop(); + return query.substring(start.getStartIndex(), stop.getStopIndex() + 1); + } + return ctx.alias().getText(); } } diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java index 6d3dee9585..5e41306c0d 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -31,6 +31,26 @@ public void canParseSelectLiterals() { assertNotNull(parser.parse("SELECT 123, 'hello'")); } + @Test + public void canParseSelectLiteralWithAlias() { + assertNotNull(parser.parse("SELECT (1 + 2) * 3 AS expr")); + } + + @Test + public void canParseSelectFields() { + assertNotNull(parser.parse("SELECT name, age FROM accounts")); + } + + @Test + public void canParseSelectFieldWithAlias() { + assertNotNull(parser.parse("SELECT name AS n, age AS a FROM accounts")); + } + + @Test + public void canParseSelectFieldWithQuotedAlias() { + assertNotNull(parser.parse("SELECT name AS \"n\", age AS `a` FROM accounts")); + } + @Test public void canParseIndexNameWithDate() { assertNotNull(parser.parse("SELECT * FROM logs_2020_01")); diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java index cd37097f13..bea0d5082e 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java @@ -16,15 +16,14 @@ package com.amazon.opendistroforelasticsearch.sql.sql.parser; +import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.alias; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.booleanLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.doubleLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.function; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.intLiteral; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.map; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.project; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.qualifiedName; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.relation; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.rename; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.stringLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.values; import static java.util.Collections.emptyList; @@ -35,7 +34,6 @@ import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.SQLSyntaxParser; import org.antlr.v4.runtime.tree.ParseTree; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; class AstBuilderTest { @@ -45,27 +43,36 @@ class AstBuilderTest { */ private final SQLSyntaxParser parser = new SQLSyntaxParser(); - /** - * AST builder class that being tested. - */ - private final AstBuilder astBuilder = new AstBuilder(); - @Test - public void canBuildSelectLiterals() { + public void can_build_select_literals() { assertEquals( project( values(emptyList()), - intLiteral(123), - stringLiteral("hello"), - booleanLiteral(false), - doubleLiteral(-4.567) + alias("123", intLiteral(123)), + alias("hello", stringLiteral("hello")), + alias("false", booleanLiteral(false)), + alias("-4.567", doubleLiteral(-4.567)) ), buildAST("SELECT 123, 'hello', false, -4.567") ); } @Test - public void canBuildSelectAllFromIndex() { + public void can_build_select_function_call_with_alias() { + assertEquals( + project( + relation("test"), + alias( + "a", + function("ABS", qualifiedName("age")) + ) + ), + buildAST("SELECT ABS(age) AS a FROM test") + ); + } + + @Test + public void can_build_select_all_from_index() { assertEquals( relation("test"), buildAST("SELECT * FROM test") @@ -75,9 +82,12 @@ public void canBuildSelectAllFromIndex() { } @Test - public void buildSelectFieldsFromIndex() { + public void can_build_select_fields_from_index() { assertEquals( - project(relation("test"), qualifiedName("age")), + project( + relation("test"), + alias("age", qualifiedName("age")) + ), buildAST("SELECT age FROM test") ); } @@ -85,35 +95,36 @@ public void buildSelectFieldsFromIndex() { @Test public void can_build_select_fields_with_alias() { assertEquals( - rename( + project( relation("test"), - map( - qualifiedName("age"), - qualifiedName("a") - ) + alias("a", qualifiedName("age")) ), buildAST("SELECT age AS a FROM test") ); } - @Disabled @Test - public void can_build_select_function_call_with_alias() { + public void can_build_select_fields_with_alias_quoted() { assertEquals( - rename( + project( relation("test"), - map( - function("ABS", qualifiedName("age")), - qualifiedName("a") + alias("first name", qualifiedName("name")), + alias( + "Age_Expr", + function("+", qualifiedName("age"), intLiteral(10)) ) ), - buildAST("SELECT ABS(age) AS a FROM test") + buildAST("SELECT" + + " name AS \"first name\", " + + " (age + 10) AS `Age_Expr` " + + "FROM test" + ) ); } private UnresolvedPlan buildAST(String query) { ParseTree parseTree = parser.parse(query); - return parseTree.accept(astBuilder); + return parseTree.accept(new AstBuilder(query)); } } \ No newline at end of file From 754d9fce46892c850d8ef6ac9379cfee6e968185 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 14:40:03 -0700 Subject: [PATCH 06/27] Add javadoc --- .../sql/ast/expression/Alias.java | 15 +++++++++++++-- .../sql/expression/NamedExpression.java | 10 +++++++++- .../sql/sql/parser/AstBuilder.java | 9 +-------- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java index 2f4f7ad7bd..a730bbdef6 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java @@ -22,12 +22,23 @@ import lombok.RequiredArgsConstructor; import lombok.ToString; -@Getter -@ToString +/** + * Field name alias that wraps an expression with an alias. + */ @EqualsAndHashCode(callSuper = false) +@Getter @RequiredArgsConstructor +@ToString public class Alias extends UnresolvedExpression { + + /** + * Original field name or alias. + */ private final String name; + + /** + * Expression aliased. + */ private final UnresolvedExpression delegate; @Override diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java index efb3d61b83..b0943888b9 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java @@ -19,16 +19,24 @@ 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 lombok.EqualsAndHashCode; import lombok.Getter; import lombok.RequiredArgsConstructor; +import lombok.ToString; /** - * Named expression that represents expression with name or alias. + * Named expression that represents expression with name. This is to preserve + * original expression name or alias in query and avoid inferring its name + * by reconstructing in toString() method. */ +@EqualsAndHashCode @RequiredArgsConstructor +@ToString public class NamedExpression implements Expression { + @Getter private final String name; + private final Expression delegation; @Override diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index e5217fd4da..fb7fc9a693 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -34,7 +34,6 @@ import com.google.common.collect.ImmutableList; import java.util.Collections; import java.util.List; -import java.util.Objects; import java.util.stream.Collectors; import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.ParseTree; @@ -91,7 +90,6 @@ public UnresolvedPlan visitSelectClause(SelectClauseContext ctx) { List selectElements = ctx.selectElements().selectElement(); return new Project(selectElements.stream() .map(this::visitSelectItem) - .filter(Objects::nonNull) .collect(Collectors.toList())); } @@ -111,12 +109,7 @@ private UnresolvedExpression visitAstExpression(ParseTree tree) { private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { UnresolvedExpression delegate = visitAstExpression(ctx.expression()); - if (delegate == null) { - return null; - } - - String alias = getAlias(ctx); - return new Alias(unquoteIdentifier(alias), delegate); + return new Alias(unquoteIdentifier(getAlias(ctx)), delegate); } private String getAlias(SelectElementContext ctx) { From a31a8d018b4aa49e0720eca82cfb7bf9dfda41af Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 14:48:47 -0700 Subject: [PATCH 07/27] Make query field required after test --- .../sql/sql/parser/AstBuilder.java | 15 +++++---------- .../sql/sql/SQLServiceTest.java | 4 ++-- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index fb7fc9a693..a44c36d5ad 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -35,30 +35,25 @@ import java.util.Collections; import java.util.List; import java.util.stream.Collectors; +import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.ParseTree; /** * Abstract syntax tree (AST) builder. */ +@RequiredArgsConstructor public class AstBuilder extends OpenDistroSQLParserBaseVisitor { private static final Project SELECT_ALL = null; + private final AstExpressionBuilder expressionBuilder = new AstExpressionBuilder(); + /** * SQL query to get original token name because token.getText() returns text * without whitespaces or other characters discarded by lexer. */ - private String query; - - public AstBuilder(String query) { - this.query = query; - } - - public AstBuilder() { - } - - private final AstExpressionBuilder expressionBuilder = new AstExpressionBuilder(); + private final String query; @Override public UnresolvedPlan visitSimpleSelect(SimpleSelectContext ctx) { diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java index ce563414ff..3291ccc1ce 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java @@ -95,7 +95,7 @@ public void canExecuteFromAst() { }).when(executionEngine).execute(any(), any()); ParseTree parseTree = new SQLSyntaxParser().parse("SELECT 123"); - UnresolvedPlan ast = parseTree.accept(new AstBuilder()); + UnresolvedPlan ast = parseTree.accept(new AstBuilder("SELECT 123")); sqlService.execute(ast, new ResponseListener() { @@ -133,7 +133,7 @@ public void canCaptureErrorDuringExecutionFromAst() { doThrow(new RuntimeException()).when(executionEngine).execute(any(), any()); ParseTree parseTree = new SQLSyntaxParser().parse("SELECT 123"); - UnresolvedPlan ast = parseTree.accept(new AstBuilder()); + UnresolvedPlan ast = parseTree.accept(new AstBuilder("SELECT 123")); sqlService.execute(ast, new ResponseListener() { From 3a5f7053eca9e134f63e7189b28ca7f89c031421 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 15:00:39 -0700 Subject: [PATCH 08/27] Change java doc --- .../sql/ast/expression/Alias.java | 2 +- .../sql/expression/NamedExpression.java | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java index a730bbdef6..cfe2dd11f0 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java @@ -23,7 +23,7 @@ import lombok.ToString; /** - * Field name alias that wraps an expression with an alias. + * Field name alias abstraction that associate an expression with a name. */ @EqualsAndHashCode(callSuper = false) @Getter diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java index b0943888b9..f183bbc77f 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java @@ -34,9 +34,15 @@ @ToString public class NamedExpression implements Expression { + /** + * Expression name. + */ @Getter private final String name; + /** + * Expression that being named. + */ private final Expression delegation; @Override From 4317a8006a1e835129ae3b442f4d3e5dd825e12c Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 17:06:12 -0700 Subject: [PATCH 09/27] Fix comparison test data type issue --- .../correctness/runner/ComparisonTest.java | 6 +- .../runner/connection/DBConnection.java | 2 +- .../runner/connection/ESConnection.java | 6 +- .../runner/connection/JDBCConnection.java | 7 ++- .../sql/correctness/testset/TestDataSet.java | 58 ++++++++++++++++++- .../correctness/expressions/arithmetics.txt | 2 + .../resources/correctness/queries/select.txt | 2 +- 7 files changed, 69 insertions(+), 14 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/ComparisonTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/ComparisonTest.java index 333d864b06..53d040dd6b 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/ComparisonTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/ComparisonTest.java @@ -156,9 +156,9 @@ private int nextId() { return testCaseId++; } - private void insertTestDataInBatch(DBConnection conn, String tableName, List testData) { - Iterator iterator = testData.iterator(); - String[] fieldNames = iterator.next(); // first row is header of column names + private void insertTestDataInBatch(DBConnection conn, String tableName, List testData) { + Iterator iterator = testData.iterator(); + String[] fieldNames = (String[]) iterator.next(); // first row is header of column names Iterators.partition(iterator, 100). forEachRemaining(batch -> conn.insert(tableName, fieldNames, batch)); } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/DBConnection.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/DBConnection.java index 6779398be4..46eff96b6f 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/DBConnection.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/DBConnection.java @@ -48,7 +48,7 @@ public interface DBConnection { * @param columnNames column names * @param batch batch of rows */ - void insert(String tableName, String[] columnNames, List batch); + void insert(String tableName, String[] columnNames, List batch); /** * Fetch data from database. diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java index 4ce4a7bde2..04dbecae0f 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java @@ -66,7 +66,7 @@ public void drop(String tableName) { } @Override - public void insert(String tableName, String[] columnNames, List batch) { + public void insert(String tableName, String[] columnNames, List batch) { Request request = new Request("POST", "/" + tableName + "/_bulk?refresh=true"); request.setJsonEntity(buildBulkBody(columnNames, batch)); performRequest(request); @@ -96,9 +96,9 @@ private void performRequest(Request request) { } } - private String buildBulkBody(String[] columnNames, List batch) { + private String buildBulkBody(String[] columnNames, List batch) { StringBuilder body = new StringBuilder(); - for (String[] fieldValues : batch) { + for (Object[] fieldValues : batch) { JSONObject json = new JSONObject(); for (int i = 0; i < columnNames.length; i++) { json.put(columnNames[i], fieldValues[i]); diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/JDBCConnection.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/JDBCConnection.java index ed731330b1..2aff6b982c 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/JDBCConnection.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/JDBCConnection.java @@ -93,10 +93,10 @@ public void drop(String tableName) { } @Override - public void insert(String tableName, String[] columnNames, List batch) { + public void insert(String tableName, String[] columnNames, List batch) { try (Statement stmt = connection.createStatement()) { String names = String.join(",", columnNames); - for (String[] fieldValues : batch) { + for (Object[] fieldValues : batch) { stmt.addBatch(StringUtils.format( "INSERT INTO %s(%s) VALUES (%s)", tableName, names, getValueList(fieldValues))); } @@ -139,8 +139,9 @@ private String parseColumnNameAndTypesInSchemaJson(String schema) { collect(joining(",")); } - private String getValueList(String[] fieldValues) { + private String getValueList(Object[] fieldValues) { return Arrays.stream(fieldValues). + map(String::valueOf). map(val -> val.replace(SINGLE_QUOTE, DOUBLE_QUOTE)). map(val -> SINGLE_QUOTE + val + SINGLE_QUOTE). collect(joining(",")); diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java index ab5b77dc09..d287ca26c3 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java @@ -18,9 +18,11 @@ import static com.amazon.opendistroforelasticsearch.sql.legacy.utils.StringUtils.unquoteSingleField; import static java.util.stream.Collectors.joining; +import com.amazon.opendistroforelasticsearch.sql.legacy.utils.StringUtils; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import org.json.JSONObject; /** * Test data set @@ -29,12 +31,12 @@ public class TestDataSet { private final String tableName; private final String schema; - private final List dataRows; + private final List dataRows; public TestDataSet(String tableName, String schemaFileContent, String dataFileContent) { this.tableName = tableName; this.schema = schemaFileContent; - this.dataRows = splitColumns(dataFileContent, ','); + this.dataRows = convertColumnStringToObject(splitColumns(dataFileContent, ',')); } public String getTableName() { @@ -45,7 +47,7 @@ public String getSchema() { return schema; } - public List getDataRows() { + public List getDataRows() { return dataRows; } @@ -82,6 +84,56 @@ private List splitColumns(String content, char separator) { return result; } + /** + * Convert column string values (read from CSV file) to objects of its real type + * based on the type information in index mapping file. + */ + private List convertColumnStringToObject(List rows) { + JSONObject types = new JSONObject(schema); + String[] columnNames = rows.get(0); + + List result = new ArrayList<>(); + result.add(columnNames); + + rows.stream() + .skip(1) + .map(row -> convertStringValues(types, columnNames, row)) + .forEach(result::add); + return result; + } + + private Object[] convertStringValues(JSONObject types, String[] columnNames, String[] row) { + Object[] result = new Object[row.length]; + for (int i = 0; i < row.length; i++) { + String colName = columnNames[i]; + String colTypePath = "/mappings/properties/" + colName; + String colType = ((JSONObject) types.query(colTypePath)).getString("type"); + result[i] = convertStringValue(colType, row[i]); + } + return result; + } + + private Object convertStringValue(String type, String str) { + switch (type.toLowerCase()) { + case "text": + case "keyword": + case "date": + return str; + case "integer": + return Integer.valueOf(str); + case "float": + case "half_float": + return Float.valueOf(str); + case "double": + return Double.valueOf(str); + case "boolean": + return Boolean.valueOf(str); + default: + throw new IllegalStateException(StringUtils.format( + "Data type %s is not supported yet for value: %s", type, str)); + } + } + @Override public String toString() { int total = dataRows.size(); diff --git a/integ-test/src/test/resources/correctness/expressions/arithmetics.txt b/integ-test/src/test/resources/correctness/expressions/arithmetics.txt index 64872a4df3..c3730393e7 100644 --- a/integ-test/src/test/resources/correctness/expressions/arithmetics.txt +++ b/integ-test/src/test/resources/correctness/expressions/arithmetics.txt @@ -9,3 +9,5 @@ 5 % 2 -5 % 2 0 % 2 +(2 / 1) +(5 - 1.2) * 3 \ No newline at end of file diff --git a/integ-test/src/test/resources/correctness/queries/select.txt b/integ-test/src/test/resources/correctness/queries/select.txt index 70cca7fa4e..daa3b39ab1 100644 --- a/integ-test/src/test/resources/correctness/queries/select.txt +++ b/integ-test/src/test/resources/correctness/queries/select.txt @@ -1,4 +1,4 @@ SELECT 1 + 2 FROM kibana_sample_data_flights SELECT ABS(-10) FROM kibana_sample_data_flights -SELECT DistanceMiles FROM kibana_sample_data_flights +SELECT ABS(DistanceMiles) FROM kibana_sample_data_flights SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500 From 55348d0b1a91752f03ca144f0cd53667fe49471c Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 17:21:12 -0700 Subject: [PATCH 10/27] Fix broken UT --- .../opendistroforelasticsearch/sql/expression/DSL.java | 4 ++++ .../sql/planner/physical/ProjectOperatorTest.java | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java index 34a648b385..b91c66a4b9 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java @@ -75,6 +75,10 @@ public static ReferenceExpression ref(String ref, ExprType type) { return new ReferenceExpression(ref, type); } + public static NamedExpression named(String name, Expression expression) { + return new NamedExpression(name, expression); + } + public FunctionExpression abs(Expression... expressions) { return function(BuiltinFunctionName.ABS, expressions); } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperatorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperatorTest.java index 09d97b19d8..ec6af198d7 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperatorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperatorTest.java @@ -45,7 +45,7 @@ public void project_one_field() { when(inputPlan.hasNext()).thenReturn(true, false); when(inputPlan.next()) .thenReturn(ExprValueUtils.tupleValue(ImmutableMap.of("action", "GET", "response", 200))); - PhysicalPlan plan = project(inputPlan, DSL.ref("action", STRING)); + PhysicalPlan plan = project(inputPlan, DSL.named("action", DSL.ref("action", STRING))); List result = execute(plan); assertThat( @@ -60,7 +60,9 @@ public void project_two_field_follow_the_project_order() { when(inputPlan.hasNext()).thenReturn(true, false); when(inputPlan.next()) .thenReturn(ExprValueUtils.tupleValue(ImmutableMap.of("action", "GET", "response", 200))); - PhysicalPlan plan = project(inputPlan, DSL.ref("response", INTEGER), DSL.ref("action", STRING)); + PhysicalPlan plan = project(inputPlan, + DSL.named("response", DSL.ref("response", INTEGER)), + DSL.named("action", DSL.ref("action", STRING))); List result = execute(plan); assertThat( @@ -77,7 +79,9 @@ public void project_ignore_missing_value() { when(inputPlan.next()) .thenReturn(ExprValueUtils.tupleValue(ImmutableMap.of("action", "GET", "response", 200))) .thenReturn(ExprValueUtils.tupleValue(ImmutableMap.of("action", "POST"))); - PhysicalPlan plan = project(inputPlan, DSL.ref("response", INTEGER), DSL.ref("action", STRING)); + PhysicalPlan plan = project(inputPlan, + DSL.named("response", DSL.ref("response", INTEGER)), + DSL.named("action", DSL.ref("action", STRING))); List result = execute(plan); assertThat( From 0365199721a2b506b2105e02a5c444562f9fa5e5 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 17:30:49 -0700 Subject: [PATCH 11/27] Fix jacoco coverage --- .../sql/analysis/ExpressionAnalyzerTest.java | 8 ++++ .../sql/expression/NamedExpressionTest.java | 38 +++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java 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 466e069d77..d60b9eaaf9 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 @@ -80,6 +80,14 @@ public void qualified_name() { AstDSL.qualifiedName("integer_value") ); } + + @Test + public void named_expression() { + assertAnalyzeEqual( + DSL.named("int", DSL.ref("integer_value", INTEGER)), + AstDSL.alias("int", AstDSL.qualifiedName("integer_value")) + ); + } @Test public void undefined_var_semantic_check_failed() { diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java new file mode 100644 index 0000000000..a1864c84f3 --- /dev/null +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java @@ -0,0 +1,38 @@ +/* + * Copyright 2020 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.expression; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; +import org.junit.jupiter.api.Test; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) +class NamedExpressionTest extends ExpressionTestBase { + + @Test + void name_an_expression() { + LiteralExpression delegate = DSL.literal(10); + NamedExpression namedExpression = DSL.named("int", delegate); + + assertEquals("int", namedExpression.getName()); + assertEquals(delegate.type(), namedExpression.type()); + assertEquals(delegate.valueOf(valueEnv()), namedExpression.valueOf(valueEnv())); + } + +} \ No newline at end of file From 75b73199a8cda39fdfe96f7825cf85fd12f7fb69 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 21:38:00 -0700 Subject: [PATCH 12/27] Fix broken UTs --- .../sql/analysis/Analyzer.java | 5 ++-- .../sql/planner/logical/LogicalPlanDSL.java | 3 +- .../sql/planner/logical/LogicalProject.java | 4 +-- .../sql/planner/physical/PhysicalPlanDSL.java | 3 +- .../sql/planner/physical/ProjectOperator.java | 7 ++--- .../sql/analysis/AnalyzerTest.java | 28 ++++++++++--------- .../sql/planner/DefaultImplementorTest.java | 4 ++- .../sql/planner/PlannerTest.java | 12 ++++---- .../logical/LogicalPlanNodeVisitorTest.java | 3 +- .../physical/PhysicalPlanNodeVisitorTest.java | 5 ++-- 10 files changed, 41 insertions(+), 33 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java index 03202b34ff..7fbb27344b 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java @@ -40,6 +40,7 @@ import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; import com.amazon.opendistroforelasticsearch.sql.expression.LiteralExpression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalAggregation; @@ -166,8 +167,8 @@ public LogicalPlan visitProject(Project node, AnalysisContext context) { } } - List expressions = node.getProjectList().stream() - .map(expr -> expressionAnalyzer.analyze(expr, context)) + List expressions = node.getProjectList().stream() + .map(expr -> (NamedExpression) expressionAnalyzer.analyze(expr, context)) .collect(Collectors.toList()); return new LogicalProject(child, expressions); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java index 2f828ad9e6..1066b279fc 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java @@ -18,6 +18,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort.SortOption; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; import com.amazon.opendistroforelasticsearch.sql.expression.LiteralExpression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.google.common.collect.ImmutableSet; @@ -50,7 +51,7 @@ public static LogicalPlan rename( return new LogicalRename(input, renameMap); } - public static LogicalPlan project(LogicalPlan input, Expression... fields) { + public static LogicalPlan project(LogicalPlan input, NamedExpression... fields) { return new LogicalProject(input, Arrays.asList(fields)); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalProject.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalProject.java index edf179903c..a68b176d60 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalProject.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalProject.java @@ -15,7 +15,7 @@ package com.amazon.opendistroforelasticsearch.sql.planner.logical; -import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import java.util.Arrays; import java.util.List; import lombok.EqualsAndHashCode; @@ -32,7 +32,7 @@ public class LogicalProject extends LogicalPlan { private final LogicalPlan child; @Getter - private final List projectList; + private final List projectList; @Override public List getChild() { diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java index 6dedd39e04..40a8348976 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java @@ -18,6 +18,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort.SortOption; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; import com.amazon.opendistroforelasticsearch.sql.expression.LiteralExpression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.google.common.collect.ImmutableSet; @@ -47,7 +48,7 @@ public static RenameOperator rename( return new RenameOperator(input, renameMap); } - public static ProjectOperator project(PhysicalPlan input, Expression... fields) { + public static ProjectOperator project(PhysicalPlan input, NamedExpression... fields) { return new ProjectOperator(input, Arrays.asList(fields)); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java index 7f1c14ace6..28c1de18dc 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/ProjectOperator.java @@ -17,7 +17,6 @@ import com.amazon.opendistroforelasticsearch.sql.data.model.ExprTupleValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; -import com.amazon.opendistroforelasticsearch.sql.expression.Expression; import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMap.Builder; @@ -38,7 +37,7 @@ public class ProjectOperator extends PhysicalPlan { @Getter private final PhysicalPlan input; @Getter - private final List projectList; + private final List projectList; @Override public R accept(PhysicalPlanNodeVisitor visitor, C context) { @@ -59,11 +58,11 @@ public boolean hasNext() { public ExprValue next() { ExprValue inputValue = input.next(); ImmutableMap.Builder mapBuilder = new Builder<>(); - for (Expression expr : projectList) { + for (NamedExpression expr : projectList) { ExprValue exprValue = expr.valueOf(inputValue.bindingTuples()); // missing value is ignored. if (!exprValue.isMissing()) { - mapBuilder.put(((NamedExpression) expr).getName(), exprValue); + mapBuilder.put(expr.getName(), exprValue); } } return ExprTupleValue.fromExprValueMap(mapBuilder.build()); diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java index b7ffc28387..c6b57ba792 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java @@ -131,13 +131,15 @@ public void rename_to_invalid_expression() { public void project_source() { assertAnalyzeEqual( LogicalPlanDSL.project( - LogicalPlanDSL.relation("schema"), DSL.ref("integer_value", INTEGER), DSL.ref( - "double_value", DOUBLE)), + LogicalPlanDSL.relation("schema"), + DSL.named("integer_value", DSL.ref("integer_value", INTEGER)), + DSL.named("double_value", DSL.ref("double_value", DOUBLE)) + ), AstDSL.projectWithArg( AstDSL.relation("schema"), AstDSL.defaultFieldsArgs(), - AstDSL.field("integer_value"), - AstDSL.field("double_value"))); + AstDSL.alias("integer_value", AstDSL.field("integer_value")), + AstDSL.alias("double_value", AstDSL.field("double_value")))); } @Test @@ -165,10 +167,10 @@ public void project_source_change_type_env() { AstDSL.projectWithArg( AstDSL.relation("schema"), AstDSL.defaultFieldsArgs(), - AstDSL.field("integer_value"), - AstDSL.field("double_value")), + AstDSL.alias("integer_value", AstDSL.field("integer_value")), + AstDSL.alias("double_value", AstDSL.field("double_value"))), AstDSL.defaultFieldsArgs(), - AstDSL.field("float_value")))); + AstDSL.alias("float_value", AstDSL.field("float_value"))))); } @Test @@ -176,15 +178,15 @@ public void project_values() { assertAnalyzeEqual( LogicalPlanDSL.project( LogicalPlanDSL.values(ImmutableList.of(DSL.literal(123))), - DSL.literal(123), - DSL.literal("hello"), - DSL.literal(false) + DSL.named("123", DSL.literal(123)), + DSL.named("hello", DSL.literal("hello")), + DSL.named("false", DSL.literal(false)) ), AstDSL.project( AstDSL.values(ImmutableList.of(AstDSL.intLiteral(123))), - AstDSL.intLiteral(123), - AstDSL.stringLiteral("hello"), - AstDSL.booleanLiteral(false) + AstDSL.alias("123", AstDSL.intLiteral(123)), + AstDSL.alias("hello", AstDSL.stringLiteral("hello")), + AstDSL.alias("false", AstDSL.booleanLiteral(false)) ) ); } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java index ce0495c97b..7343c3c9b5 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java @@ -19,6 +19,7 @@ import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.STRING; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.literal; +import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.named; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.aggregation; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.eval; @@ -36,6 +37,7 @@ import com.amazon.opendistroforelasticsearch.sql.data.model.ExprBooleanValue; import com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.AvgAggregator; @@ -59,7 +61,7 @@ class DefaultImplementorTest { @Test public void visitShouldReturnDefaultPhysicalOperator() { String indexName = "test"; - ReferenceExpression include = ref("age", INTEGER); + NamedExpression include = named("age", ref("age", INTEGER)); ReferenceExpression exclude = ref("name", STRING); ReferenceExpression dedupeField = ref("name", STRING); Expression filterExpr = literal(ExprBooleanValue.ofTrue()); diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/PlannerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/PlannerTest.java index eddd35493e..33e14cf902 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/PlannerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/PlannerTest.java @@ -98,15 +98,15 @@ public void plan_a_query_without_relation_involved() { assertPhysicalPlan( PhysicalPlanDSL.project( PhysicalPlanDSL.values(emptyList()), - DSL.literal(123), - DSL.literal("hello"), - DSL.literal(false) + DSL.named("123", DSL.literal(123)), + DSL.named("hello", DSL.literal("hello")), + DSL.named("false", DSL.literal(false)) ), LogicalPlanDSL.project( LogicalPlanDSL.values(emptyList()), - DSL.literal(123), - DSL.literal("hello"), - DSL.literal(false) + DSL.named("123", DSL.literal(123)), + DSL.named("hello", DSL.literal("hello")), + DSL.named("false", DSL.literal(false)) ) ); } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java index 4f9dcb2107..fd360850c6 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java @@ -15,6 +15,7 @@ package com.amazon.opendistroforelasticsearch.sql.planner.logical; +import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.named; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; @@ -77,7 +78,7 @@ public void testAbstractPlanNodeVisitorShouldReturnNull() { assertNull(rename.accept(new LogicalPlanNodeVisitor() { }, null)); - LogicalPlan project = LogicalPlanDSL.project(relation, ref); + LogicalPlan project = LogicalPlanDSL.project(relation, named("ref", ref)); assertNull(project.accept(new LogicalPlanNodeVisitor() { }, null)); diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java index 4a6de485cc..2feaf09f7f 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java @@ -17,6 +17,7 @@ import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.DOUBLE; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; +import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.named; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; @@ -56,7 +57,7 @@ public void print_physical_plan() { ImmutableList.of(dsl.avg(DSL.ref("response", INTEGER))), ImmutableList.of()), ImmutableMap.of(DSL.ref("ivalue", INTEGER), DSL.ref("avg(response)", DOUBLE))), - ref), + named("ref", ref)), ref); PhysicalPlanPrinter printer = new PhysicalPlanPrinter(); @@ -90,7 +91,7 @@ public void test_PhysicalPlanVisitor_should_return_null() { assertNull(rename.accept(new PhysicalPlanNodeVisitor() { }, null)); - PhysicalPlan project = PhysicalPlanDSL.project(plan, ref); + PhysicalPlan project = PhysicalPlanDSL.project(plan, named("ref", ref)); assertNull(project.accept(new PhysicalPlanNodeVisitor() { }, null)); From 4a06c175feaff28a4443c722237e951f26a004ec Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 23 Jul 2020 21:49:57 -0700 Subject: [PATCH 13/27] Fix PPL ast builder --- .../opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 9924f3ac12..004da009c6 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 @@ -28,6 +28,7 @@ import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.StatsCommandContext; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.WhereCommandContext; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Field; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Let; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Map; @@ -103,7 +104,8 @@ public UnresolvedPlan visitFieldsCommand(FieldsCommandContext ctx) { ctx.wcFieldList() .wcFieldExpression() .stream() - .map(this::visitExpression) + .map(field -> (Field) visitExpression(field)) + .map(field -> new Alias(field.getField().toString(), field)) .collect(Collectors.toList()), ArgumentFactory.getArgumentList(ctx) ); From 391d5c5caf3e882bce63f48336b71c6f807ed705 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 24 Jul 2020 09:17:35 -0700 Subject: [PATCH 14/27] Add doctest --- docs/user/general/identifiers.rst | 5 ----- 1 file changed, 5 deletions(-) diff --git a/docs/user/general/identifiers.rst b/docs/user/general/identifiers.rst index 7bc9dddd36..82b7afd21e 100644 --- a/docs/user/general/identifiers.rst +++ b/docs/user/general/identifiers.rst @@ -113,11 +113,6 @@ Examples Here is an example for ... - od> SELECT acc.age FROM accounts AS acc; - fetched rows / total rows = 4/4 - - - Limitations ----------- From c20b011be586b6591696530d0c3103606b7490c5 Mon Sep 17 00:00:00 2001 From: Dai Date: Tue, 28 Jul 2020 13:08:28 -0700 Subject: [PATCH 15/27] Fix broken legacy IT --- .../sql/analysis/ExpressionAnalyzer.java | 13 ++++++ docs/user/dql/functions.rst | 44 +++++++++---------- .../sql/legacy/QueryIT.java | 1 + .../sql/sql/MathematicalFunctionIT.java | 2 +- .../sql/legacy/plugin/RestSQLQueryAction.java | 13 ++++-- .../sql/legacy/plugin/RestSqlAction.java | 8 +++- .../sql/sql/SQLService.java | 23 +++++++--- .../sql/sql/SQLServiceTest.java | 20 +++------ 8 files changed, 76 insertions(+), 48 deletions(-) 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 f6f0702f81..61a8339fd7 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 @@ -32,7 +32,9 @@ import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedAttribute; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Xor; +import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValueUtils; +import com.amazon.opendistroforelasticsearch.sql.data.type.ExprType; import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException; import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; @@ -166,6 +168,17 @@ private Expression visitIdentifier(String ident, AnalysisContext context) { TypeEnvironment typeEnv = context.peek(); ReferenceExpression ref = DSL.ref(ident, typeEnv.resolve(new Symbol(Namespace.FIELD_NAME, ident))); + + if (isTypeNotSupported(ref.type())) { + throw new SyntaxCheckException(String.format( + "Identifier [%s] of type [%s] is not supported yet,", ident, ref.type())); + } return ref; } + + private boolean isTypeNotSupported(ExprType type) { + return "struct".equalsIgnoreCase(type.typeName()) + || "array".equalsIgnoreCase(type.typeName()); + } + } diff --git a/docs/user/dql/functions.rst b/docs/user/dql/functions.rst index a8d97fde7c..85ed109425 100644 --- a/docs/user/dql/functions.rst +++ b/docs/user/dql/functions.rst @@ -44,7 +44,7 @@ Example:: od> SELECT ACOS(0) fetched rows / total rows = 1/1 +--------------------+ - | acos(0) | + | ACOS(0) | |--------------------| | 1.5707963267948966 | +--------------------+ @@ -89,7 +89,7 @@ Example:: od> SELECT ASIN(0) fetched rows / total rows = 1/1 +-----------+ - | asin(0) | + | ASIN(0) | |-----------| | 0 | +-----------+ @@ -112,7 +112,7 @@ Example:: od> SELECT ATAN(2), ATAN(2, 3) fetched rows / total rows = 1/1 +--------------------+--------------------+ - | atan(2) | atan(2, 3) | + | ATAN(2) | ATAN(2, 3) | |--------------------+--------------------| | 1.1071487177940904 | 0.5880026035475675 | +--------------------+--------------------+ @@ -135,7 +135,7 @@ Example:: od> SELECT ATAN2(2, 3) fetched rows / total rows = 1/1 +--------------------+ - | atan2(2, 3) | + | ATAN2(2, 3) | |--------------------| | 0.5880026035475675 | +--------------------+ @@ -205,7 +205,7 @@ Example:: od> SELECT CONV('12', 10, 16), CONV('2C', 16, 10), CONV(12, 10, 2), CONV(1111, 2, 10) fetched rows / total rows = 1/1 +----------------------+----------------------+-------------------+---------------------+ - | conv("12", 10, 16) | conv("2C", 16, 10) | conv(12, 10, 2) | conv(1111, 2, 10) | + | CONV('12', 10, 16) | CONV('2C', 16, 10) | CONV(12, 10, 2) | CONV(1111, 2, 10) | |----------------------+----------------------+-------------------+---------------------| | c | 44 | 1100 | 15 | +----------------------+----------------------+-------------------+---------------------+ @@ -227,7 +227,7 @@ Example:: od> SELECT COS(0) fetched rows / total rows = 1/1 +----------+ - | cos(0) | + | COS(0) | |----------| | 1 | +----------+ @@ -261,7 +261,7 @@ Example:: od> SELECT COT(1) fetched rows / total rows = 1/1 +--------------------+ - | cot(1) | + | COT(1) | |--------------------| | 0.6420926159343306 | +--------------------+ @@ -284,7 +284,7 @@ Example:: od> SELECT CRC32('MySQL') fetched rows / total rows = 1/1 +------------------+ - | crc32("MySQL") | + | CRC32('MySQL') | |------------------| | 3259397556 | +------------------+ @@ -352,7 +352,7 @@ Example:: od> SELECT DEGREES(1.57) fetched rows / total rows = 1/1 +-------------------+ - | degrees(1.57) | + | DEGREES(1.57) | |-------------------| | 89.95437383553924 | +-------------------+ @@ -384,7 +384,7 @@ Example:: od> SELECT E() fetched rows / total rows = 1/1 +-------------------+ - | e() | + | E() | |-------------------| | 2.718281828459045 | +-------------------+ @@ -586,7 +586,7 @@ Example:: od> SELECT MOD(3, 2), MOD(3.1, 2) fetched rows / total rows = 1/1 +-------------+---------------+ - | mod(3, 2) | mod(3.1, 2) | + | MOD(3, 2) | MOD(3.1, 2) | |-------------+---------------| | 1 | 1.1 | +-------------+---------------+ @@ -651,7 +651,7 @@ Example:: od> SELECT PI() fetched rows / total rows = 1/1 +-------------------+ - | pi() | + | PI() | |-------------------| | 3.141592653589793 | +-------------------+ @@ -674,7 +674,7 @@ Example:: od> SELECT POW(3, 2), POW(-3, 2), POW(3, -2) fetched rows / total rows = 1/1 +-------------+--------------+--------------------+ - | pow(3, 2) | pow(-3, 2) | pow(3, -2) | + | POW(3, 2) | POW(-3, 2) | POW(3, -2) | |-------------+--------------+--------------------| | 9 | 9 | 0.1111111111111111 | +-------------+--------------+--------------------+ @@ -697,7 +697,7 @@ Example:: od> SELECT POWER(3, 2), POWER(-3, 2), POWER(3, -2) fetched rows / total rows = 1/1 +---------------+----------------+--------------------+ - | power(3, 2) | power(-3, 2) | power(3, -2) | + | POWER(3, 2) | POWER(-3, 2) | POWER(3, -2) | |---------------+----------------+--------------------| | 9 | 9 | 0.1111111111111111 | +---------------+----------------+--------------------+ @@ -720,7 +720,7 @@ Example:: od> SELECT RADIANS(90) fetched rows / total rows = 1/1 +--------------------+ - | radians(90) | + | RADIANS(90) | |--------------------| | 1.5707963267948966 | +--------------------+ @@ -743,7 +743,7 @@ Example:: od> SELECT RAND(3) fetched rows / total rows = 1/1 +------------+ - | rand(3) | + | RAND(3) | |------------| | 0.73105735 | +------------+ @@ -802,7 +802,7 @@ Example:: od> SELECT ROUND(12.34), ROUND(12.34, 1), ROUND(12.34, -1), ROUND(12, 1) fetched rows / total rows = 1/1 +----------------+-------------------+--------------------+----------------+ - | round(12.34) | round(12.34, 1) | round(12.34, -1) | round(12, 1) | + | ROUND(12.34) | ROUND(12.34, 1) | ROUND(12.34, -1) | ROUND(12, 1) | |----------------+-------------------+--------------------+----------------| | 12 | 12.3 | 10 | 12 | +----------------+-------------------+--------------------+----------------+ @@ -836,7 +836,7 @@ Example:: od> SELECT SIGN(1), SIGN(0), SIGN(-1.1) fetched rows / total rows = 1/1 +-----------+-----------+--------------+ - | sign(1) | sign(0) | sign(-1.1) | + | SIGN(1) | SIGN(0) | SIGN(-1.1) | |-----------+-----------+--------------| | 1 | 0 | -1 | +-----------+-----------+--------------+ @@ -870,7 +870,7 @@ Example:: od> SELECT SIN(0) fetched rows / total rows = 1/1 +----------+ - | sin(0) | + | SIN(0) | |----------| | 0 | +----------+ @@ -907,7 +907,7 @@ Example:: od> SELECT SQRT(4), SQRT(4.41) fetched rows / total rows = 1/1 +-----------+--------------+ - | sqrt(4) | sqrt(4.41) | + | SQRT(4) | SQRT(4.41) | |-----------+--------------| | 2 | 2.1 | +-----------+--------------+ @@ -952,7 +952,7 @@ Example:: od> SELECT TAN(0) fetched rows / total rows = 1/1 +----------+ - | tan(0) | + | TAN(0) | |----------| | 0 | +----------+ @@ -999,7 +999,7 @@ Example:: fetched rows / total rows = 1/1 +----------------------+-----------------------+-------------------+ - | truncate(56.78, 1) | truncate(56.78, -1) | truncate(56, 1) | + | TRUNCATE(56.78, 1) | TRUNCATE(56.78, -1) | TRUNCATE(56, 1) | |----------------------+-----------------------+-------------------| | 56.7 | 50 | 56 | +----------------------+-----------------------+-------------------+ diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java index f4a4c36a16..94314cfd32 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java @@ -1749,6 +1749,7 @@ public void functionInCaseFieldShouldThrowESExceptionDueToIllegalScriptInJdbc() "For more details, please send request for Json format"); } + @Ignore("This is already supported in our new query engine") @Test public void functionCallWithIllegalScriptShouldThrowESExceptionInJdbc() { String response = executeQuery("select log(balance + 2) from " + TEST_INDEX_BANK, diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java index 5bfadb3c6f..ffc33c1324 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java @@ -54,7 +54,7 @@ public void testConv() throws IOException { @Test public void testCrc32() throws IOException { JSONObject result = executeQuery("select crc32('MySQL')"); - verifySchema(result, schema("crc32(\"MySQL\")", null, "long")); + verifySchema(result, schema("crc32('MySQL')", null, "long")); verifyDataRows(result, rows(3259397556L)); } diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java index 6f243e684b..70eb40cb8c 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -21,10 +21,11 @@ import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR; import static org.elasticsearch.rest.RestStatus.OK; -import com.amazon.opendistroforelasticsearch.sql.ast.tree.UnresolvedPlan; import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; import com.amazon.opendistroforelasticsearch.sql.common.response.ResponseListener; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.security.SecurityAccess; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; +import com.amazon.opendistroforelasticsearch.sql.planner.physical.PhysicalPlan; import com.amazon.opendistroforelasticsearch.sql.protocol.response.QueryResult; import com.amazon.opendistroforelasticsearch.sql.protocol.response.format.SimpleJsonResponseFormatter; import com.amazon.opendistroforelasticsearch.sql.sql.SQLService; @@ -89,13 +90,17 @@ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient no } SQLService sqlService = createSQLService(nodeClient); - UnresolvedPlan ast; + PhysicalPlan plan; try { - ast = sqlService.parse(request.getQuery()); + // For now analyzing and planning stage may throw syntax exception as well + // which hints the fallback to legacy code is necessary here. + plan = sqlService.plan( + sqlService.analyze( + sqlService.parse(request.getQuery()))); } catch (SyntaxCheckException e) { return NOT_SUPPORTED_YET; } - return channel -> sqlService.execute(ast, createListener(channel)); + return channel -> sqlService.execute(plan, createListener(channel)); } private SQLService createSQLService(NodeClient client) { diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java index b715dc8364..8e9d6452fb 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java @@ -67,6 +67,7 @@ import java.util.function.Predicate; import java.util.regex.Pattern; +import static com.amazon.opendistroforelasticsearch.sql.legacy.plugin.SqlSettings.CURSOR_ENABLED; import static com.amazon.opendistroforelasticsearch.sql.legacy.plugin.SqlSettings.QUERY_ANALYSIS_ENABLED; import static com.amazon.opendistroforelasticsearch.sql.legacy.plugin.SqlSettings.QUERY_ANALYSIS_SEMANTIC_SUGGESTION; import static com.amazon.opendistroforelasticsearch.sql.legacy.plugin.SqlSettings.QUERY_ANALYSIS_SEMANTIC_THRESHOLD; @@ -145,7 +146,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli Format format = SqlRequestParam.getFormat(request.params()); - if (isNewEngineEnabled()) { + if (isNewEngineEnabled() && isCursorDisabled()) { // Route request to new query engine if it's supported already SQLQueryRequest newSqlRequest = new SQLQueryRequest(sqlRequest.getJsonContent(), sqlRequest.getSql(), @@ -266,6 +267,11 @@ private boolean isNewEngineEnabled() { return LocalClusterState.state().getSettingValue(SQL_NEW_ENGINE_ENABLED); } + private boolean isCursorDisabled() { + Boolean isEnabled = LocalClusterState.state().getSettingValue(CURSOR_ENABLED); + return isEnabled == Boolean.FALSE; + } + private static ColumnTypeProvider performAnalysis(String sql) { LocalClusterState clusterState = LocalClusterState.state(); SqlAnalysisConfig config = new SqlAnalysisConfig( diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java index 36b275fc2f..5f0f40e919 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java @@ -76,15 +76,26 @@ public void execute(SQLQueryRequest request, ResponseListener lis } /** - * Given AST, run the remaining steps to execute it. - * @param ast AST + * Given physical plan, execute it and listen on response. + * @param plan physical plan * @param listener callback listener */ - public void execute(UnresolvedPlan ast, ResponseListener listener) { + public void execute(PhysicalPlan plan, ResponseListener listener) { try { - executionEngine.execute( - plan( - analyze(ast)), listener); + executionEngine.execute(plan, listener); + } catch (Exception e) { + listener.onFailure(e); + } + } + + /** + * Given logical plan, run the remaining steps to execute it. + * @param logicalPlan logical plan + * @param listener callback listener + */ + public void execute(LogicalPlan logicalPlan, ResponseListener listener) { + try { + executionEngine.execute(plan(logicalPlan), listener); } catch (Exception e) { listener.onFailure(e); } diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java index 3291ccc1ce..017278454f 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLServiceTest.java @@ -22,17 +22,15 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; -import com.amazon.opendistroforelasticsearch.sql.ast.tree.UnresolvedPlan; import com.amazon.opendistroforelasticsearch.sql.common.response.ResponseListener; import com.amazon.opendistroforelasticsearch.sql.executor.ExecutionEngine; -import com.amazon.opendistroforelasticsearch.sql.sql.antlr.SQLSyntaxParser; +import com.amazon.opendistroforelasticsearch.sql.planner.physical.PhysicalPlan; import com.amazon.opendistroforelasticsearch.sql.sql.config.SQLServiceConfig; import com.amazon.opendistroforelasticsearch.sql.sql.domain.SQLQueryRequest; -import com.amazon.opendistroforelasticsearch.sql.sql.parser.AstBuilder; import com.amazon.opendistroforelasticsearch.sql.storage.StorageEngine; import java.util.Collections; -import org.antlr.v4.runtime.tree.ParseTree; import org.json.JSONObject; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -87,17 +85,14 @@ public void onFailure(Exception e) { } @Test - public void canExecuteFromAst() { + public void canExecuteFromPhysicalPlan() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); listener.onResponse(new QueryResponse(Collections.emptyList())); return null; }).when(executionEngine).execute(any(), any()); - ParseTree parseTree = new SQLSyntaxParser().parse("SELECT 123"); - UnresolvedPlan ast = parseTree.accept(new AstBuilder("SELECT 123")); - - sqlService.execute(ast, + sqlService.execute(mock(PhysicalPlan.class), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -129,13 +124,10 @@ public void onFailure(Exception e) { } @Test - public void canCaptureErrorDuringExecutionFromAst() { + public void canCaptureErrorDuringExecutionFromPhysicalPlan() { doThrow(new RuntimeException()).when(executionEngine).execute(any(), any()); - ParseTree parseTree = new SQLSyntaxParser().parse("SELECT 123"); - UnresolvedPlan ast = parseTree.accept(new AstBuilder("SELECT 123")); - - sqlService.execute(ast, + sqlService.execute(mock(PhysicalPlan.class), new ResponseListener() { @Override public void onResponse(QueryResponse response) { From 75f55200153bd381397988f92eca17eec23a15fd Mon Sep 17 00:00:00 2001 From: Dai Date: Tue, 28 Jul 2020 16:15:59 -0700 Subject: [PATCH 16/27] Fix jacoco --- .../sql/analysis/ExpressionAnalyzer.java | 6 +++- .../sql/analysis/ExpressionAnalyzerTest.java | 32 +++++++++++++++++++ .../sql/sql/SQLService.java | 13 -------- .../sql/sql/parser/AstExpressionBuilder.java | 28 +++++++++------- 4 files changed, 53 insertions(+), 26 deletions(-) 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 61a8339fd7..4936a2b7ab 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 @@ -156,6 +156,10 @@ public Expression visitField(Field node, AnalysisContext context) { @Override public Expression visitQualifiedName(QualifiedName node, AnalysisContext context) { + if (node.getParts().size() > 1) { + throw new SyntaxCheckException(String.format( + "Qualified name [%s] is not supported yet", node)); + } return visitIdentifier(node.toString(), context); } @@ -171,7 +175,7 @@ private Expression visitIdentifier(String ident, AnalysisContext context) { if (isTypeNotSupported(ref.type())) { throw new SyntaxCheckException(String.format( - "Identifier [%s] of type [%s] is not supported yet,", ident, ref.type())); + "Identifier [%s] of type [%s] is not supported yet", ident, ref.type())); } return ref; } 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 d60b9eaaf9..4048bafe98 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 @@ -25,6 +25,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; +import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException; import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; @@ -89,6 +90,37 @@ public void named_expression() { ); } + @Test + public void skip_identifier_with_qualifier() { + SyntaxCheckException exception = + assertThrows(SyntaxCheckException.class, + () -> analyze(AstDSL.qualifiedName("index_alias", "integer_value"))); + + assertEquals( + "Qualified name [index_alias.integer_value] is not supported yet", + exception.getMessage() + ); + } + + @Test + public void skip_struct_and_array_data_type() { + SyntaxCheckException exception1 = + assertThrows(SyntaxCheckException.class, + () -> analyze(AstDSL.qualifiedName("struct_value"))); + assertEquals( + "Identifier [struct_value] of type [STRUCT] is not supported yet", + exception1.getMessage() + ); + + SyntaxCheckException exception2 = + assertThrows(SyntaxCheckException.class, + () -> analyze(AstDSL.qualifiedName("array_value"))); + assertEquals( + "Identifier [array_value] of type [ARRAY] is not supported yet", + exception2.getMessage() + ); + } + @Test public void undefined_var_semantic_check_failed() { SemanticCheckException exception = assertThrows(SemanticCheckException.class, diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java index 5f0f40e919..387da42a79 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLService.java @@ -88,19 +88,6 @@ public void execute(PhysicalPlan plan, ResponseListener listener) } } - /** - * Given logical plan, run the remaining steps to execute it. - * @param logicalPlan logical plan - * @param listener callback listener - */ - public void execute(LogicalPlan logicalPlan, ResponseListener listener) { - try { - executionEngine.execute(plan(logicalPlan), listener); - } catch (Exception e) { - listener.onFailure(e); - } - } - /** * Parse query and convert parse tree (CST) to abstract syntax tree (AST). */ diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java index 8a80e03684..afadf3f608 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java @@ -28,6 +28,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.expression.Function; import com.amazon.opendistroforelasticsearch.sql.ast.expression.QualifiedName; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; +import com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.ColumnNameContext; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.IdentContext; @@ -36,8 +37,10 @@ import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.TableNameContext; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParserBaseVisitor; import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.stream.Collectors; -import org.antlr.v4.runtime.tree.RuleNode; +import org.antlr.v4.runtime.RuleContext; /** * Expression builder to parse text to expression in AST. @@ -46,27 +49,22 @@ public class AstExpressionBuilder extends OpenDistroSQLParserBaseVisitor identifiers) { + return new QualifiedName( + identifiers + .stream() + .map(RuleContext::getText) + .map(StringUtils::unquoteIdentifier) + .collect(Collectors.toList()) + ); } } From a41b3072e1ee831c35900095cb44a72855e5dc79 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 10:15:38 -0700 Subject: [PATCH 17/27] Fix all IT and preserve original name with alias --- .../sql/analysis/ExpressionAnalyzer.java | 5 ++++- .../sql/ast/dsl/AstDSL.java | 4 ++++ .../sql/ast/expression/Alias.java | 9 +++++++- .../sql/expression/DSL.java | 4 ++++ .../sql/expression/NamedExpression.java | 7 ++++++ .../sql/analysis/ExpressionAnalyzerTest.java | 10 ++++++++- .../sql/legacy/PrettyFormatResponseIT.java | 1 + .../sql/legacy/QueryIT.java | 1 + .../sql/sql/parser/AstBuilder.java | 22 +++++++++++++------ .../sql/sql/parser/AstBuilderTest.java | 18 ++++++++++----- 10 files changed, 65 insertions(+), 16 deletions(-) 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 4936a2b7ab..a1273667e2 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 @@ -165,7 +165,10 @@ public Expression visitQualifiedName(QualifiedName node, AnalysisContext context @Override public Expression visitAlias(Alias node, AnalysisContext context) { - return new NamedExpression(node.getName(), node.getDelegate().accept(this, context)); + return new NamedExpression( + node.getName(), + node.getDelegate().accept(this, context), + node.getAlias()); } private Expression visitIdentifier(String ident, AnalysisContext context) { 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 5a9b10a22a..cb9578a6bf 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 @@ -230,6 +230,10 @@ public Alias alias(String name, UnresolvedExpression expr) { return new Alias(name, expr); } + public Alias alias(String name, UnresolvedExpression expr, String alias) { + return new Alias(name, expr, alias); + } + public static List exprList(UnresolvedExpression... exprList) { return Arrays.asList(exprList); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java index cfe2dd11f0..5430f54d86 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java @@ -17,6 +17,7 @@ package com.amazon.opendistroforelasticsearch.sql.ast.expression; import com.amazon.opendistroforelasticsearch.sql.ast.AbstractNodeVisitor; +import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.RequiredArgsConstructor; @@ -25,6 +26,7 @@ /** * Field name alias abstraction that associate an expression with a name. */ +@AllArgsConstructor @EqualsAndHashCode(callSuper = false) @Getter @RequiredArgsConstructor @@ -32,7 +34,7 @@ public class Alias extends UnresolvedExpression { /** - * Original field name or alias. + * Original field name. */ private final String name; @@ -41,6 +43,11 @@ public class Alias extends UnresolvedExpression { */ private final UnresolvedExpression delegate; + /** + * Optional field alias. + */ + private String alias; + @Override public T accept(AbstractNodeVisitor nodeVisitor, C context) { return nodeVisitor.visitAlias(this, context); diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java index b91c66a4b9..98ca4b1f4d 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java @@ -79,6 +79,10 @@ public static NamedExpression named(String name, Expression expression) { return new NamedExpression(name, expression); } + public static NamedExpression named(String name, Expression expression, String alias) { + return new NamedExpression(name, expression, alias); + } + public FunctionExpression abs(Expression... expressions) { return function(BuiltinFunctionName.ABS, expressions); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java index f183bbc77f..07a986c20e 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java @@ -19,6 +19,7 @@ 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 lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.RequiredArgsConstructor; @@ -29,6 +30,7 @@ * original expression name or alias in query and avoid inferring its name * by reconstructing in toString() method. */ +@AllArgsConstructor @EqualsAndHashCode @RequiredArgsConstructor @ToString @@ -45,6 +47,11 @@ public class NamedExpression implements Expression { */ private final Expression delegation; + /** + * Optional alias. + */ + private String alias; + @Override public ExprValue valueOf(Environment valueEnv) { return delegation.valueOf(valueEnv); 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 4048bafe98..94c6a07886 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 @@ -81,7 +81,7 @@ public void qualified_name() { AstDSL.qualifiedName("integer_value") ); } - + @Test public void named_expression() { assertAnalyzeEqual( @@ -90,6 +90,14 @@ public void named_expression() { ); } + @Test + public void named_expression_with_alias() { + assertAnalyzeEqual( + DSL.named("integer", DSL.ref("integer_value", INTEGER), "int"), + AstDSL.alias("integer", AstDSL.qualifiedName("integer_value"), "int") + ); + } + @Test public void skip_identifier_with_qualifier() { SyntaxCheckException exception = 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 cb5be363dc..273f366171 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 @@ -433,6 +433,7 @@ public void aggregationFunctionInHaving() throws IOException { // public void nestedAggregationFunctionInSelect() { // String query = String.format(Locale.ROOT, "SELECT SUM(SQRT(age)) FROM age GROUP BY age", TEST_INDEX_ACCOUNT); // } + @Ignore("New engine returns string type") @Test public void fieldsWithAlias() throws IOException { JSONObject response = executeQuery( diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java index 94314cfd32..cba9b1c2e4 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/QueryIT.java @@ -1643,6 +1643,7 @@ public void fieldCollapsingTest() throws IOException { Assert.assertEquals(21, hits.length()); } + @Ignore("New engine doesn't have 'alias' field in schema in response") @Test public void backticksQuotedIndexNameTest() throws Exception { TestUtils.createIndexByRestClient(client(), "bank_unquote", null); diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index a44c36d5ad..5efaacb2d8 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -36,6 +36,7 @@ import java.util.List; import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; +import org.antlr.v4.runtime.ParserRuleContext; import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.ParseTree; @@ -103,17 +104,24 @@ private UnresolvedExpression visitAstExpression(ParseTree tree) { } private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { + String name = unquoteIdentifier(getTextInQuery(ctx.expression())); UnresolvedExpression delegate = visitAstExpression(ctx.expression()); - return new Alias(unquoteIdentifier(getAlias(ctx)), delegate); - } - private String getAlias(SelectElementContext ctx) { if (ctx.alias() == null) { - Token start = ctx.expression().getStart(); - Token stop = ctx.expression().getStop(); - return query.substring(start.getStartIndex(), stop.getStopIndex() + 1); + return new Alias(name, delegate); } - return ctx.alias().getText(); + + String alias = unquoteIdentifier(ctx.alias().getText()); + return new Alias(name, delegate, alias); + } + + /** + * Get original text in query as it is. + */ + private String getTextInQuery(ParserRuleContext ctx) { + Token start = ctx.getStart(); + Token stop = ctx.getStop(); + return query.substring(start.getStartIndex(), stop.getStopIndex() + 1); } } diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java index bea0d5082e..9bc6970e38 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java @@ -63,8 +63,9 @@ public void can_build_select_function_call_with_alias() { project( relation("test"), alias( - "a", - function("ABS", qualifiedName("age")) + "ABS(age)", + function("ABS", qualifiedName("age")), + "a" ) ), buildAST("SELECT ABS(age) AS a FROM test") @@ -97,7 +98,7 @@ public void can_build_select_fields_with_alias() { assertEquals( project( relation("test"), - alias("a", qualifiedName("age")) + alias("age", qualifiedName("age"), "a") ), buildAST("SELECT age AS a FROM test") ); @@ -108,10 +109,15 @@ public void can_build_select_fields_with_alias_quoted() { assertEquals( project( relation("test"), - alias("first name", qualifiedName("name")), alias( - "Age_Expr", - function("+", qualifiedName("age"), intLiteral(10)) + "name", + qualifiedName("name"), + "first name" + ), + alias( + "(age + 10)", + function("+", qualifiedName("age"), intLiteral(10)), + "Age_Expr" ) ), buildAST("SELECT" From 0654f41f7abdb49632504add8aac67d49bd5b242 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 11:18:44 -0700 Subject: [PATCH 18/27] Fix remove command failure --- .../opendistroforelasticsearch/sql/analysis/Analyzer.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java index 7fbb27344b..08f8de9fd9 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java @@ -18,6 +18,7 @@ import com.amazon.opendistroforelasticsearch.sql.analysis.symbol.Namespace; import com.amazon.opendistroforelasticsearch.sql.analysis.symbol.Symbol; import com.amazon.opendistroforelasticsearch.sql.ast.AbstractNodeVisitor; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Field; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Let; @@ -161,7 +162,8 @@ public LogicalPlan visitProject(Project node, AnalysisContext context) { if (exclude) { List referenceExpressions = node.getProjectList().stream() - .map(expr -> (ReferenceExpression) expressionAnalyzer.analyze(expr, context)) + .map(expr -> (ReferenceExpression) + expressionAnalyzer.analyze(((Alias) expr).getDelegate(), context)) .collect(Collectors.toList()); return new LogicalRemove(child, ImmutableSet.copyOf(referenceExpressions)); } From 33db7675b97764681f540932192f1a974d5b821d Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 11:26:36 -0700 Subject: [PATCH 19/27] Fix remove command failure --- .../opendistroforelasticsearch/sql/analysis/AnalyzerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java index c6b57ba792..9d66da3ba2 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java @@ -151,8 +151,8 @@ public void remove_source() { AstDSL.projectWithArg( AstDSL.relation("schema"), Collections.singletonList(argument("exclude", booleanLiteral(true))), - AstDSL.field("integer_value"), - AstDSL.field("double_value"))); + AstDSL.alias("integer", AstDSL.field("integer_value")), + AstDSL.alias("double", AstDSL.field("double_value")))); } @Disabled("the project/remove command should shrink the type env") From 07f8a10cf450bf51c91184d3d0b9715c07848b66 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 11:44:35 -0700 Subject: [PATCH 20/27] Fix doctest --- docs/user/dql/expressions.rst | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/user/dql/expressions.rst b/docs/user/dql/expressions.rst index dca1c6463b..2e4e475e30 100644 --- a/docs/user/dql/expressions.rst +++ b/docs/user/dql/expressions.rst @@ -36,11 +36,11 @@ Here is an example for different type of literals:: od> SELECT 123, 'hello', false, -4.567, DATE '2020-07-07', TIME '01:01:01', TIMESTAMP '2020-07-07 01:01:01'; fetched rows / total rows = 1/1 - +-------+-----------+---------+----------+---------------------+-------------------+-----------------------------------+ - | 123 | "hello" | false | -4.567 | DATE '2020-07-07' | TIME '01:01:01' | TIMESTAMP '2020-07-07 01:01:01' | - |-------+-----------+---------+----------+---------------------+-------------------+-----------------------------------| - | 123 | hello | False | -4.567 | 2020-07-07 | 01:01:01 | 2020-07-07 01:01:01 | - +-------+-----------+---------+----------+---------------------+-------------------+-----------------------------------+ + +-------+---------+---------+----------+---------------------+-------------------+-----------------------------------+ + | 123 | hello | false | -4.567 | DATE '2020-07-07' | TIME '01:01:01' | TIMESTAMP '2020-07-07 01:01:01' | + |-------+---------+---------+----------+---------------------+-------------------+-----------------------------------| + | 123 | hello | False | -4.567 | 2020-07-07 | 01:01:01 | 2020-07-07 01:01:01 | + +-------+---------+---------+----------+---------------------+-------------------+-----------------------------------+ Limitations ----------- @@ -86,11 +86,11 @@ Here is an example for different type of arithmetic expressions:: od> SELECT 1 + 2, (9 - 1) % 3, 2 * 4 / 3; fetched rows / total rows = 1/1 - +---------+-------------+-------------+ - | 1 + 2 | 9 - 1 % 3 | 2 * 4 / 3 | - |---------+-------------+-------------| - | 3 | 2 | 2 | - +---------+-------------+-------------+ + +---------+---------------+-------------+ + | 1 + 2 | (9 - 1) % 3 | 2 * 4 / 3 | + |---------+---------------+-------------| + | 3 | 2 | 2 | + +---------+---------------+-------------+ Function Call From 88556f2dee46fc007ffaebadb57c775288433d65 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 11:56:32 -0700 Subject: [PATCH 21/27] Fix broken UT --- .../executor/ElasticsearchExecutionProtectorTest.java | 4 +++- .../sql/elasticsearch/storage/ElasticsearchIndexTest.java | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java index c5dcf97b55..e254274197 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java @@ -21,6 +21,7 @@ import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.STRING; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.literal; +import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.named; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref; import static com.amazon.opendistroforelasticsearch.sql.planner.physical.PhysicalPlanDSL.filter; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -33,6 +34,7 @@ import com.amazon.opendistroforelasticsearch.sql.elasticsearch.executor.protector.ResourceMonitorPlan; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.ElasticsearchIndexScan; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.AvgAggregator; @@ -72,7 +74,7 @@ public void setup() { @Test public void testProtectIndexScan() { String indexName = "test"; - ReferenceExpression include = ref("age", INTEGER); + NamedExpression include = named("age", ref("age", INTEGER)); ReferenceExpression exclude = ref("name", STRING); ReferenceExpression dedupeField = ref("name", STRING); Expression filterExpr = literal(ExprBooleanValue.of(true)); diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java index 458c2f2ecd..12eaf2c9fc 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java @@ -20,6 +20,7 @@ import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.STRING; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.literal; +import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.named; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.aggregation; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.eval; @@ -45,6 +46,7 @@ import com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.value.ElasticsearchExprValueFactory; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.mapping.IndexMapping; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.AvgAggregator; @@ -122,7 +124,7 @@ void implementRelationOperatorOnly() { @Test void implementOtherLogicalOperators() { String indexName = "test"; - ReferenceExpression include = ref("age", INTEGER); + NamedExpression include = named("age", ref("age", INTEGER)); ReferenceExpression exclude = ref("name", STRING); ReferenceExpression dedupeField = ref("name", STRING); Expression filterExpr = literal(ExprBooleanValue.of(true)); From 1d643f1a6eec3f8bb91342d7a710c3c0f50920e7 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 13:40:10 -0700 Subject: [PATCH 22/27] Fix broken UT --- .../correctness/tests/TestDataSetTest.java | 66 +++++++++++++++---- 1 file changed, 53 insertions(+), 13 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestDataSetTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestDataSetTest.java index 6ca49fa34c..6077e18f47 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestDataSetTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestDataSetTest.java @@ -29,41 +29,81 @@ public class TestDataSetTest { @Test public void testDataSetWithSingleColumnData() { - TestDataSet dataSet = new TestDataSet("test", "mappings", "hello\nworld\n123"); + String mappings = + "{\n" + + " \"mappings\": {\n" + + " \"properties\": {\n" + + " \"field\": {\n" + + " \"type\": \"text\"\n" + + " }\n" + + " }\n" + + " }\n" + + "}"; + + TestDataSet dataSet = new TestDataSet("test", mappings, "field\nhello\nworld\n123"); assertEquals("test", dataSet.getTableName()); - assertEquals("mappings", dataSet.getSchema()); + assertEquals(mappings, dataSet.getSchema()); assertThat( dataSet.getDataRows(), contains( - new String[] {"hello"}, - new String[] {"world"}, - new String[] {"123"} + new Object[] {"field"}, + new Object[] {"hello"}, + new Object[] {"world"}, + new Object[] {"123"} ) ); } @Test public void testDataSetWithMultiColumnsData() { - TestDataSet dataSet = new TestDataSet("test", "mappings", "hello,world\n123"); + String mappings = + "{\n" + + " \"mappings\": {\n" + + " \"properties\": {\n" + + " \"field1\": {\n" + + " \"type\": \"text\"\n" + + " },\n" + + " \"field2\": {\n" + + " \"type\": \"integer\"\n" + + " }\n" + + " }\n" + + " }\n" + + "}"; + + TestDataSet dataSet = new TestDataSet("test", mappings, + "field1,field2\nhello,123\nworld,456"); assertThat( dataSet.getDataRows(), contains( - new String[] {"hello", "world"}, - new String[] {"123"} + new Object[] {"field1", "field2"}, + new Object[] {"hello", 123}, + new Object[] {"world", 456} ) ); } @Test public void testDataSetWithEscapedComma() { - TestDataSet dataSet = new TestDataSet("test", "mappings", - "hello,\"hello,world,123\"\n123\n\"[abc,def,ghi]\",456"); + String mappings = + "{\n" + + " \"mappings\": {\n" + + " \"properties\": {\n" + + " \"field\": {\n" + + " \"type\": \"text\"\n" + + " }\n" + + " }\n" + + " }\n" + + "}"; + + TestDataSet dataSet = new TestDataSet("test", mappings, + "field\n\"hello,world,123\"\n123\n\"[abc,def,ghi]\""); assertThat( dataSet.getDataRows(), contains( - new String[] {"hello", "hello,world,123"}, - new String[] {"123"}, - new String[] {"[abc,def,ghi]", "456"} + new Object[] {"field"}, + new Object[] {"hello,world,123"}, + new Object[] {"123"}, + new Object[] {"[abc,def,ghi]"} ) ); } From e394d376a787e019b26ab20b719d4e1a17f1d08e Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 15:01:59 -0700 Subject: [PATCH 23/27] Revert to make Alias optional for PPL --- .../sql/analysis/Analyzer.java | 8 ++++---- .../sql/analysis/ExpressionAnalyzer.java | 2 +- .../sql/expression/DSL.java | 12 ++++++++++++ .../sql/expression/NamedExpression.java | 2 +- .../sql/analysis/AnalyzerTest.java | 12 ++++++------ .../sql/analysis/ExpressionAnalyzerTest.java | 13 ++++++++----- .../sql/expression/NamedExpressionTest.java | 3 ++- docs/user/general/identifiers.rst | 12 ++---------- .../sql/correctness/testset/TestDataSet.java | 12 ++++++------ .../correctness/expressions/arithmetics.txt | 2 -- .../sql/legacy/plugin/RestSqlAction.java | 2 +- .../sql/ppl/parser/AstBuilder.java | 2 -- .../sql/sql/parser/AstBuilder.java | 6 +++--- 13 files changed, 46 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java index 08f8de9fd9..628720efec 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java @@ -15,10 +15,11 @@ package com.amazon.opendistroforelasticsearch.sql.analysis; +import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.named; + import com.amazon.opendistroforelasticsearch.sql.analysis.symbol.Namespace; import com.amazon.opendistroforelasticsearch.sql.analysis.symbol.Symbol; import com.amazon.opendistroforelasticsearch.sql.ast.AbstractNodeVisitor; -import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Field; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Let; @@ -162,15 +163,14 @@ public LogicalPlan visitProject(Project node, AnalysisContext context) { if (exclude) { List referenceExpressions = node.getProjectList().stream() - .map(expr -> (ReferenceExpression) - expressionAnalyzer.analyze(((Alias) expr).getDelegate(), context)) + .map(expr -> (ReferenceExpression) expressionAnalyzer.analyze(expr, context)) .collect(Collectors.toList()); return new LogicalRemove(child, ImmutableSet.copyOf(referenceExpressions)); } } List expressions = node.getProjectList().stream() - .map(expr -> (NamedExpression) expressionAnalyzer.analyze(expr, context)) + .map(expr -> named(expressionAnalyzer.analyze(expr, context))) .collect(Collectors.toList()); return new LogicalProject(child, expressions); } 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 a1273667e2..9d621eeec2 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 @@ -165,7 +165,7 @@ public Expression visitQualifiedName(QualifiedName node, AnalysisContext context @Override public Expression visitAlias(Alias node, AnalysisContext context) { - return new NamedExpression( + return DSL.named( node.getName(), node.getDelegate().accept(this, context), node.getAlias()); diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java index 98ca4b1f4d..9cd113b7c5 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java @@ -75,6 +75,18 @@ public static ReferenceExpression ref(String ref, ExprType type) { return new ReferenceExpression(ref, type); } + /** + * Wrap a named expression if not. + * @param expression expression + * @return expression if named already or expression wrapped by named expression. + */ + public static NamedExpression named(Expression expression) { + if (expression instanceof NamedExpression) { + return (NamedExpression) expression; + } + return named(expression.toString(), expression); + } + public static NamedExpression named(String name, Expression expression) { return new NamedExpression(name, expression); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java index 07a986c20e..ce152a2da1 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java @@ -32,6 +32,7 @@ */ @AllArgsConstructor @EqualsAndHashCode +@Getter @RequiredArgsConstructor @ToString public class NamedExpression implements Expression { @@ -39,7 +40,6 @@ public class NamedExpression implements Expression { /** * Expression name. */ - @Getter private final String name; /** diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java index 9d66da3ba2..d705854661 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java @@ -138,7 +138,7 @@ public void project_source() { AstDSL.projectWithArg( AstDSL.relation("schema"), AstDSL.defaultFieldsArgs(), - AstDSL.alias("integer_value", AstDSL.field("integer_value")), + AstDSL.field("integer_value"), // Field not wrapped by Alias AstDSL.alias("double_value", AstDSL.field("double_value")))); } @@ -151,8 +151,8 @@ public void remove_source() { AstDSL.projectWithArg( AstDSL.relation("schema"), Collections.singletonList(argument("exclude", booleanLiteral(true))), - AstDSL.alias("integer", AstDSL.field("integer_value")), - AstDSL.alias("double", AstDSL.field("double_value")))); + AstDSL.field("integer_value"), + AstDSL.field("double_value"))); } @Disabled("the project/remove command should shrink the type env") @@ -167,10 +167,10 @@ public void project_source_change_type_env() { AstDSL.projectWithArg( AstDSL.relation("schema"), AstDSL.defaultFieldsArgs(), - AstDSL.alias("integer_value", AstDSL.field("integer_value")), - AstDSL.alias("double_value", AstDSL.field("double_value"))), + AstDSL.field("integer_value"), + AstDSL.field("double_value")), AstDSL.defaultFieldsArgs(), - AstDSL.alias("float_value", AstDSL.field("float_value"))))); + AstDSL.field("float_value")))); } @Test 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 94c6a07886..9d7535253c 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 @@ -111,21 +111,24 @@ public void skip_identifier_with_qualifier() { } @Test - public void skip_struct_and_array_data_type() { - SyntaxCheckException exception1 = + public void skip_struct_data_type() { + SyntaxCheckException exception = assertThrows(SyntaxCheckException.class, () -> analyze(AstDSL.qualifiedName("struct_value"))); assertEquals( "Identifier [struct_value] of type [STRUCT] is not supported yet", - exception1.getMessage() + exception.getMessage() ); + } - SyntaxCheckException exception2 = + @Test + public void skip_array_data_type() { + SyntaxCheckException exception = assertThrows(SyntaxCheckException.class, () -> analyze(AstDSL.qualifiedName("array_value"))); assertEquals( "Identifier [array_value] of type [ARRAY] is not supported yet", - exception2.getMessage() + exception.getMessage() ); } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java index a1864c84f3..72e0fcefa2 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java @@ -28,11 +28,12 @@ class NamedExpressionTest extends ExpressionTestBase { @Test void name_an_expression() { LiteralExpression delegate = DSL.literal(10); - NamedExpression namedExpression = DSL.named("int", delegate); + NamedExpression namedExpression = DSL.named("int", delegate, "i"); assertEquals("int", namedExpression.getName()); assertEquals(delegate.type(), namedExpression.type()); assertEquals(delegate.valueOf(valueEnv()), namedExpression.valueOf(valueEnv())); + assertEquals("i", namedExpression.getAlias()); } } \ No newline at end of file diff --git a/docs/user/general/identifiers.rst b/docs/user/general/identifiers.rst index 5a0c09a062..c948c18d0e 100644 --- a/docs/user/general/identifiers.rst +++ b/docs/user/general/identifiers.rst @@ -103,17 +103,9 @@ For example, if you run ``SELECT * FROM ACCOUNTS``, it will end up with an index Identifier Qualifiers ===================== -Description ------------ - -TODO: field name qualifiers - -Examples --------- - -Here is an example for ... - Limitations ----------- For now, we do not support using Elasticsearch cluster name as catalog name to qualify an index name, such as ``my-cluster.logs``. + +TODO: field name qualifiers \ No newline at end of file diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java index d287ca26c3..42036a2f2a 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/testset/TestDataSet.java @@ -36,7 +36,7 @@ public class TestDataSet { public TestDataSet(String tableName, String schemaFileContent, String dataFileContent) { this.tableName = tableName; this.schema = schemaFileContent; - this.dataRows = convertColumnStringToObject(splitColumns(dataFileContent, ',')); + this.dataRows = convertStringDataToActualType(splitColumns(dataFileContent, ',')); } public String getTableName() { @@ -88,7 +88,7 @@ private List splitColumns(String content, char separator) { * Convert column string values (read from CSV file) to objects of its real type * based on the type information in index mapping file. */ - private List convertColumnStringToObject(List rows) { + private List convertStringDataToActualType(List rows) { JSONObject types = new JSONObject(schema); String[] columnNames = rows.get(0); @@ -97,23 +97,23 @@ private List convertColumnStringToObject(List rows) { rows.stream() .skip(1) - .map(row -> convertStringValues(types, columnNames, row)) + .map(row -> convertStringArrayToObjectArray(types, columnNames, row)) .forEach(result::add); return result; } - private Object[] convertStringValues(JSONObject types, String[] columnNames, String[] row) { + private Object[] convertStringArrayToObjectArray(JSONObject types, String[] columnNames, String[] row) { Object[] result = new Object[row.length]; for (int i = 0; i < row.length; i++) { String colName = columnNames[i]; String colTypePath = "/mappings/properties/" + colName; String colType = ((JSONObject) types.query(colTypePath)).getString("type"); - result[i] = convertStringValue(colType, row[i]); + result[i] = convertStringToObject(colType, row[i]); } return result; } - private Object convertStringValue(String type, String str) { + private Object convertStringToObject(String type, String str) { switch (type.toLowerCase()) { case "text": case "keyword": diff --git a/integ-test/src/test/resources/correctness/expressions/arithmetics.txt b/integ-test/src/test/resources/correctness/expressions/arithmetics.txt index c3730393e7..64872a4df3 100644 --- a/integ-test/src/test/resources/correctness/expressions/arithmetics.txt +++ b/integ-test/src/test/resources/correctness/expressions/arithmetics.txt @@ -9,5 +9,3 @@ 5 % 2 -5 % 2 0 % 2 -(2 / 1) -(5 - 1.2) * 3 \ No newline at end of file diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java index 8e9d6452fb..073fe52efc 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/plugin/RestSqlAction.java @@ -269,7 +269,7 @@ private boolean isNewEngineEnabled() { private boolean isCursorDisabled() { Boolean isEnabled = LocalClusterState.state().getSettingValue(CURSOR_ENABLED); - return isEnabled == Boolean.FALSE; + return Boolean.FALSE.equals(isEnabled); } private static ColumnTypeProvider performAnalysis(String sql) { 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 004da009c6..741a196974 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 @@ -28,7 +28,6 @@ import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.StatsCommandContext; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.WhereCommandContext; -import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Field; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Let; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Map; @@ -105,7 +104,6 @@ public UnresolvedPlan visitFieldsCommand(FieldsCommandContext ctx) { .wcFieldExpression() .stream() .map(field -> (Field) visitExpression(field)) - .map(field -> new Alias(field.getField().toString(), field)) .collect(Collectors.toList()), ArgumentFactory.getArgumentList(ctx) ); diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index 5efaacb2d8..c82a0a29ff 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -105,14 +105,14 @@ private UnresolvedExpression visitAstExpression(ParseTree tree) { private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { String name = unquoteIdentifier(getTextInQuery(ctx.expression())); - UnresolvedExpression delegate = visitAstExpression(ctx.expression()); + UnresolvedExpression expr = visitAstExpression(ctx.expression()); if (ctx.alias() == null) { - return new Alias(name, delegate); + return new Alias(name, expr); } String alias = unquoteIdentifier(ctx.alias().getText()); - return new Alias(name, delegate, alias); + return new Alias(name, expr, alias); } /** From 488bfe72af06f72f121cb6d890321b802f909db1 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 17:34:02 -0700 Subject: [PATCH 24/27] Prepare PR --- .../sql/analysis/ExpressionAnalyzer.java | 11 ++++++----- .../sql/ast/expression/Alias.java | 2 +- .../sql/expression/NamedExpression.java | 17 ++++++++++++----- .../sql/expression/NamedExpressionTest.java | 18 ++++++++++++------ docs/user/general/identifiers.rst | 5 +---- .../sql/sql/SQLCorrectnessIT.java | 2 +- .../resources/correctness/queries/select.txt | 8 ++++++-- .../sql/ppl/parser/AstBuilder.java | 2 +- .../sql/sql/parser/AstBuilder.java | 12 ++++++------ .../sql/sql/parser/AstExpressionBuilder.java | 9 ++++----- 10 files changed, 50 insertions(+), 36 deletions(-) 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 9d621eeec2..db9a782d94 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 @@ -38,7 +38,6 @@ import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException; import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; -import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.Aggregator; import com.amazon.opendistroforelasticsearch.sql.expression.function.BuiltinFunctionName; @@ -156,6 +155,8 @@ public Expression visitField(Field node, AnalysisContext context) { @Override public Expression visitQualifiedName(QualifiedName node, AnalysisContext context) { + // Name with qualifier (index.field, index_alias.field, object/nested.inner_field + // text.keyword) is not supported for now if (node.getParts().size() > 1) { throw new SyntaxCheckException(String.format( "Qualified name [%s] is not supported yet", node)); @@ -165,10 +166,9 @@ public Expression visitQualifiedName(QualifiedName node, AnalysisContext context @Override public Expression visitAlias(Alias node, AnalysisContext context) { - return DSL.named( - node.getName(), - node.getDelegate().accept(this, context), - node.getAlias()); + return DSL.named(node.getName(), + node.getDelegated().accept(this, context), + node.getAlias()); } private Expression visitIdentifier(String ident, AnalysisContext context) { @@ -176,6 +176,7 @@ private Expression visitIdentifier(String ident, AnalysisContext context) { ReferenceExpression ref = DSL.ref(ident, typeEnv.resolve(new Symbol(Namespace.FIELD_NAME, ident))); + // Fall back to old engine too if type is not supported semantically if (isTypeNotSupported(ref.type())) { throw new SyntaxCheckException(String.format( "Identifier [%s] of type [%s] is not supported yet", ident, ref.type())); diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java index 5430f54d86..58d8a6a6e6 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java @@ -41,7 +41,7 @@ public class Alias extends UnresolvedExpression { /** * Expression aliased. */ - private final UnresolvedExpression delegate; + private final UnresolvedExpression delegated; /** * Optional field alias. diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java index ce152a2da1..9e68e1240a 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java @@ -19,9 +19,9 @@ 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 com.google.common.base.Strings; import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; -import lombok.Getter; import lombok.RequiredArgsConstructor; import lombok.ToString; @@ -32,7 +32,6 @@ */ @AllArgsConstructor @EqualsAndHashCode -@Getter @RequiredArgsConstructor @ToString public class NamedExpression implements Expression { @@ -45,7 +44,7 @@ public class NamedExpression implements Expression { /** * Expression that being named. */ - private final Expression delegation; + private final Expression delegated; /** * Optional alias. @@ -54,12 +53,20 @@ public class NamedExpression implements Expression { @Override public ExprValue valueOf(Environment valueEnv) { - return delegation.valueOf(valueEnv); + return delegated.valueOf(valueEnv); } @Override public ExprType type() { - return delegation.type(); + return delegated.type(); + } + + /** + * Get expression name using either name or alias. + * @return expression name + */ + public String getName() { + return Strings.isNullOrEmpty(alias) ? name : alias; } } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java index 72e0fcefa2..77196099ff 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpressionTest.java @@ -27,13 +27,19 @@ class NamedExpressionTest extends ExpressionTestBase { @Test void name_an_expression() { - LiteralExpression delegate = DSL.literal(10); - NamedExpression namedExpression = DSL.named("int", delegate, "i"); + LiteralExpression delegated = DSL.literal(10); + NamedExpression namedExpression = DSL.named("10", delegated); - assertEquals("int", namedExpression.getName()); - assertEquals(delegate.type(), namedExpression.type()); - assertEquals(delegate.valueOf(valueEnv()), namedExpression.valueOf(valueEnv())); - assertEquals("i", namedExpression.getAlias()); + assertEquals("10", namedExpression.getName()); + assertEquals(delegated.type(), namedExpression.type()); + assertEquals(delegated.valueOf(valueEnv()), namedExpression.valueOf(valueEnv())); + } + + @Test + void name_an_expression_with_alias() { + LiteralExpression delegated = DSL.literal(10); + NamedExpression namedExpression = DSL.named("10", delegated, "ten"); + assertEquals("ten", namedExpression.getName()); } } \ No newline at end of file diff --git a/docs/user/general/identifiers.rst b/docs/user/general/identifiers.rst index c948c18d0e..5dbee26131 100644 --- a/docs/user/general/identifiers.rst +++ b/docs/user/general/identifiers.rst @@ -103,9 +103,6 @@ For example, if you run ``SELECT * FROM ACCOUNTS``, it will end up with an index Identifier Qualifiers ===================== -Limitations ------------ - For now, we do not support using Elasticsearch cluster name as catalog name to qualify an index name, such as ``my-cluster.logs``. -TODO: field name qualifiers \ No newline at end of file +TODO: field name qualifiers diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java index 58c10073a1..8a1379a8fc 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java @@ -32,7 +32,7 @@ public class SQLCorrectnessIT extends CorrectnessTestBase { private static final String ROOT_DIR = "correctness/"; private static final String[] EXPR_TEST_DIR = { "expressions" }; - private static final String[] QUERY_TEST_DIR = { "queries"/*, "bugfixes"*/ }; //TODO: skip bugfixes folder for now since it fails + private static final String[] QUERY_TEST_DIR = { "queries", "bugfixes" }; @Override protected void init() throws Exception { diff --git a/integ-test/src/test/resources/correctness/queries/select.txt b/integ-test/src/test/resources/correctness/queries/select.txt index daa3b39ab1..298e0f22a1 100644 --- a/integ-test/src/test/resources/correctness/queries/select.txt +++ b/integ-test/src/test/resources/correctness/queries/select.txt @@ -1,4 +1,8 @@ SELECT 1 + 2 FROM kibana_sample_data_flights -SELECT ABS(-10) FROM kibana_sample_data_flights -SELECT ABS(DistanceMiles) FROM kibana_sample_data_flights +SELECT Cancelled, AvgTicketPrice, FlightDelayMin, Carrier, timestamp FROM kibana_sample_data_flights +SELECT `Cancelled`, `AvgTicketPrice` FROM kibana_sample_data_flights +SELECT ABS(DistanceMiles), (FlightDelayMin * 2) - 3 FROM kibana_sample_data_flights +SELECT abs(DistanceMiles), Abs(FlightDelayMin) FROM kibana_sample_data_flights +SELECT Cancelled AS Cancel, AvgTicketPrice AS ATP FROM kibana_sample_data_flights +SELECT Cancelled AS `Cancel`, AvgTicketPrice AS "ATP" FROM kibana_sample_data_flights SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500 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 741a196974..9924f3ac12 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 @@ -103,7 +103,7 @@ public UnresolvedPlan visitFieldsCommand(FieldsCommandContext ctx) { ctx.wcFieldList() .wcFieldExpression() .stream() - .map(field -> (Field) visitExpression(field)) + .map(this::visitExpression) .collect(Collectors.toList()), ArgumentFactory.getArgumentList(ctx) ); diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index c82a0a29ff..a8d10bb935 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -51,8 +51,8 @@ public class AstBuilder extends OpenDistroSQLParserBaseVisitor { private final AstExpressionBuilder expressionBuilder = new AstExpressionBuilder(); /** - * SQL query to get original token name because token.getText() returns text - * without whitespaces or other characters discarded by lexer. + * SQL query to get original token text. This is necessary because token.getText() returns + * text without whitespaces or other characters discarded by lexer. */ private final String query; @@ -109,14 +109,14 @@ private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { if (ctx.alias() == null) { return new Alias(name, expr); + } else { + String alias = unquoteIdentifier(ctx.alias().getText()); + return new Alias(name, expr, alias); } - - String alias = unquoteIdentifier(ctx.alias().getText()); - return new Alias(name, expr, alias); } /** - * Get original text in query as it is. + * Get original text in query. */ private String getTextInQuery(ParserRuleContext ctx) { Token start = ctx.getStart(); diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java index afadf3f608..90f9c94f79 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java @@ -130,11 +130,10 @@ public UnresolvedExpression visitTimestampLiteral( private QualifiedName visitIdentifiers(List identifiers) { return new QualifiedName( - identifiers - .stream() - .map(RuleContext::getText) - .map(StringUtils::unquoteIdentifier) - .collect(Collectors.toList()) + identifiers.stream() + .map(RuleContext::getText) + .map(StringUtils::unquoteIdentifier) + .collect(Collectors.toList()) ); } From 6e5d23c2c3995af7a0f120151d59ed80039fbc0f Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 29 Jul 2020 17:45:34 -0700 Subject: [PATCH 25/27] Prepare PR --- .../sql/sql/IdentifierIT.java | 29 +++++++++++++++++++ .../sql/sql/SQLCorrectnessIT.java | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java index 21d9614e25..dbd65c4bc8 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java @@ -50,6 +50,35 @@ public void testQuotedIndexNames() throws IOException { queryAndAssertTheDoc("SELECT * FROM \"logs.2020.01\""); } + @Test + public void testSpecialFieldName() throws IOException { + new Index("test") + .addDoc("{\"@timestamp\": 10, \"dimensions:major_version\": 30}"); + + assertEquals( + "{\n" + + " \"schema\": [\n" + + " {\n" + + " \"name\": \"@timestamp\",\n" + + " \"type\": \"long\"\n" + + " },\n" + + " {\n" + + " \"name\": \"dimensions:major_version\",\n" + + " \"type\": \"long\"\n" + + " }\n" + + " ],\n" + + " \"total\": 1,\n" + + " \"datarows\": [[\n" + + " 10,\n" + + " 30\n" + + " ]],\n" + + " \"size\": 1,\n" + + " \"status\": 200\n" + + "}\n", + executeQuery("SELECT @timestamp, `dimensions:major_version` FROM test", "jdbc") + ); + } + private void createIndexWithOneDoc(String... indexNames) throws IOException { for (String indexName : indexNames) { new Index(indexName).addDoc("{\"age\": 30}"); diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java index 8a1379a8fc..58c10073a1 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java @@ -32,7 +32,7 @@ public class SQLCorrectnessIT extends CorrectnessTestBase { private static final String ROOT_DIR = "correctness/"; private static final String[] EXPR_TEST_DIR = { "expressions" }; - private static final String[] QUERY_TEST_DIR = { "queries", "bugfixes" }; + private static final String[] QUERY_TEST_DIR = { "queries"/*, "bugfixes"*/ }; //TODO: skip bugfixes folder for now since it fails @Override protected void init() throws Exception { From 86ac2b8e6edafced3e4c8515e600569eb29c92fd Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 30 Jul 2020 12:32:26 -0700 Subject: [PATCH 26/27] Prepare PR --- .../sql/ast/expression/Alias.java | 5 ++++- .../opendistroforelasticsearch/sql/expression/DSL.java | 6 +++++- .../sql/expression/NamedExpression.java | 8 ++++---- .../opendistroforelasticsearch/sql/sql/IdentifierIT.java | 3 +-- sql/src/main/antlr/OpenDistroSQLLexer.g4 | 2 +- .../sql/sql/parser/AstQualifiedNameBuilderTest.java | 2 +- 6 files changed, 16 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java index 58d8a6a6e6..bcdac6e607 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/expression/Alias.java @@ -24,7 +24,10 @@ import lombok.ToString; /** - * Field name alias abstraction that associate an expression with a name. + * Alias abstraction that associate an unnamed expression with a name and an optional alias. + * The name and alias information preserved is useful for semantic analysis and response + * formatting eventually. This can avoid restoring the info in toString() method which is + * inaccurate because original info is already lost. */ @AllArgsConstructor @EqualsAndHashCode(callSuper = false) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java index 9cd113b7c5..46968f4339 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java @@ -76,7 +76,11 @@ public static ReferenceExpression ref(String ref, ExprType type) { } /** - * Wrap a named expression if not. + * Wrap a named expression if not yet. The intent is that different languages may use + * Alias or not when building AST. This caused either named or unnamed expression + * is resolved by analyzer. To make unnamed expression acceptable for logical project, + * it is required to wrap it by named expression here before passing to logical project. + * * @param expression expression * @return expression if named already or expression wrapped by named expression. */ diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java index 9e68e1240a..6f3889fa6c 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/NamedExpression.java @@ -26,9 +26,9 @@ import lombok.ToString; /** - * Named expression that represents expression with name. This is to preserve - * original expression name or alias in query and avoid inferring its name - * by reconstructing in toString() method. + * Named expression that represents expression with name. + * Please see more details in associated unresolved expression operator + * {@link com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias}. */ @AllArgsConstructor @EqualsAndHashCode @@ -62,7 +62,7 @@ public ExprType type() { } /** - * Get expression name using either name or alias. + * Get expression name using name or its alias (if it's present). * @return expression name */ public String getName() { diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java index dbd65c4bc8..3b5dc26bc5 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/IdentifierIT.java @@ -72,8 +72,7 @@ public void testSpecialFieldName() throws IOException { + " 10,\n" + " 30\n" + " ]],\n" - + " \"size\": 1,\n" - + " \"status\": 200\n" + + " \"size\": 1\n" + "}\n", executeQuery("SELECT @timestamp, `dimensions:major_version` FROM test", "jdbc") ); diff --git a/sql/src/main/antlr/OpenDistroSQLLexer.g4 b/sql/src/main/antlr/OpenDistroSQLLexer.g4 index 66e3d1bade..f128099275 100644 --- a/sql/src/main/antlr/OpenDistroSQLLexer.g4 +++ b/sql/src/main/antlr/OpenDistroSQLLexer.g4 @@ -319,7 +319,7 @@ BACKTICK_QUOTE_ID: BQUOTA_STRING; // Fragments for Literal primitives fragment EXPONENT_NUM_PART: 'E' [-+]? DEC_DIGIT+; -fragment ID_LITERAL: [*A-Z]+?[*A-Z_\-0-9]*; +fragment ID_LITERAL: [@*A-Z]+?[*A-Z_\-0-9]*; fragment DQUOTA_STRING: '"' ( '\\'. | '""' | ~('"'| '\\') )* '"'; fragment SQUOTA_STRING: '\'' ('\\'. | '\'\'' | ~('\'' | '\\'))* '\''; fragment BQUOTA_STRING: '`' ( '\\'. | '``' | ~('`'|'\\'))* '`'; diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstQualifiedNameBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstQualifiedNameBuilderTest.java index e8c1506e7d..7f7d5cf48a 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstQualifiedNameBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstQualifiedNameBuilderTest.java @@ -42,7 +42,7 @@ public void canBuildRegularIdentifierForSQLStandard() { @Test public void canBuildRegularIdentifierForElasticsearch() { buildFromTableName(".kibana").expectQualifiedName(".kibana"); - //buildFromIdentifier("@timestamp").expectQualifiedName("@timestamp");//TODO: field name + buildFromIdentifier("@timestamp").expectQualifiedName("@timestamp"); buildFromIdentifier("logs-2020-01").expectQualifiedName("logs-2020-01"); buildFromIdentifier("*logs*").expectQualifiedName("*logs*"); } From 08410e0440638445443cc3a5c031e941902b5575 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 31 Jul 2020 12:25:12 -0700 Subject: [PATCH 27/27] Dont remove single quotes for column and alias name --- .../sql/common/utils/StringUtils.java | 20 +++++++++++++--- docs/user/dql/expressions.rst | 23 +++++++++---------- .../sql/ppl/parser/AstExpressionBuilder.java | 7 +++--- .../sql/ppl/utils/ArgumentFactory.java | 4 ++-- .../sql/sql/parser/AstBuilder.java | 6 ++--- .../sql/sql/parser/AstExpressionBuilder.java | 9 ++++---- .../sql/sql/parser/AstBuilderTest.java | 2 +- 7 files changed, 41 insertions(+), 30 deletions(-) diff --git a/common/src/main/java/com/amazon/opendistroforelasticsearch/sql/common/utils/StringUtils.java b/common/src/main/java/com/amazon/opendistroforelasticsearch/sql/common/utils/StringUtils.java index 1df4b26758..51a757506e 100644 --- a/common/src/main/java/com/amazon/opendistroforelasticsearch/sql/common/utils/StringUtils.java +++ b/common/src/main/java/com/amazon/opendistroforelasticsearch/sql/common/utils/StringUtils.java @@ -19,13 +19,13 @@ public class StringUtils { /** - * Unquote Identifier with mark. + * Unquote any string with mark specified. * @param text string * @param mark quotation mark * @return An unquoted string whose outer pair of (single/double/back-tick) quotes have been * removed */ - public static String unquoteIdentifier(String text, String mark) { + public static String unquote(String text, String mark) { if (isQuoted(text, mark)) { return text.substring(mark.length(), text.length() - mark.length()); } @@ -38,7 +38,7 @@ public static String unquoteIdentifier(String text, String mark) { * @return An unquoted string whose outer pair of (single/double/back-tick) quotes have been * removed */ - public static String unquoteIdentifier(String text) { + public static String unquoteText(String text) { if (isQuoted(text, "\"") || isQuoted(text, "'") || isQuoted(text, "`")) { return text.substring(1, text.length() - 1); } else { @@ -46,6 +46,20 @@ public static String unquoteIdentifier(String text) { } } + /** + * Unquote Identifier which has " or ` as mark. + * @param identifier identifier that possibly enclosed by double quotes or back ticks + * @return An unquoted string whose outer pair of (double/back-tick) quotes have been + * removed + */ + public static String unquoteIdentifier(String identifier) { + if (isQuoted(identifier, "\"") || isQuoted(identifier, "`")) { + return identifier.substring(1, identifier.length() - 1); + } else { + return identifier; + } + } + private static boolean isQuoted(String text, String mark) { return !Strings.isNullOrEmpty(text) && text.startsWith(mark) && text.endsWith(mark); } diff --git a/docs/user/dql/expressions.rst b/docs/user/dql/expressions.rst index ca75c822c9..8b2fc88dce 100644 --- a/docs/user/dql/expressions.rst +++ b/docs/user/dql/expressions.rst @@ -36,11 +36,11 @@ Here is an example for different type of literals:: od> SELECT 123, 'hello', false, -4.567, DATE '2020-07-07', TIME '01:01:01', TIMESTAMP '2020-07-07 01:01:01'; fetched rows / total rows = 1/1 - +-------+---------+---------+----------+---------------------+-------------------+-----------------------------------+ - | 123 | hello | false | -4.567 | DATE '2020-07-07' | TIME '01:01:01' | TIMESTAMP '2020-07-07 01:01:01' | - |-------+---------+---------+----------+---------------------+-------------------+-----------------------------------| - | 123 | hello | False | -4.567 | 2020-07-07 | 01:01:01 | 2020-07-07 01:01:01 | - +-------+---------+---------+----------+---------------------+-------------------+-----------------------------------+ + +-------+-----------+---------+----------+---------------------+-------------------+-----------------------------------+ + | 123 | 'hello' | false | -4.567 | DATE '2020-07-07' | TIME '01:01:01' | TIMESTAMP '2020-07-07 01:01:01' | + |-------+-----------+---------+----------+---------------------+-------------------+-----------------------------------| + | 123 | hello | False | -4.567 | 2020-07-07 | 01:01:01 | 2020-07-07 01:01:01 | + +-------+-----------+---------+----------+---------------------+-------------------+-----------------------------------+ Limitations ----------- @@ -151,7 +151,7 @@ expr LIKE pattern. The expr is string value, pattern is supports literal text, a od> SELECT 'axyzb' LIKE 'a%b', 'acb' LIKE 'a_b', 'axyzb' NOT LIKE 'a%b', 'acb' NOT LIKE 'a_b'; fetched rows / total rows = 1/1 +----------------------+--------------------+--------------------------+------------------------+ - | "axyzb" like "a%b" | "acb" like "a_b" | "axyzb" not like "a%b" | "acb" not like "a_b" | + | 'axyzb' LIKE 'a%b' | 'acb' LIKE 'a_b' | 'axyzb' NOT LIKE 'a%b' | 'acb' NOT LIKE 'a_b' | |----------------------+--------------------+--------------------------+------------------------| | True | True | False | False | +----------------------+--------------------+--------------------------+------------------------+ @@ -163,12 +163,11 @@ Here is an example for null value test:: od> SELECT 0 IS NULL, 0 IS NOT NULL, NULL IS NULL, NULL IS NOT NULL; fetched rows / total rows = 1/1 - +--------------+------------------+-----------------+---------------------+ - | is null(0) | is not null(0) | is null(NULL) | is not null(NULL) | - |--------------+------------------+-----------------+---------------------| - | False | True | True | False | - +--------------+------------------+-----------------+---------------------+ - + +-------------+-----------------+----------------+--------------------+ + | 0 IS NULL | 0 IS NOT NULL | NULL IS NULL | NULL IS NOT NULL | + |-------------+-----------------+----------------+--------------------| + | False | True | True | False | + +-------------+-----------------+----------------+--------------------+ Function Call ============= 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 f8c4b351e0..1834f98196 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,7 +15,6 @@ package com.amazon.opendistroforelasticsearch.sql.ppl.parser; -import static com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils.unquoteIdentifier; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.BinaryArithmeticContext; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.BooleanLiteralContext; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.CompareExprContext; @@ -193,7 +192,7 @@ public UnresolvedExpression visitQualifiedName(QualifiedNameContext ctx) { ctx.ident() .stream() .map(ParserRuleContext::getText) - .map(StringUtils::unquoteIdentifier) + .map(StringUtils::unquoteText) .collect(Collectors.toList()) ); } @@ -204,14 +203,14 @@ public UnresolvedExpression visitWcQualifiedName(WcQualifiedNameContext ctx) { ctx.wildcard() .stream() .map(ParserRuleContext::getText) - .map(StringUtils::unquoteIdentifier) + .map(StringUtils::unquoteText) .collect(Collectors.toList()) ); } @Override public UnresolvedExpression visitStringLiteral(StringLiteralContext ctx) { - return new Literal(unquoteIdentifier(ctx.getText()), DataType.STRING); + return new Literal(StringUtils.unquoteText(ctx.getText()), DataType.STRING); } @Override diff --git a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java index aaa839d0b2..6167639ed5 100644 --- a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java +++ b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java @@ -15,7 +15,6 @@ package com.amazon.opendistroforelasticsearch.sql.ppl.utils; -import static com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils.unquoteIdentifier; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.BooleanLiteralContext; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.DedupCommandContext; import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.FieldsCommandContext; @@ -27,6 +26,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; import com.amazon.opendistroforelasticsearch.sql.ast.expression.DataType; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Literal; +import com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -140,7 +140,7 @@ private static Literal getArgumentValue(ParserRuleContext ctx) { ? new Literal(Integer.parseInt(ctx.getText()), DataType.INTEGER) : ctx instanceof BooleanLiteralContext ? new Literal(Boolean.valueOf(ctx.getText()), DataType.BOOLEAN) - : new Literal(unquoteIdentifier(ctx.getText()), DataType.STRING); + : new Literal(StringUtils.unquoteText(ctx.getText()), DataType.STRING); } } diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java index a8d10bb935..dd3da3f91f 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilder.java @@ -16,7 +16,6 @@ package com.amazon.opendistroforelasticsearch.sql.sql.parser; -import static com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils.unquoteIdentifier; import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.FromClauseContext; import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.SelectClauseContext; import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.SelectElementContext; @@ -29,6 +28,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.tree.UnresolvedPlan; import com.amazon.opendistroforelasticsearch.sql.ast.tree.Values; import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; +import com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.QuerySpecificationContext; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParserBaseVisitor; import com.google.common.collect.ImmutableList; @@ -104,13 +104,13 @@ private UnresolvedExpression visitAstExpression(ParseTree tree) { } private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { - String name = unquoteIdentifier(getTextInQuery(ctx.expression())); + String name = StringUtils.unquoteIdentifier(getTextInQuery(ctx.expression())); UnresolvedExpression expr = visitAstExpression(ctx.expression()); if (ctx.alias() == null) { return new Alias(name, expr); } else { - String alias = unquoteIdentifier(ctx.alias().getText()); + String alias = StringUtils.unquoteIdentifier(ctx.alias().getText()); return new Alias(name, expr, alias); } } diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java index ac663dfbdc..1dadde63ff 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java @@ -16,7 +16,6 @@ package com.amazon.opendistroforelasticsearch.sql.sql.parser; -import static com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils.unquoteIdentifier; 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.expression.function.BuiltinFunctionName.LIKE; @@ -114,7 +113,7 @@ public UnresolvedExpression visitLikePredicate(OpenDistroSQLParser.LikePredicate @Override public UnresolvedExpression visitString(StringContext ctx) { - return AstDSL.stringLiteral(unquoteIdentifier(ctx.getText())); + return AstDSL.stringLiteral(StringUtils.unquoteText(ctx.getText())); } @Override @@ -139,18 +138,18 @@ public UnresolvedExpression visitNullLiteral(OpenDistroSQLParser.NullLiteralCont @Override public UnresolvedExpression visitDateLiteral(OpenDistroSQLParser.DateLiteralContext ctx) { - return AstDSL.dateLiteral(unquoteIdentifier(ctx.date.getText())); + return AstDSL.dateLiteral(StringUtils.unquoteText(ctx.date.getText())); } @Override public UnresolvedExpression visitTimeLiteral(OpenDistroSQLParser.TimeLiteralContext ctx) { - return AstDSL.timeLiteral(unquoteIdentifier(ctx.time.getText())); + return AstDSL.timeLiteral(StringUtils.unquoteText(ctx.time.getText())); } @Override public UnresolvedExpression visitTimestampLiteral( OpenDistroSQLParser.TimestampLiteralContext ctx) { - return AstDSL.timestampLiteral(unquoteIdentifier(ctx.timestamp.getText())); + return AstDSL.timestampLiteral(StringUtils.unquoteText(ctx.timestamp.getText())); } @Override diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java index 9bc6970e38..4a80334b94 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java @@ -49,7 +49,7 @@ public void can_build_select_literals() { project( values(emptyList()), alias("123", intLiteral(123)), - alias("hello", stringLiteral("hello")), + alias("'hello'", stringLiteral("hello")), alias("false", booleanLiteral(false)), alias("-4.567", doubleLiteral(-4.567)) ),