From 2c5cfcfae44552fef09a099ba7ffb2e5f6f8fe51 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 20 Dec 2017 09:25:00 -0500 Subject: [PATCH] SQL: Proper errors on set qualifiers (elastic/x-pack-elasticsearch#3377) `SELECT DISTINCT foo FROM bar` is not yet implemented and was returning an "unplanned item" error which isn't useful to users so I replaced it with `SELECT DISTINCT is not yet supported`. `SELECT foo, COUNT(*) FROM bar GROUP BY ALL foo` is not supported. Specifically the `ALL` part. It is fairly esoteric so see [1] for what it does. Regardless of what it does it is not widely supported and even Microsoft's SQL Server has deprecated it so we should never support it. Probably. [1]: https://technet.microsoft.com/en-us/library/ms175028(v=sql.90).aspx Related to elastic/x-pack-elasticsearch#3176 Original commit: elastic/x-pack-elasticsearch@56e5ca3009574dba4b6dd80ecd2cca7e43f4d128 --- .../xpack/qa/sql/rest/RestSqlTestCase.java | 12 ++++++++++++ .../xpack/sql/analysis/analyzer/Verifier.java | 6 ++++-- .../xpack/sql/parser/LogicalPlanBuilder.java | 15 ++++++++++++--- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java index 8b789087225..f9a45d0a14b 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java @@ -162,6 +162,18 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe containsString("line 1:67: Queries with JOIN are not yet supported")); } + public void testSelectDistinctFails() throws Exception { + index("{\"name\":\"test\"}"); + expectBadRequest(() -> runSql("SELECT DISTINCT name FROM test"), + containsString("line 1:8: SELECT DISTINCT is not yet supported")); + } + + public void testSelectGroupByAllFails() throws Exception { + index("{\"foo\":1}", "{\"foo\":2}"); + expectBadRequest(() -> runSql("SELECT foo FROM test GROUP BY ALL foo"), + containsString("line 1:32: GROUP BY ALL is not supported")); + } + @Override public void testSelectInvalidSql() { expectBadRequest(() -> runSql("SELECT * FRO"), containsString("1:8: Cannot determine columns for *")); diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index cb49af38e9a..91f729809e3 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -18,6 +18,7 @@ import org.elasticsearch.xpack.sql.expression.function.Functions; import org.elasticsearch.xpack.sql.expression.function.Score; import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction; import org.elasticsearch.xpack.sql.plan.logical.Aggregate; +import org.elasticsearch.xpack.sql.plan.logical.Distinct; import org.elasticsearch.xpack.sql.plan.logical.Filter; import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.sql.plan.logical.OrderBy; @@ -107,8 +108,9 @@ abstract class Verifier { if (p instanceof Unresolvable) { localFailures.add(fail(p, ((Unresolvable) p).unresolvedMessage())); - } - else { + } else if (p instanceof Distinct) { + localFailures.add(fail(p, "SELECT DISTINCT is not yet supported")); + } else { // then take a look at the expressions p.forEachExpressions(e -> { // everything is fine, skip expression diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java index 9c71872721f..ac293c2e137 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.sql.parser; +import org.antlr.v4.runtime.tree.TerminalNode; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Literal; import org.elasticsearch.xpack.sql.expression.NamedExpression; @@ -13,6 +14,7 @@ import org.elasticsearch.xpack.sql.expression.UnresolvedAlias; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.AliasedQueryContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.AliasedRelationContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.FromClauseContext; +import org.elasticsearch.xpack.sql.parser.SqlBaseParser.GroupByContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.JoinCriteriaContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.JoinRelationContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.JoinTypeContext; @@ -21,6 +23,7 @@ import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryNoWithContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QuerySpecificationContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.RelationContext; +import org.elasticsearch.xpack.sql.parser.SqlBaseParser.SetQuantifierContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.SubqueryContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.TableNameContext; import org.elasticsearch.xpack.sql.plan.TableIdentifier; @@ -109,9 +112,15 @@ abstract class LogicalPlanBuilder extends ExpressionBuilder { } // GROUP BY - if (ctx.groupBy() != null) { - List groupBy = expressions(ctx.groupBy().groupingElement()); - query = new Aggregate(source(ctx.groupBy()), query, groupBy, selectTarget); + GroupByContext groupByCtx = ctx.groupBy(); + if (groupByCtx != null) { + SetQuantifierContext setQualifierContext = groupByCtx.setQuantifier(); + TerminalNode groupByAll = setQualifierContext == null ? null : setQualifierContext.ALL(); + if (groupByAll != null) { + throw new ParsingException(source(groupByAll), "GROUP BY ALL is not supported"); + } + List groupBy = expressions(groupByCtx.groupingElement()); + query = new Aggregate(source(groupByCtx), query, groupBy, selectTarget); } else if (!selectTarget.isEmpty()) { query = new Project(source(ctx.selectItem(0)), query, selectTarget);