From 291a34c7154921a7779eaa6e37755f83dad1de43 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Fri, 1 Feb 2019 18:21:24 +0200 Subject: [PATCH 1/2] Remove AnalysisExceptions as it trips the Verifier --- .../xpack/sql/analysis/analyzer/Analyzer.java | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 38009399b8e11..4c6744936fd54 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.sql.analysis.analyzer; +import org.elasticsearch.common.logging.LoggerMessageFormat; import org.elasticsearch.xpack.sql.analysis.AnalysisException; import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier.Failure; import org.elasticsearch.xpack.sql.analysis.index.IndexResolution; @@ -16,7 +17,7 @@ import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; import org.elasticsearch.xpack.sql.expression.FieldAttribute; -import org.elasticsearch.xpack.sql.expression.Literal; +import org.elasticsearch.xpack.sql.expression.Foldables; import org.elasticsearch.xpack.sql.expression.NamedExpression; import org.elasticsearch.xpack.sql.expression.Order; import org.elasticsearch.xpack.sql.expression.SubQueryExpression; @@ -516,6 +517,7 @@ protected LogicalPlan rule(LogicalPlan plan) { int max = ordinalReference.size(); for (Order order : orderBy.order()) { + Expression child = order.child(); Integer ordinal = findOrdinal(order.child()); if (ordinal != null) { changed = true; @@ -524,7 +526,11 @@ protected LogicalPlan rule(LogicalPlan plan) { order.nullsPosition())); } else { - throw new AnalysisException(order, "Invalid %d specified in OrderBy (valid range is [1, %d])", ordinal, max); + // report error + String message = LoggerMessageFormat.format("Invalid ordinal [{}] specified in {} (valid range is [1, {}])", + ordinal, orderBy.sourceText(), max); + UnresolvedAttribute ua = new UnresolvedAttribute(child.source(), child.sourceText(), null, message); + newOrder.add(new Order(order.source(), ua, order.direction(), order.nullsPosition())); } } else { @@ -554,14 +560,15 @@ protected LogicalPlan rule(LogicalPlan plan) { if (ordinal > 0 && ordinal <= max) { NamedExpression reference = aggregates.get(ordinal - 1); if (containsAggregate(reference)) { - throw new AnalysisException(exp, "Group ordinal " + ordinal + " refers to an aggregate function " - + reference.nodeName() + " which is not compatible/allowed with GROUP BY"); + throw new AnalysisException(exp, + "Group ordinal {} refers to an aggregate function [{}] which is not compatible/allowed with GROUP BY", + ordinal, reference.source()); } newGroupings.add(reference); } else { - throw new AnalysisException(exp, "Invalid ordinal " + ordinal - + " specified in Aggregate (valid range is [1, " + max + "])"); + throw new AnalysisException(exp, "Invalid ordinal [{}] specified in {} (valid range is [1, {}])", + agg.sourceText(), ordinal, max); } } else { @@ -576,10 +583,9 @@ protected LogicalPlan rule(LogicalPlan plan) { } private Integer findOrdinal(Expression expression) { - if (expression instanceof Literal) { - Literal l = (Literal) expression; - if (l.dataType().isInteger()) { - Object v = l.value(); + if (expression.foldable()) { + if (expression.dataType().isInteger()) { + Object v = Foldables.valueOf(expression); if (v instanceof Number) { return Integer.valueOf(((Number) v).intValue()); } From 7961bf79ddcb8877d8d2aa40748a85fe52218f70 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Sat, 2 Feb 2019 18:40:50 +0200 Subject: [PATCH 2/2] SQL: Remove exceptions from Analyzer Instead of throwing an exception, use an unresolved attribute to pass the message to the Verifier. Additionally improve the parser to save the extended source for the Aggregate and OrderBy. Close #38208 --- .../xpack/sql/analysis/analyzer/Analyzer.java | 23 +++++++++++-------- .../xpack/sql/parser/AbstractBuilder.java | 9 ++++++++ .../xpack/sql/parser/LogicalPlanBuilder.java | 13 ++++++++--- .../analyzer/VerifierErrorMessagesTests.java | 23 +++++++++++++++++-- 4 files changed, 54 insertions(+), 14 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 4c6744936fd54..f6bd4c6dc9b50 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.analysis.analyzer; import org.elasticsearch.common.logging.LoggerMessageFormat; -import org.elasticsearch.xpack.sql.analysis.AnalysisException; import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier.Failure; import org.elasticsearch.xpack.sql.analysis.index.IndexResolution; import org.elasticsearch.xpack.sql.capabilities.Resolvables; @@ -527,9 +526,9 @@ protected LogicalPlan rule(LogicalPlan plan) { } else { // report error - String message = LoggerMessageFormat.format("Invalid ordinal [{}] specified in {} (valid range is [1, {}])", + String message = LoggerMessageFormat.format("Invalid ordinal [{}] specified in [{}] (valid range is [1, {}])", ordinal, orderBy.sourceText(), max); - UnresolvedAttribute ua = new UnresolvedAttribute(child.source(), child.sourceText(), null, message); + UnresolvedAttribute ua = new UnresolvedAttribute(child.source(), orderBy.sourceText(), null, message); newOrder.add(new Order(order.source(), ua, order.direction(), order.nullsPosition())); } } @@ -557,18 +556,24 @@ protected LogicalPlan rule(LogicalPlan plan) { Integer ordinal = findOrdinal(exp); if (ordinal != null) { changed = true; + String errorMessage = null; if (ordinal > 0 && ordinal <= max) { NamedExpression reference = aggregates.get(ordinal - 1); if (containsAggregate(reference)) { - throw new AnalysisException(exp, - "Group ordinal {} refers to an aggregate function [{}] which is not compatible/allowed with GROUP BY", - ordinal, reference.source()); + errorMessage = LoggerMessageFormat.format( + "Ordinal [{}] in [{}] refers to an invalid argument, aggregate function [{}]", + ordinal, agg.sourceText(), reference.sourceText()); + + } else { + newGroupings.add(reference); } - newGroupings.add(reference); } else { - throw new AnalysisException(exp, "Invalid ordinal [{}] specified in {} (valid range is [1, {}])", - agg.sourceText(), ordinal, max); + errorMessage = LoggerMessageFormat.format("Invalid ordinal [{}] specified in [{}] (valid range is [1, {}])", + ordinal, agg.sourceText(), max); + } + if (errorMessage != null) { + newGroupings.add(new UnresolvedAttribute(exp.source(), agg.sourceText(), null, errorMessage)); } } else { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java index 81c2e7578ccb9..aa5af5389a796 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java @@ -102,6 +102,15 @@ Source source(ParserRuleContext begin, ParserRuleContext end) { return new Source(new Location(start.getLine(), start.getCharPositionInLine()), text); } + static Source source(TerminalNode begin, ParserRuleContext end) { + Check.notNull(begin, "begin is null"); + Check.notNull(end, "end is null"); + Token start = begin.getSymbol(); + Token stop = end.stop != null ? end.stop : start; + String text = start.getInputStream().getText(new Interval(start.getStartIndex(), stop.getStopIndex())); + return new Source(new Location(start.getLine(), start.getCharPositionInLine()), text); + } + /** * Retrieves the raw text of the node (without interpreting it as a string literal). */ diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java index e4cc65cf50500..f27368912c1d2 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/sql/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.ParserRuleContext; import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.TerminalNode; import org.elasticsearch.xpack.sql.expression.Expression; @@ -16,10 +17,12 @@ 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.GroupingElementContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.JoinCriteriaContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.JoinRelationContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.LimitClauseContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.NamedQueryContext; +import org.elasticsearch.xpack.sql.parser.SqlBaseParser.OrderByContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryNoWithContext; import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QuerySpecificationContext; @@ -85,7 +88,9 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) { LogicalPlan plan = plan(ctx.queryTerm()); if (!ctx.orderBy().isEmpty()) { - plan = new OrderBy(source(ctx.ORDER()), plan, visitList(ctx.orderBy(), Order.class)); + List orders = ctx.orderBy(); + OrderByContext endContext = orders.get(orders.size() - 1); + plan = new OrderBy(source(ctx.ORDER(), endContext), plan, visitList(ctx.orderBy(), Order.class)); } LimitClauseContext limitClause = ctx.limitClause(); @@ -131,8 +136,10 @@ public LogicalPlan visitQuerySpecification(QuerySpecificationContext ctx) { 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); + List groupingElement = groupByCtx.groupingElement(); + List groupBy = expressions(groupingElement); + ParserRuleContext endSource = groupingElement.isEmpty() ? groupByCtx : groupingElement.get(groupingElement.size() - 1); + query = new Aggregate(source(ctx.GROUP(), endSource), query, groupBy, selectTarget); } else if (!selectTarget.isEmpty()) { query = new Project(source(ctx.selectItem(0)), query, selectTarget); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 2fa5946700a79..0c13461b7eeb5 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -7,7 +7,6 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.TestUtils; -import org.elasticsearch.xpack.sql.analysis.AnalysisException; import org.elasticsearch.xpack.sql.analysis.index.EsIndex; import org.elasticsearch.xpack.sql.analysis.index.IndexResolution; import org.elasticsearch.xpack.sql.analysis.index.IndexResolverTests; @@ -42,7 +41,7 @@ private String error(String sql) { private String error(IndexResolution getIndexResult, String sql) { Analyzer analyzer = new Analyzer(TestUtils.TEST_CFG, new FunctionRegistry(), getIndexResult, new Verifier(new Metrics())); - AnalysisException e = expectThrows(AnalysisException.class, () -> analyzer.analyze(parser.createStatement(sql), true)); + VerificationException e = expectThrows(VerificationException.class, () -> analyzer.analyze(parser.createStatement(sql), true)); assertTrue(e.getMessage().startsWith("Found ")); String header = "Found 1 problem(s)\nline "; return e.getMessage().substring(header.length()); @@ -170,6 +169,11 @@ public void testMissingColumnInGroupBy() { assertEquals("1:41: Unknown column [xxx]", error("SELECT * FROM test GROUP BY DAY_OF_YEAR(xxx)")); } + public void testInvalidOrdinalInOrderBy() { + assertEquals("1:56: Invalid ordinal [3] specified in [ORDER BY 2, 3] (valid range is [1, 2])", + error("SELECT bool, MIN(int) FROM test GROUP BY 1 ORDER BY 2, 3")); + } + public void testFilterOnUnknownColumn() { assertEquals("1:26: Unknown column [xxx]", error("SELECT * FROM test WHERE xxx = 1")); } @@ -239,6 +243,21 @@ public void testGroupByOrderByNonGrouped_WithHaving() { error("SELECT MAX(int) FROM test GROUP BY text HAVING MAX(int) > 10 ORDER BY bool")); } + public void testGroupByOrdinalPointingToAggregate() { + assertEquals("1:42: Ordinal [2] in [GROUP BY 2] refers to an invalid argument, aggregate function [MIN(int)]", + error("SELECT bool, MIN(int) FROM test GROUP BY 2")); + } + + public void testGroupByInvalidOrdinal() { + assertEquals("1:42: Invalid ordinal [3] specified in [GROUP BY 3] (valid range is [1, 2])", + error("SELECT bool, MIN(int) FROM test GROUP BY 3")); + } + + public void testGroupByNegativeOrdinal() { + assertEquals("1:42: Invalid ordinal [-1] specified in [GROUP BY -1] (valid range is [1, 2])", + error("SELECT bool, MIN(int) FROM test GROUP BY -1")); + } + public void testGroupByOrderByAliasedInSelectAllowed() { LogicalPlan lp = accept("SELECT text t FROM test GROUP BY text ORDER BY t"); assertNotNull(lp);