From 88b8794801468b575a7bfef6d6f396e2ebfcc6ab Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 5 Dec 2017 18:41:19 +0200 Subject: [PATCH] SQL: Forbid multi field groups (elastic/x-pack-elasticsearch#3199) * SQL: GROUP BY with multiple fields are forbidden The check is performed in the folder Verifier as the optimizer can eliminate some fields (like those with constants) Original commit: elastic/x-pack-elasticsearch@8d49f4ab02efefc0db54428a53e464b5a40ba2ee --- .../xpack/qa/sql/jdbc/DebugCsvSpec.java | 3 +- .../xpack/qa/sql/jdbc/DebugSqlSpec.java | 2 +- qa/sql/src/main/resources/debug.csv-spec | 2 +- qa/sql/src/main/resources/debug.sql-spec | 2 +- .../xpack/sql/analysis/analyzer/Verifier.java | 29 ++++--- .../xpack/sql/expression/Expressions.java | 12 +-- .../xpack/sql/planner/PlanningException.java | 16 ++-- .../xpack/sql/planner/Verifier.java | 26 +++++-- .../planner/VerifierErrorMessagesTests.java | 78 +++++++++++++++++++ 9 files changed, 132 insertions(+), 38 deletions(-) create mode 100644 sql/server/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugCsvSpec.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugCsvSpec.java index 87ed700636a..22924e3e648 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugCsvSpec.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugCsvSpec.java @@ -15,7 +15,7 @@ import java.sql.SQLException; import java.util.List; @TestLogging(JdbcTestUtils.SQL_TRACE) -public abstract class DebugCsvSpec extends CsvSpecTestCase { +public class DebugCsvSpec extends CsvSpecTestCase { @ParametersFactory(shuffle = false, argumentFormatting = SqlSpecTestCase.PARAM_FORMATTING) public static List readScriptSpec() throws Exception { @@ -27,6 +27,7 @@ public abstract class DebugCsvSpec extends CsvSpecTestCase { super(fileName, groupName, testName, lineNumber, testCase); } + @Override protected void assertResults(ResultSet expected, ResultSet elastic) throws SQLException { Logger log = logEsResultSet() ? logger : null; diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugSqlSpec.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugSqlSpec.java index 8726d8ddb9c..1fd4a98bba8 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugSqlSpec.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DebugSqlSpec.java @@ -12,7 +12,7 @@ import org.elasticsearch.test.junit.annotations.TestLogging; import java.util.List; @TestLogging(JdbcTestUtils.SQL_TRACE) -public abstract class DebugSqlSpec extends SqlSpecTestCase { +public class DebugSqlSpec extends SqlSpecTestCase { @ParametersFactory(shuffle = false, argumentFormatting = PARAM_FORMATTING) public static List readScriptSpec() throws Exception { Parser parser = specParser(); diff --git a/qa/sql/src/main/resources/debug.csv-spec b/qa/sql/src/main/resources/debug.csv-spec index 5cb326c17a8..30d0048aa6b 100644 --- a/qa/sql/src/main/resources/debug.csv-spec +++ b/qa/sql/src/main/resources/debug.csv-spec @@ -3,7 +3,7 @@ // debug -SELECT DAY_OF_YEAR(birth_date) d, last_name l FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no; +SELECT int FROM test GROUP BY AVG(int) + 2; table:s test_emp diff --git a/qa/sql/src/main/resources/debug.sql-spec b/qa/sql/src/main/resources/debug.sql-spec index 7ad29bef1c7..ff2e2c5adf7 100644 --- a/qa/sql/src/main/resources/debug.sql-spec +++ b/qa/sql/src/main/resources/debug.sql-spec @@ -3,4 +3,4 @@ // debug -SELECT MONTH(birth_date) AS d, COUNT(*) AS c, CAST(SUM(emp_no) AS INT) s FROM "test_emp" GROUP BY MONTH(birth_date) ORDER BY MONTH(birth_date) DESC; +SELECT int FROM test GROUP BY AVG(int) + 2; diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index d4cd50b70e0..992975288fc 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -150,9 +150,8 @@ abstract class Verifier { // Concrete verifications - // + // // if there are no (major) unresolved failures, do more in-depth analysis - // if (failures.isEmpty()) { Map resolvedFunctions = new LinkedHashMap<>(); @@ -183,14 +182,14 @@ abstract class Verifier { if (!groupingFailures.contains(p)) { checkGroupBy(p, localFailures, resolvedFunctions, groupingFailures); } - // everything checks out - // mark the plan as analyzed - if (localFailures.isEmpty()) { - p.setAnalyzed(); - } + // everything checks out + // mark the plan as analyzed + if (localFailures.isEmpty()) { + p.setAnalyzed(); + } - failures.addAll(localFailures); - }); + failures.addAll(localFailures); + }); } return failures; @@ -252,7 +251,7 @@ abstract class Verifier { Expressions.names(a.groupings()))); groupingFailures.add(a); return false; - } + } } } return true; @@ -286,7 +285,7 @@ abstract class Verifier { a.aggregates().forEach(ne -> ne.collectFirstChildren(c -> checkGroupMatch(c, ne, a.groupings(), missing, functions))); - if (!missing.isEmpty()) { + if (!missing.isEmpty()) { String plural = missing.size() > 1 ? "s" : StringUtils.EMPTY; localFailures.add(fail(missing.values().iterator().next(), "Cannot use non-grouped column" + plural + " %s, expected %s", Expressions.names(missing.keySet()), @@ -307,9 +306,9 @@ abstract class Verifier { // TODO: this should be handled by a different rule if (function == null) { return false; - } + } e = function; - } + } // scalar functions can be a binary tree // first test the function against the grouping @@ -319,11 +318,11 @@ abstract class Verifier { // found group for the expression if (Expressions.anyMatch(groupings, e::semanticEquals)) { return true; - } + } // unwrap function to find the base for (Expression arg : sf.arguments()) { arg.collectFirstChildren(c -> checkGroupMatch(c, source, groupings, missing, functions)); - } + } return true; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java index d6f4de5bdd0..a948c1952bc 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java @@ -17,7 +17,7 @@ import static java.util.stream.Collectors.toList; public abstract class Expressions { - public static List asNamed(List exp) { + public static List asNamed(List exp) { return exp.stream() .map(NamedExpression.class::cast) .collect(toList()); @@ -72,25 +72,25 @@ public abstract class Expressions { return e instanceof NamedExpression ? ((NamedExpression) e).name() : e.nodeName(); } - public static List names(Collection e) { + public static List names(Collection e) { List names = new ArrayList<>(e.size()); for (Expression ex : e) { names.add(name(ex)); } - + return names; } public static Attribute attribute(Expression e) { return e instanceof NamedExpression ? ((NamedExpression) e).toAttribute() : null; } - + public static TypeResolution typeMustBe(Expression e, Predicate predicate, String message) { return predicate.test(e) ? TypeResolution.TYPE_RESOLVED : new TypeResolution(message); } - + public static TypeResolution typeMustBeNumeric(Expression e) { - return e.dataType().isNumeric()? TypeResolution.TYPE_RESOLVED : new TypeResolution( + return e.dataType().isNumeric()? TypeResolution.TYPE_RESOLVED : new TypeResolution( "Argument required to be numeric ('%s' of type '%s')", Expressions.name(e), e.dataType().esName()); } } \ No newline at end of file diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java index 735027bf189..0537550944b 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java @@ -7,9 +7,13 @@ package org.elasticsearch.xpack.sql.planner; import org.elasticsearch.xpack.sql.ClientSqlException; import org.elasticsearch.xpack.sql.planner.Verifier.Failure; +import org.elasticsearch.xpack.sql.util.StringUtils; import java.util.Collection; -import java.util.StringJoiner; +import java.util.Locale; +import java.util.stream.Collectors; + +import static java.lang.String.format; public class PlanningException extends ClientSqlException { @@ -21,11 +25,9 @@ public class PlanningException extends ClientSqlException { super(extractMessage(sources)); } - private static String extractMessage(Collection sources) { - StringJoiner sj = new StringJoiner(",", "{", "}"); - sources.forEach(s -> { - sj.add(s.source().nodeString() + s.source().location()); - }); - return "Fail to plan items " + sj.toString(); + private static String extractMessage(Collection failures) { + return failures.stream() + .map(f -> format(Locale.ROOT, "line %s:%s: %s", f.source().location().getLineNumber(), f.source().location().getColumnNumber(), f.message())) + .collect(Collectors.joining(StringUtils.NEW_LINE, "Found " + failures.size() + " problem(s)\n", StringUtils.EMPTY)); } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java index e70a391709a..c9a1b9fa0b1 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java @@ -5,24 +5,26 @@ */ package org.elasticsearch.xpack.sql.planner; -import java.util.ArrayList; -import java.util.List; -import java.util.Objects; - +import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.plan.physical.AggregateExec; import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.sql.plan.physical.Unexecutable; import org.elasticsearch.xpack.sql.plan.physical.UnplannedExec; import org.elasticsearch.xpack.sql.tree.Node; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; + abstract class Verifier { static class Failure { private final Node source; private final String message; - + Failure(Node source, String message) { this.source = source; - this.message = message + " " + source.nodeString(); + this.message = message; } Node source() { @@ -69,11 +71,23 @@ abstract class Verifier { failures.add(fail(e, "Unresolved expression")); } }); + + if (p instanceof AggregateExec) { + forbidMultiFieldGroupBy((AggregateExec) p, failures); + } }); return failures; } + private static void forbidMultiFieldGroupBy(AggregateExec a, List failures) { + if (a.groupings().size() > 1) { + failures.add(fail(a.groupings().get(0), "Currently, only a single expression can be used with GROUP BY; please select one of " + + Expressions.names(a.groupings()))); + } + } + + static List verifyExecutingPlan(PhysicalPlan plan) { List failures = new ArrayList<>(); diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java new file mode 100644 index 00000000000..929e0277d28 --- /dev/null +++ b/sql/server/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java @@ -0,0 +1,78 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.sql.planner; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; +import org.elasticsearch.xpack.sql.analysis.catalog.Catalog; +import org.elasticsearch.xpack.sql.analysis.catalog.EsIndex; +import org.elasticsearch.xpack.sql.analysis.catalog.InMemoryCatalog; +import org.elasticsearch.xpack.sql.expression.function.DefaultFunctionRegistry; +import org.elasticsearch.xpack.sql.expression.function.FunctionRegistry; +import org.elasticsearch.xpack.sql.optimizer.Optimizer; +import org.elasticsearch.xpack.sql.parser.SqlParser; +import org.elasticsearch.xpack.sql.session.TestingSqlSession; +import org.elasticsearch.xpack.sql.type.DataType; +import org.elasticsearch.xpack.sql.type.DataTypes; +import org.junit.After; +import org.junit.Before; + +import java.util.LinkedHashMap; +import java.util.Map; + +import static java.util.Collections.singletonList; + +public class VerifierErrorMessagesTests extends ESTestCase { + + private SqlParser parser; + private FunctionRegistry functionRegistry; + private Catalog catalog; + private Analyzer analyzer; + private Optimizer optimizer; + private Planner planner; + + public VerifierErrorMessagesTests() { + parser = new SqlParser(); + functionRegistry = new DefaultFunctionRegistry(); + + Map mapping = new LinkedHashMap<>(); + mapping.put("bool", DataTypes.BOOLEAN); + mapping.put("int", DataTypes.INTEGER); + mapping.put("text", DataTypes.TEXT); + mapping.put("keyword", DataTypes.KEYWORD); + EsIndex test = new EsIndex("test", mapping); + catalog = new InMemoryCatalog(singletonList(test)); + analyzer = new Analyzer(functionRegistry); + optimizer = new Optimizer(); + planner = new Planner(); + + } + + @Before + public void setupContext() { + TestingSqlSession.setCurrentContext(TestingSqlSession.ctx(catalog)); + } + + @After + public void disposeContext() { + TestingSqlSession.removeCurrentContext(); + } + + private String verify(String sql) { + PlanningException e = expectThrows(PlanningException.class, + () -> planner.mapPlan(optimizer.optimize(analyzer.analyze(parser.createStatement(sql), true)), true)); + assertTrue(e.getMessage().startsWith("Found ")); + String header = "Found 1 problem(s)\nline "; + return e.getMessage().substring(header.length()); + } + + + public void testMultiGroupBy() { + // TODO: location needs to be updated after merging extend-having + assertEquals("1:32: Currently, only a single expression can be used with GROUP BY; please select one of [bool, keyword]", + verify("SELECT bool FROM test GROUP BY bool, keyword")); + } +} \ No newline at end of file