SQL: Generate relevant error message when grouping functions are not used in GROUP BY by astefan · Pull Request #38017 · elastic/elasticsearch

astefan · 2019-01-30T09:28:37Z

Grouping functions (HISTOGRAM, so far) have a requirement of being used inside the GROUP BY. This PR adds checks to make sure this happens. Otherwise, an error message is generated.

Fixes #37952.

…UP BY

elasticmachine · 2019-01-30T09:28:39Z

Pinging @elastic/es-search

costin

The check needs to be extended so it traverses the tree in case the grouping function is embedded.
For example SELECT YEAR(HISTOGRAM(date, 1 YEAR)) FROM x GROUP BY HISTOGRAM(date, 1 YEAR).
Additionally (and this might be a separate PR), it's worth investigating how we handle functions over histograms FROM x GROUP BY YEAR(HISTOGRAM(date, 1 YEAR)).

matriv

Left a coupe of questions, otherwise LGTM

matriv · 2019-01-30T12:30:30Z

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

+
+            proj.projections().forEach(e -> {
+                if (Functions.isGrouping(e) == false) {
+                    e.collectFirstChildren(c -> {


collectFirstChildren uses a List to add the matches which we don't need in this impl.
Why don't you use forEachDown?

Changed the approach and pushed a new commit.

matriv · 2019-01-30T12:31:35Z

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

+
+            a.aggregates().forEach(as -> {
+                Expression exp = as instanceof Alias ? ((Alias) as).child() : as;
+                if (Functions.isGrouping(exp) && false == Expressions.anyMatch(a.groupings(), g -> exp.semanticEquals(g))) {


what if the histogram is wrapped with a Scalar function? is it caught somewhere else?

This was already handled. See the test here.

properly in the Verifier due to functions equality not working correctly.

…o 37952_fix

astefan · 2019-01-31T21:35:05Z

@costin I pushed a new commit.

astefan · 2019-01-31T23:38:47Z

@elasticmachine run elasticsearch-ci/2

astefan · 2019-02-01T07:48:23Z

@elasticmachine run elasticsearch-ci/2

astefan · 2019-02-01T09:36:19Z

@elasticmachine run elasticsearch-ci/2

…o 37952_fix

costin

Left a few comments, I'm unclear on why extraChildren is needed...

costin · 2019-02-01T17:46:44Z

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

+            Project proj = (Project) p;
+            proj.projections().forEach(e -> e.forEachDown(f -> localFailures.add(fail(f, "[%s] needs to be part of the grouping",
+                    Expressions.name(f))), GroupingFunction.class));
+        } else if (p instanceof Aggregate) {


this block is a bit condensed and could use some whitespace and indentation to make it more readable.
For example put localFailures on a new line.

costin · 2019-02-01T17:47:49Z

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

+            // if it does have a GROUP BY, check if the groupings contain the grouping functions (Histograms) 
+            Aggregate a = (Aggregate) p;
+            a.aggregates().forEach(agg -> agg.forEachDown(e -> {
+                if (Expressions.anyMatch(a.groupings(), g -> {return g instanceof Function && e.functionEquals((Function) g);}) == false


anyMatch(a.groupings(), g -> g instanceof Function && functionEquals((Function) g)
Avoids the use of {, return and ;
Also like above it will be readable if you put each statement on a new line

costin · 2019-02-01T17:48:30Z

...src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java


-    protected GroupingFunction(Source source, Expression field) {
-        this(source, field, emptyList());
+    protected GroupingFunction(Source source, Expression field, List<Expression> extraChildren) {


extraChildren is meaningless - use arguments instead (to preserve the terminology from the parent) or parameters instead.

costin · 2019-02-01T17:50:26Z

...src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java


-    protected GroupingFunction(Source source, Expression field, List<Expression> parameters) {
-        super(source, CollectionUtils.combine(singletonList(field), parameters));
+    protected GroupingFunction(Source source, Expression field, List<Expression> extraChildren, List<Expression> parameters) {


Why is there extraChildren and parameters?

costin · 2019-02-01T17:51:06Z

...in/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java

    public Histogram(Source source, Expression field, Expression interval, ZoneId zoneId) {
-        super(source, field);
-        this.interval = (Literal) interval;
+        super(source, field, Arrays.asList(interval));


Why not have this passed through parameters instead of extraChildren?

Minor improvement, use: Collections.singletonList(interval)

astefan · 2019-02-01T20:54:23Z

@costin the idea is to make interval part of the node's children so that the functionEquals check here does actually make two Histograms be equal. Since the ZoneId isn't really a children, that's why I defined a List of "extra" children, to make it more generic for any future (if any) function that will need a variable list of children...

…o 37952_fix

astefan · 2019-02-02T08:33:54Z

@costin addressed your comments. Initially I looked at parameters, but I regarded them as having a different role than field and interval.

costin

LGTM

costin · 2019-02-02T11:04:36Z

...in/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java

    public Histogram(Source source, Expression field, Expression interval, ZoneId zoneId) {
-        super(source, field);
-        this.interval = (Literal) interval;
+        super(source, field, Arrays.asList(interval));


Minor improvement, use: Collections.singletonList(interval)

…o 37952_fix

matriv · 2019-02-02T16:34:58Z

...in/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java

        return resolution;
    }
-
+    


Really minor: remove the whitespaces.

…used in GROUP BY (#38017) * Add checks for Grouping functions restriction to be placed inside GROUP BY * Fixed bug where GROUP BY HISTOGRAM (not using alias) wasn't recognized properly in the Verifier due to functions equality not working correctly. (cherry picked from commit 6968f09)

…round-sync-6.x * elastic/6.x: Fix testRestoreIncreasesPrimaryTerms on 6.x (elastic#38314) SQL: Remove exceptions from Analyzer (elastic#38260) (elastic#38287) SQL: Move metrics tracking inside PlanExecutor (elastic#38259) (elastic#38288) Backport of elastic#38311: Move TokenService to seqno powered cas Handle scheduler exceptions (elastic#38183) Mute MlMigrationFullClusterRestartIT#testMigration (elastic#38316) 6.x Backport of elastic#38278: Move ML Optimistic Concurrency Control to Seq No Cleanup construction of interceptors (elastic#38296) Throw if two inner_hits have the same name (elastic#37645) (elastic#38194) AsyncTwoPhaseIndexerTests race condition fixed elastic#38195 Backport#37830 Enable SSL in reindex with security QA tests (elastic#38293) Ensure ILM policies run safely on leader indices (elastic#38140) Introduce ssl settings to reindex from remote (elastic#38292) Fix ordering problem in add or renew lease test (elastic#38281) Mute ReplicationTrackerRetentionLeaseTests#testAddOrRenewRetentionLease (elastic#38276) Fix NPE in Logfile Audit Filter (elastic#38120) (elastic#38271) Enable trace log in FollowerFailOverIT (elastic#38148) SQL: Generate relevant error message when grouping functions are not used in GROUP BY (elastic#38017)

* 6.6: (121 commits) [DOCS] Add warning about bypassing ML PUT APIs (elastic#38608) fix dissect doc "ip" --> "clientip" (elastic#38512) bad formatted JSON object (elastic#38515) SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440) Update ilm-api.asciidoc, point to REMOVE policy (elastic#38235) Backport changes to the release notes script. (elastic#38347) Change the milliseconds precision to 3 digits for intervals. (elastic#38297) SecuritySettingsSource license.self_generated: trial (elastic#38233) (elastic#38398) Fix IndexAuditTrail rolling upgrade on rollover edge 2 (elastic#38286) (elastic#38381) Cleanup construction of interceptors (elastic#38388) Skip unsupported languages for tests (elastic#38328) (elastic#38385) [ILM][TEST] increase assertBusy timeout (elastic#36864) (elastic#38354) Docs: Drop inline callout from scroll example (elastic#38340) (elastic#38365) Preserve ILM operation mode when creating new lifecycles (elastic#38134) (elastic#38230) [ML] Add explanation so far to file structure finder exceptions (elastic#38337) ML: Fix error race condition on stop _all datafeeds and close _all jobs (elastic#38113) (elastic#38211) (elastic#38222) SQL: Generate relevant error message when grouping functions are not used in GROUP BY (elastic#38017) Fix NPE in Logfile Audit Filter (elastic#38120) (elastic#38273) Enable trace log in FollowerFailOverIT (elastic#38148) Replace awaitBusy with assertBusy in atLeastDocsIndexed (elastic#38190) ...

Add checks for Grouping functions restriction to be placed inside GRO…

2707a44

…UP BY

astefan added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.7.0 v6.6.1 labels Jan 30, 2019

astefan requested review from costin and matriv January 30, 2019 09:28

costin reviewed Jan 30, 2019

View reviewed changes

matriv approved these changes Jan 30, 2019

View reviewed changes

astefan added 2 commits January 31, 2019 23:12

Fixed bug where GROUP BY HISTOGRAM (not using alias) wasn't recognized

51b19ef

properly in the Verifier due to functions equality not working correctly.

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

f1f7397

…o 37952_fix

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

08f75e5

…o 37952_fix

costin reviewed Feb 1, 2019

View reviewed changes

astefan added 4 commits February 2, 2019 01:20

Address comments

2658b70

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

36244c7

…o 37952_fix

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

ff39950

…o 37952_fix

Changes after merging master

08a2bf7

costin approved these changes Feb 2, 2019

View reviewed changes

astefan added 2 commits February 2, 2019 13:08

Address comment

597eeaa

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

0e1b6c4

…o 37952_fix

matriv reviewed Feb 2, 2019

View reviewed changes

astefan merged commit 6968f09 into elastic:master Feb 2, 2019

astefan deleted the 37952_fix branch February 2, 2019 20:43

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

astefan commented Jan 30, 2019

Uh oh!

elasticmachine commented Jan 30, 2019

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Jan 31, 2019

Uh oh!

astefan commented Jan 31, 2019

Uh oh!

astefan commented Feb 1, 2019

Uh oh!

astefan commented Feb 1, 2019

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Feb 1, 2019

Uh oh!

astefan commented Feb 2, 2019

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants