Skip to content

Commit

Permalink
SQL: Remove exceptions from Analyzer
Browse files Browse the repository at this point in the history 
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 elastic#38208
  • Loading branch information
@costin
costin committed Feb 2, 2019
1 parent 291a34c commit 7961bf7
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 14 deletions.
23 changes: 14 additions & 9 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()));
}
}
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 9 additions & 0 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).
*/
Expand Down
13 changes: 10 additions & 3 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<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();
Expand Down Expand Up @@ -131,8 +136,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);
Expand Down
23 changes: 21 additions & 2 deletions ...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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"));
}
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 7961bf7

Please sign in to comment.