From dc69f92b495447d2ec38bf848bd6e274eced8fd0 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 18 Dec 2017 16:42:03 -0500 Subject: [PATCH] SQL: Respond with nice error if there is a JOIN (elastic/x-pack-elasticsearch#3343) `JOIN`s aren't supported right yet so we should send back a nice 400 level error to the user telling them that. Also pulls out some common code in `RestSqlTestCase` that I've been staring at for a while. Relates to elastic/x-pack-elasticsearch#3176 Original commit: elastic/x-pack-elasticsearch@1c1bd1c3550cb87dca64389e360865745f1fdea3 --- .../xpack/qa/sql/rest/RestSqlTestCase.java | 88 +++++++++---------- .../sql/analysis/index/MappingException.java | 6 ++ .../xpack/sql/parser/LogicalPlanBuilder.java | 16 ++-- .../xpack/sql/parser/ParsingException.java | 6 ++ .../xpack/sql/session/SqlSession.java | 14 +-- 5 files changed, 70 insertions(+), 60 deletions(-) diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java index 64aa45e2c8c..e96ca4e43d9 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java @@ -52,13 +52,8 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe } public void testBasicQuery() throws IOException { - StringBuilder bulk = new StringBuilder(); - bulk.append("{\"index\":{\"_id\":\"1\"}}\n"); - bulk.append("{\"test\":\"test\"}\n"); - bulk.append("{\"index\":{\"_id\":\"2\"}}\n"); - bulk.append("{\"test\":\"test\"}\n"); - client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), - new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); + index("{\"test\":\"test\"}", + "{\"test\":\"test\"}"); Map expected = new HashMap<>(); expected.put("columns", singletonList(columnInfo("test", "text"))); @@ -110,13 +105,8 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe @AwaitsFix(bugUrl = "https://github.com/elastic/x-pack-elasticsearch/issues/2074") public void testTimeZone() throws IOException { - StringBuilder bulk = new StringBuilder(); - bulk.append("{\"index\":{\"_id\":\"1\"}}\n"); - bulk.append("{\"test\":\"2017-07-27 00:00:00\"}\n"); - bulk.append("{\"index\":{\"_id\":\"2\"}}\n"); - bulk.append("{\"test\":\"2017-07-27 01:00:00\"}\n"); - client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), - new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); + index("{\"test\":\"2017-07-27 00:00:00\"}", + "{\"test\":\"2017-07-27 01:00:00\"}"); Map expected = new HashMap<>(); expected.put("columns", singletonMap("test", singletonMap("type", "text"))); @@ -128,6 +118,24 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe new StringEntity("{\"query\":\"SELECT DAY_OF_YEAR(test), COUNT(*) FROM test\"}", ContentType.APPLICATION_JSON))); } + public void testSelectWithJoinFails() throws Exception { + // Normal join not supported + expectBadRequest(() -> runSql("SELECT * FROM test JOIN other"), + containsString("line 1:21: Queries with JOIN are not yet supported")); + // Neither is a self join + expectBadRequest(() -> runSql("SELECT * FROM test JOIN test"), + containsString("line 1:21: Queries with JOIN are not yet supported")); + // Nor fancy stuff like CTEs + expectBadRequest(() -> runSql( + " WITH evil" + + " AS (SELECT *" + + " FROM foo)" + + "SELECT *" + + " FROM test" + + " JOIN evil"), + containsString("line 1:67: Queries with JOIN are not yet supported")); + } + @Override public void testSelectInvalidSql() { expectBadRequest(() -> runSql("SELECT * FRO"), containsString("1:8: Cannot determine columns for *")); @@ -149,26 +157,26 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe @Override public void testSelectMissingField() throws IOException { - StringBuilder bulk = new StringBuilder(); - bulk.append("{\"index\":{\"_id\":\"1\"}}\n"); - bulk.append("{\"test\":\"test\"}\n"); - client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), - new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); - + index("{\"test\":\"test\"}"); expectBadRequest(() -> runSql("SELECT foo FROM test"), containsString("1:8: Unknown column [foo]")); } @Override public void testSelectMissingFunction() throws Exception { - StringBuilder bulk = new StringBuilder(); - bulk.append("{\"index\":{\"_id\":\"1\"}}\n"); - bulk.append("{\"foo\":1}\n"); - client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), - new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); - + index("{\"foo\":1}"); expectBadRequest(() -> runSql("SELECT missing(foo) FROM test"), containsString("1:8: Unknown function [missing]")); } + private void index(String... docs) throws IOException { + StringBuilder bulk = new StringBuilder(); + for (String doc : docs) { + bulk.append("{\"index\":{}\n"); + bulk.append(doc + "\n"); + } + client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), + new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); + } + private void expectBadRequest(ThrowingRunnable code, Matcher errorMessageMatcher) { ResponseException e = expectThrows(ResponseException.class, code); assertEquals(e.getMessage(), 400, e.getResponse().getStatusLine().getStatusCode()); @@ -217,13 +225,8 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe } public void testBasicQueryWithFilter() throws IOException { - StringBuilder bulk = new StringBuilder(); - bulk.append("{\"index\":{\"_id\":\"1\"}}\n"); - bulk.append("{\"test\":\"foo\"}\n"); - bulk.append("{\"index\":{\"_id\":\"2\"}}\n"); - bulk.append("{\"test\":\"bar\"}\n"); - client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), - new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); + index("{\"test\":\"foo\"}", + "{\"test\":\"bar\"}"); Map expected = new HashMap<>(); expected.put("columns", singletonList(columnInfo("test", "text"))); @@ -234,13 +237,8 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe } public void testBasicTranslateQueryWithFilter() throws IOException { - StringBuilder bulk = new StringBuilder(); - bulk.append("{\"index\":{\"_id\":\"1\"}}\n"); - bulk.append("{\"test\":\"foo\"}\n"); - bulk.append("{\"index\":{\"_id\":\"2\"}}\n"); - bulk.append("{\"test\":\"bar\"}\n"); - client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), - new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); + index("{\"test\":\"foo\"}", + "{\"test\":\"bar\"}"); Map response = runSql("/translate/", new StringEntity("{\"query\":\"SELECT * FROM test\", \"filter\":{\"match\": {\"test\": \"foo\"}}}", @@ -276,13 +274,9 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe } public void testBasicQueryText() throws IOException { - StringBuilder bulk = new StringBuilder(); - bulk.append("{\"index\":{\"_id\":\"1\"}}\n"); - bulk.append("{\"test\":\"test\"}\n"); - bulk.append("{\"index\":{\"_id\":\"2\"}}\n"); - bulk.append("{\"test\":\"test\"}\n"); - client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"), - new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON)); + index("{\"test\":\"test\"}", + "{\"test\":\"test\"}"); + String expected = "test \n" + "---------------\n" + diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/MappingException.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/MappingException.java index 46387936a90..b510aa04830 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/MappingException.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/MappingException.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.sql.analysis.index; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.xpack.sql.ClientSqlException; public class MappingException extends ClientSqlException { @@ -16,4 +17,9 @@ public class MappingException extends ClientSqlException { public MappingException(String message, Throwable ex) { super(message, ex); } + + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java index f522b15ef2e..d2118ac1014 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java @@ -52,7 +52,7 @@ abstract class LogicalPlanBuilder extends ExpressionBuilder { @Override public LogicalPlan visitQuery(QueryContext ctx) { LogicalPlan body = plan(ctx.queryNoWith()); - + List namedQueries = visitList(ctx.namedQuery(), SubQueryAlias.class); // unwrap query (and validate while at it) @@ -78,18 +78,18 @@ abstract class LogicalPlanBuilder extends ExpressionBuilder { if (!ctx.orderBy().isEmpty()) { plan = new OrderBy(source(ctx.ORDER()), plan, visitList(ctx.orderBy(), Order.class)); } - + if (ctx.limit != null && ctx.INTEGER_VALUE() != null) { plan = new Limit(source(ctx.limit), new Literal(source(ctx), Integer.parseInt(ctx.limit.getText()), DataTypes.INTEGER), plan); } - + return plan; } @Override public LogicalPlan visitQuerySpecification(QuerySpecificationContext ctx) { LogicalPlan query = (ctx.fromClause() != null)? plan(ctx.fromClause()) : new LocalRelation(source(ctx), new EmptyExecutable(emptyList())); - + // add WHERE if (ctx.where != null) { query = new Filter(source(ctx), query, expression(ctx.where)); @@ -138,7 +138,7 @@ abstract class LogicalPlanBuilder extends ExpressionBuilder { // check if there are multiple join clauses. ANTLR produces a right nested tree with the left join clause // at the top. However the fields previously references might be used in the following clauses. // As such, swap/reverse the tree. - + LogicalPlan result = plan(ctx.relationPrimary()); for (JoinRelationContext j : ctx.joinRelation()) { result = doJoin(result, j); @@ -174,7 +174,9 @@ abstract class LogicalPlanBuilder extends ExpressionBuilder { } } - return new Join(source(ctx), left, plan(ctx.right), type, condition); + // We would return this if we actually supported JOINs, but we don't yet. + // new Join(source(ctx), left, plan(ctx.right), type, condition); + throw new ParsingException(source(ctx), "Queries with JOIN are not yet supported"); } @Override @@ -198,4 +200,4 @@ abstract class LogicalPlanBuilder extends ExpressionBuilder { TableIdentifier tableIdentifier = visitTableIdentifier(ctx.tableIdentifier()); return new UnresolvedRelation(source(ctx), tableIdentifier, alias); } -} \ No newline at end of file +} diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/ParsingException.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/ParsingException.java index 3382ef240e3..0f874f044fe 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/ParsingException.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/ParsingException.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.parser; import org.antlr.v4.runtime.RecognitionException; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.xpack.sql.ClientSqlException; import org.elasticsearch.xpack.sql.tree.Location; @@ -48,4 +49,9 @@ public class ParsingException extends ClientSqlException { public String getMessage() { return format(Locale.ROOT, "line %s:%s: %s", getLineNumber(), getColumnNumber(), getErrorMessage()); } + + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java index 4a4e5644176..75a58c3d9c4 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java @@ -13,6 +13,7 @@ import org.elasticsearch.xpack.sql.analysis.analyzer.PreAnalyzer; import org.elasticsearch.xpack.sql.analysis.analyzer.PreAnalyzer.PreAnalysis; import org.elasticsearch.xpack.sql.analysis.index.GetIndexResult; import org.elasticsearch.xpack.sql.analysis.index.IndexResolver; +import org.elasticsearch.xpack.sql.analysis.index.MappingException; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.function.FunctionRegistry; import org.elasticsearch.xpack.sql.optimizer.Optimizer; @@ -41,7 +42,7 @@ public class SqlSession { private Configuration settings; public static class SessionContext { - + public final Configuration configuration; public final GetIndexResult getIndexResult; @@ -50,7 +51,7 @@ public class SqlSession { this.getIndexResult = getIndexResult; } } - + // thread-local used for sharing settings across the plan compilation // Currently this is used during: // 1. parsing - to set the TZ in date time functions (if they are used) @@ -68,7 +69,7 @@ public class SqlSession { }; public SqlSession(SqlSession other) { - this(other.settings, other.client, other.functionRegistry, other.parser, other.indexResolver, + this(other.settings, other.client, other.functionRegistry, other.parser, other.indexResolver, other.preAnalyzer, other.analyzer, other.optimizer,other.planner); } @@ -173,9 +174,10 @@ public class SqlSession { private void preAnalyze(LogicalPlan parsed, Function action, ActionListener listener) { PreAnalysis preAnalysis = preAnalyzer.preAnalyze(parsed); - //TODO why do we have a list if we only support one single element? Seems like it's the wrong data structure? + // TODO we plan to support joins in the future when possible, but for now we'll just fail early if we see one if (preAnalysis.indices.size() > 1) { - listener.onFailure(new SqlIllegalArgumentException("Queries with multiple indices are not supported")); + // Note: JOINs are not supported but we detect them when + listener.onFailure(new MappingException("Queries with multiple indices are not supported")); } else if (preAnalysis.indices.size() == 1) { indexResolver.asIndex(preAnalysis.indices.get(0), wrap(indexResult -> listener.onResponse(action.apply(indexResult)), listener::onFailure)); @@ -212,4 +214,4 @@ public class SqlSession { public Configuration settings() { return settings; } -} \ No newline at end of file +}