elastic · costin · Feb 4, 2019 · Feb 3, 2019
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
@@ -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
@@ -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
@@ -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<OrderByContext> 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<Expression> groupBy = expressions(groupByCtx.groupingElement());
-            query = new Aggregate(source(groupByCtx), query, groupBy, selectTarget);
+            List<GroupingElementContext> groupingElement = groupByCtx.groupingElement();
+            List<Expression> 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/...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/...c/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);