Skip to content

Commit 75f0750

Browse files
costincostin
authored
SQL: Remove exceptions from Analyzer (#38260)
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
1 parent a088155 commit 75f0750

File tree

4 files changed

+63
-17
lines changed

4 files changed

+63
-17
lines changed

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66
package org.elasticsearch.xpack.sql.analysis.analyzer;
77

8-
import org.elasticsearch.xpack.sql.analysis.AnalysisException;
8+
import org.elasticsearch.common.logging.LoggerMessageFormat;
99
import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier.Failure;
1010
import org.elasticsearch.xpack.sql.analysis.index.IndexResolution;
1111
import org.elasticsearch.xpack.sql.capabilities.Resolvables;
@@ -16,7 +16,7 @@
1616
import org.elasticsearch.xpack.sql.expression.Expression;
1717
import org.elasticsearch.xpack.sql.expression.Expressions;
1818
import org.elasticsearch.xpack.sql.expression.FieldAttribute;
19-
import org.elasticsearch.xpack.sql.expression.Literal;
19+
import org.elasticsearch.xpack.sql.expression.Foldables;
2020
import org.elasticsearch.xpack.sql.expression.NamedExpression;
2121
import org.elasticsearch.xpack.sql.expression.Order;
2222
import org.elasticsearch.xpack.sql.expression.SubQueryExpression;
@@ -516,6 +516,7 @@ protected LogicalPlan rule(LogicalPlan plan) {
516516
int max = ordinalReference.size();
517517

518518
for (Order order : orderBy.order()) {
519+
Expression child = order.child();
519520
Integer ordinal = findOrdinal(order.child());
520521
if (ordinal != null) {
521522
changed = true;
@@ -524,7 +525,11 @@ protected LogicalPlan rule(LogicalPlan plan) {
524525
order.nullsPosition()));
525526
}
526527
else {
527-
throw new AnalysisException(order, "Invalid %d specified in OrderBy (valid range is [1, %d])", ordinal, max);
528+
// report error
529+
String message = LoggerMessageFormat.format("Invalid ordinal [{}] specified in [{}] (valid range is [1, {}])",
530+
ordinal, orderBy.sourceText(), max);
531+
UnresolvedAttribute ua = new UnresolvedAttribute(child.source(), orderBy.sourceText(), null, message);
532+
newOrder.add(new Order(order.source(), ua, order.direction(), order.nullsPosition()));
528533
}
529534
}
530535
else {
@@ -551,17 +556,24 @@ protected LogicalPlan rule(LogicalPlan plan) {
551556
Integer ordinal = findOrdinal(exp);
552557
if (ordinal != null) {
553558
changed = true;
559+
String errorMessage = null;
554560
if (ordinal > 0 && ordinal <= max) {
555561
NamedExpression reference = aggregates.get(ordinal - 1);
556562
if (containsAggregate(reference)) {
557-
throw new AnalysisException(exp, "Group ordinal " + ordinal + " refers to an aggregate function "
558-
+ reference.nodeName() + " which is not compatible/allowed with GROUP BY");
563+
errorMessage = LoggerMessageFormat.format(
564+
"Ordinal [{}] in [{}] refers to an invalid argument, aggregate function [{}]",
565+
ordinal, agg.sourceText(), reference.sourceText());
566+
567+
} else {
568+
newGroupings.add(reference);
559569
}
560-
newGroupings.add(reference);
561570
}
562571
else {
563-
throw new AnalysisException(exp, "Invalid ordinal " + ordinal
564-
+ " specified in Aggregate (valid range is [1, " + max + "])");
572+
errorMessage = LoggerMessageFormat.format("Invalid ordinal [{}] specified in [{}] (valid range is [1, {}])",
573+
ordinal, agg.sourceText(), max);
574+
}
575+
if (errorMessage != null) {
576+
newGroupings.add(new UnresolvedAttribute(exp.source(), agg.sourceText(), null, errorMessage));
565577
}
566578
}
567579
else {
@@ -576,10 +588,9 @@ protected LogicalPlan rule(LogicalPlan plan) {
576588
}
577589

578590
private Integer findOrdinal(Expression expression) {
579-
if (expression instanceof Literal) {
580-
Literal l = (Literal) expression;
581-
if (l.dataType().isInteger()) {
582-
Object v = l.value();
591+
if (expression.foldable()) {
592+
if (expression.dataType().isInteger()) {
593+
Object v = Foldables.valueOf(expression);
583594
if (v instanceof Number) {
584595
return Integer.valueOf(((Number) v).intValue());
585596
}

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,15 @@ Source source(ParserRuleContext begin, ParserRuleContext end) {
102102
return new Source(new Location(start.getLine(), start.getCharPositionInLine()), text);
103103
}
104104

105+
static Source source(TerminalNode begin, ParserRuleContext end) {
106+
Check.notNull(begin, "begin is null");
107+
Check.notNull(end, "end is null");
108+
Token start = begin.getSymbol();
109+
Token stop = end.stop != null ? end.stop : start;
110+
String text = start.getInputStream().getText(new Interval(start.getStartIndex(), stop.getStopIndex()));
111+
return new Source(new Location(start.getLine(), start.getCharPositionInLine()), text);
112+
}
113+
105114
/**
106115
* Retrieves the raw text of the node (without interpreting it as a string literal).
107116
*/

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
package org.elasticsearch.xpack.sql.parser;
77

8+
import org.antlr.v4.runtime.ParserRuleContext;
89
import org.antlr.v4.runtime.Token;
910
import org.antlr.v4.runtime.tree.TerminalNode;
1011
import org.elasticsearch.xpack.sql.expression.Expression;
@@ -16,10 +17,12 @@
1617
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.AliasedRelationContext;
1718
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.FromClauseContext;
1819
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.GroupByContext;
20+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.GroupingElementContext;
1921
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.JoinCriteriaContext;
2022
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.JoinRelationContext;
2123
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.LimitClauseContext;
2224
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.NamedQueryContext;
25+
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.OrderByContext;
2326
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryContext;
2427
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QueryNoWithContext;
2528
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.QuerySpecificationContext;
@@ -85,7 +88,9 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) {
8588
LogicalPlan plan = plan(ctx.queryTerm());
8689

8790
if (!ctx.orderBy().isEmpty()) {
88-
plan = new OrderBy(source(ctx.ORDER()), plan, visitList(ctx.orderBy(), Order.class));
91+
List<OrderByContext> orders = ctx.orderBy();
92+
OrderByContext endContext = orders.get(orders.size() - 1);
93+
plan = new OrderBy(source(ctx.ORDER(), endContext), plan, visitList(ctx.orderBy(), Order.class));
8994
}
9095

9196
LimitClauseContext limitClause = ctx.limitClause();
@@ -131,8 +136,10 @@ public LogicalPlan visitQuerySpecification(QuerySpecificationContext ctx) {
131136
if (groupByAll != null) {
132137
throw new ParsingException(source(groupByAll), "GROUP BY ALL is not supported");
133138
}
134-
List<Expression> groupBy = expressions(groupByCtx.groupingElement());
135-
query = new Aggregate(source(groupByCtx), query, groupBy, selectTarget);
139+
List<GroupingElementContext> groupingElement = groupByCtx.groupingElement();
140+
List<Expression> groupBy = expressions(groupingElement);
141+
ParserRuleContext endSource = groupingElement.isEmpty() ? groupByCtx : groupingElement.get(groupingElement.size() - 1);
142+
query = new Aggregate(source(ctx.GROUP(), endSource), query, groupBy, selectTarget);
136143
}
137144
else if (!selectTarget.isEmpty()) {
138145
query = new Project(source(ctx.selectItem(0)), query, selectTarget);

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
import org.elasticsearch.test.ESTestCase;
99
import org.elasticsearch.xpack.sql.TestUtils;
10-
import org.elasticsearch.xpack.sql.analysis.AnalysisException;
1110
import org.elasticsearch.xpack.sql.analysis.index.EsIndex;
1211
import org.elasticsearch.xpack.sql.analysis.index.IndexResolution;
1312
import org.elasticsearch.xpack.sql.analysis.index.IndexResolverTests;
@@ -42,7 +41,7 @@ private String error(String sql) {
4241

4342
private String error(IndexResolution getIndexResult, String sql) {
4443
Analyzer analyzer = new Analyzer(TestUtils.TEST_CFG, new FunctionRegistry(), getIndexResult, new Verifier(new Metrics()));
45-
AnalysisException e = expectThrows(AnalysisException.class, () -> analyzer.analyze(parser.createStatement(sql), true));
44+
VerificationException e = expectThrows(VerificationException.class, () -> analyzer.analyze(parser.createStatement(sql), true));
4645
assertTrue(e.getMessage().startsWith("Found "));
4746
String header = "Found 1 problem(s)\nline ";
4847
return e.getMessage().substring(header.length());
@@ -170,6 +169,11 @@ public void testMissingColumnInGroupBy() {
170169
assertEquals("1:41: Unknown column [xxx]", error("SELECT * FROM test GROUP BY DAY_OF_YEAR(xxx)"));
171170
}
172171

172+
public void testInvalidOrdinalInOrderBy() {
173+
assertEquals("1:56: Invalid ordinal [3] specified in [ORDER BY 2, 3] (valid range is [1, 2])",
174+
error("SELECT bool, MIN(int) FROM test GROUP BY 1 ORDER BY 2, 3"));
175+
}
176+
173177
public void testFilterOnUnknownColumn() {
174178
assertEquals("1:26: Unknown column [xxx]", error("SELECT * FROM test WHERE xxx = 1"));
175179
}
@@ -239,6 +243,21 @@ public void testGroupByOrderByNonGrouped_WithHaving() {
239243
error("SELECT MAX(int) FROM test GROUP BY text HAVING MAX(int) > 10 ORDER BY bool"));
240244
}
241245

246+
public void testGroupByOrdinalPointingToAggregate() {
247+
assertEquals("1:42: Ordinal [2] in [GROUP BY 2] refers to an invalid argument, aggregate function [MIN(int)]",
248+
error("SELECT bool, MIN(int) FROM test GROUP BY 2"));
249+
}
250+
251+
public void testGroupByInvalidOrdinal() {
252+
assertEquals("1:42: Invalid ordinal [3] specified in [GROUP BY 3] (valid range is [1, 2])",
253+
error("SELECT bool, MIN(int) FROM test GROUP BY 3"));
254+
}
255+
256+
public void testGroupByNegativeOrdinal() {
257+
assertEquals("1:42: Invalid ordinal [-1] specified in [GROUP BY -1] (valid range is [1, 2])",
258+
error("SELECT bool, MIN(int) FROM test GROUP BY -1"));
259+
}
260+
242261
public void testGroupByOrderByAliasedInSelectAllowed() {
243262
LogicalPlan lp = accept("SELECT text t FROM test GROUP BY text ORDER BY t");
244263
assertNotNull(lp);

0 commit comments

Comments
 (0)