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 3a8931025b8ef..ccc8a1e798a67 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,7 +5,7 @@ */ package org.elasticsearch.xpack.sql.analysis.analyzer; -import org.elasticsearch.xpack.sql.analysis.AnalysisException; +import org.elasticsearch.common.logging.LoggerMessageFormat; import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier.Failure; import org.elasticsearch.xpack.sql.analysis.index.IndexResolution; import org.elasticsearch.xpack.sql.capabilities.Resolvables; @@ -16,7 +16,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 +516,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 +525,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(), orderBy.sourceText(), null, message); + newOrder.add(new Order(order.source(), ua, order.direction(), order.nullsPosition())); } } else { @@ -551,17 +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 " + ordinal + " refers to an aggregate function " - + reference.nodeName() + " which is not compatible/allowed with GROUP BY"); + 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 " + ordinal - + " specified in Aggregate (valid range is [1, " + 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 { @@ -576,10 +588,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()); } 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 cb758904624fd..c153a800250a8 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,11 +17,13 @@ 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.JoinTypeContext; 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; @@ -87,7 +90,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(); @@ -133,8 +138,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 c603a6b97cae0..992ab6e1905fe 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);