From b4bb8b77824e06f78a74e2645939efeeb5857271 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Fri, 4 May 2018 10:51:06 +0300 Subject: [PATCH 1/3] SQL: Verify GROUP BY ordering on grouped columns Due to the way composite aggregation works, ordering in GROUP BY can be applied only through grouped columns which now the analyzer verifier enforces. Fix 29900 --- .../xpack/sql/analysis/analyzer/Verifier.java | 25 ++++++++++++++++--- .../analyzer/VerifierErrorMessagesTests.java | 14 +++++++++-- .../planner/VerifierErrorMessagesTests.java | 14 +++++++++++ 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index f5147b84468b7..b3c316b63c4f5 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -211,12 +211,13 @@ static Collection verify(LogicalPlan plan) { /** * Check validity of Aggregate/GroupBy. - * This rule is needed for two reasons: + * This rule is needed for multiple reasons: * 1. a user might specify an invalid aggregate (SELECT foo GROUP BY bar) * 2. the order/having might contain a non-grouped attribute. This is typically * caught by the Analyzer however if wrapped in a function (ABS()) it gets resolved * (because the expression gets resolved little by little without being pushed down, * without the Analyzer modifying anything. + * 3. composite agg (used for GROUP BY) allows ordering only on the group keys */ private static boolean checkGroupBy(LogicalPlan p, Set localFailures, Map resolvedFunctions, Set groupingFailures) { @@ -225,7 +226,7 @@ && checkGroupByOrder(p, localFailures, groupingFailures, resolvedFunctions) && checkGroupByHaving(p, localFailures, groupingFailures, resolvedFunctions); } - // check whether an orderBy failed + // check whether an orderBy failed or if it occurs on a non-key private static boolean checkGroupByOrder(LogicalPlan p, Set localFailures, Set groupingFailures, Map functions) { if (p instanceof OrderBy) { @@ -234,7 +235,25 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur Aggregate a = (Aggregate) o.child(); Map> missing = new LinkedHashMap<>(); - o.order().forEach(oe -> oe.collectFirstChildren(c -> checkGroupMatch(c, oe, a.groupings(), missing, functions))); + o.order().forEach(oe -> { + Expression e = oe.child(); + // skip score + if (e instanceof Score) { + return; + } + // cannot order by aggregates (not supported by composite) + if (Functions.isAggregate(e)) { + missing.put(e, oe); + return; + } + + if (Expressions.anyMatch(a.groupings(), e::semanticEquals)) { + return; + } + + // nothing matched, cannot group by it + missing.put(e, oe); + }); if (!missing.isEmpty()) { String plural = missing.size() > 1 ? "s" : StringUtils.EMPTY; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 355c4d2f7b763..3cef8455038a8 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -111,7 +111,7 @@ public void testGroupByOrderByNonGrouped() { } public void testGroupByOrderByScalarOverNonGrouped() { - assertEquals("1:50: Cannot order by non-grouped column [date], expected [text]", + assertEquals("1:50: Cannot order by non-grouped column [YEAR(date [UTC])], expected [text]", verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY YEAR(date)")); } @@ -144,4 +144,14 @@ public void testUnsupportedType() { assertEquals("1:8: Cannot use field [unsupported] type [ip_range] as is unsupported", verify("SELECT unsupported FROM test")); } -} + + public void testGroupByOrderByNonKey() { + assertEquals("1:52: Cannot order by non-grouped column [a], expected [bool]", + verify("SELECT AVG(int) a FROM test GROUP BY bool ORDER BY a")); + } + + public void testGroupByOrderByFunctionOverKey() { + assertEquals("1:44: Cannot order by non-grouped column [MAX(int)], expected [int]", + verify("SELECT int FROM test GROUP BY int ORDER BY MAX(int)")); + } +} \ No newline at end of file diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java index 154885261fdb8..5d6f479b7558d 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorMessagesTests.java @@ -49,4 +49,18 @@ public void testMultiGroupBy() { 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")); } + + // + // TODO potential improvements + // + // regarding resolution + // public void testGroupByOrderByKeyAlias() { + // assertEquals("1:8: Cannot use field [unsupported] type [ip_range] as is unsupported", + // verify("SELECT int i FROM test GROUP BY int ORDER BY i")); + // } + // + // public void testGroupByAlias() { + // assertEquals("1:8: Cannot use field [unsupported] type [ip_range] as is unsupported", + // verify("SELECT int i FROM test GROUP BY i ORDER BY int")); + // } } From efb8a964becc5866bd71879be585dcff271f6dc8 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 15 May 2018 00:09:30 +0300 Subject: [PATCH 2/3] Eliminate SCORE() from supported ORDER BYs --- .../elasticsearch/xpack/sql/analysis/analyzer/Verifier.java | 4 ---- .../sql/analysis/analyzer/VerifierErrorMessagesTests.java | 5 +++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index b3c316b63c4f5..8d0b8a95b3ba3 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -237,10 +237,6 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur Map> missing = new LinkedHashMap<>(); o.order().forEach(oe -> { Expression e = oe.child(); - // skip score - if (e instanceof Score) { - return; - } // cannot order by aggregates (not supported by composite) if (Functions.isAggregate(e)) { missing.put(e, oe); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 3cef8455038a8..60875e0194a0c 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -154,4 +154,9 @@ public void testGroupByOrderByFunctionOverKey() { assertEquals("1:44: Cannot order by non-grouped column [MAX(int)], expected [int]", verify("SELECT int FROM test GROUP BY int ORDER BY MAX(int)")); } + + public void testGroupByOrderByScore() { + assertEquals("1:44: Cannot order by non-grouped column [SCORE()], expected [int]", + verify("SELECT int FROM test GROUP BY int ORDER BY SCORE()")); + } } \ No newline at end of file From 19bc9f9cda7c3dc1f3278638ee56c5cd87ab6430 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 15 May 2018 11:47:03 +0300 Subject: [PATCH 3/3] Fix comparison between expressions and attributes --- .../elasticsearch/xpack/sql/analysis/analyzer/Verifier.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index 8d0b8a95b3ba3..6f8be61b463fd 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -243,7 +243,9 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur return; } - if (Expressions.anyMatch(a.groupings(), e::semanticEquals)) { + // make sure to compare attributes directly + if (Expressions.anyMatch(a.groupings(), + g -> e.semanticEquals(e instanceof Attribute ? Expressions.attribute(g) : g))) { return; }