SQL: SubSelect unresolved bugfix #55956

astefan · 2020-04-29T18:03:34Z

This fixes an issue where a sub-query was attempted to be resolved at the wrong time in the analysis, generating an error message similar to org.elasticsearch.xpack.sql.capabilities.UnresolvedException: Invalid call to attribute on an unresolved object ?* AS ?. Bug introduced with #41964.

…o subselect_unresolved_bugfix

elasticmachine · 2020-04-29T18:03:36Z

Pinging @elastic/es-ql (:Query Languages/SQL)

bpintea · 2020-04-29T18:19:04Z

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

@@ -722,7 +722,7 @@ protected LogicalPlan rule(LogicalPlan plan) {
            }

            // Try to resolve aggregates and groupings based on the child plan
-            if (plan instanceof Aggregate) {
+            if (plan instanceof Aggregate && !plan.resolved() && plan.childrenResolved()) {


Suggested change

if (plan instanceof Aggregate && !plan.resolved() && plan.childrenResolved()) {

if (plan instanceof Aggregate && plan.resolved() == false && plan.childrenResolved()) {

despite my own prefs.

Considering this condition applies for all ifs inside this rule, I would extract it to the top:

@Override protected LogicalPlan rule(LogicalPlan plan) { if (plan.resolved() || plan.childrenResolved() == false) { return plan; } if (plan instanceof Filter) {...

In fact the check for children resolution is present I believe in all analyzer rules so it might make to create a subclass that all other rules extend to avoid having to repeat this bit of code which as we can see, introduces some subtle bugs.

I'm fine with doing it in a follow-up PR to get this in (with the if moved to the top level).

bpintea

Great set of tests. I'd be great to see some more complex subqueries, but I guess I'm just trying to better understand our own boundaries.
Anyways, LGTM.

costin

LGTM - thanks for the quick turn-around!

costin · 2020-04-29T20:33:34Z

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

@@ -722,7 +722,7 @@ protected LogicalPlan rule(LogicalPlan plan) {
            }

            // Try to resolve aggregates and groupings based on the child plan
-            if (plan instanceof Aggregate) {
+            if (plan instanceof Aggregate && !plan.resolved() && plan.childrenResolved()) {


Considering this condition applies for all ifs inside this rule, I would extract it to the top:

@Override protected LogicalPlan rule(LogicalPlan plan) { if (plan.resolved() || plan.childrenResolved() == false) { return plan; } if (plan instanceof Filter) {...

In fact the check for children resolution is present I believe in all analyzer rules so it might make to create a subclass that all other rules extend to avoid having to repeat this bit of code which as we can see, introduces some subtle bugs.

I'm fine with doing it in a follow-up PR to get this in (with the if moved to the top level).

…o subselect_unresolved_bugfix

costin

LGTM

costin · 2020-04-30T09:29:07Z

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

@@ -610,9 +600,12 @@ private AggGroupingFailure(List<String> expectedGrouping) {
        }

        @Override
-        protected LogicalPlan rule(LogicalPlan plan) {
+        protected LogicalPlan rulePlan(LogicalPlan plan) {
+            if (plan.resolved()) {


I don't think this is needed anymore - see AnalyzeRule which takes care of skipping if the node is resolved and skipResolved is true (default).

costin · 2020-04-30T09:30:06Z

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

@@ -1312,4 +1303,17 @@ protected boolean skipResolved() {
            return true;
        }
    }
+
+    abstract static class LogicalPlanAnalyzeRule extends AnalyzeRule<LogicalPlan> {


I'd pick a different name - maybe DefaultAnalyzeRule or BaseAnalyzeRule.

I was struggling with the name, indeed. I didn't particularly like LogicalPlanAnalyzeRule, but didn't find any better alternatives.

I like BaseAnalyzeRule.

costin · 2020-04-30T09:31:39Z

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

+            return rulePlan(plan);
+        }
+
+        protected abstract LogicalPlan rulePlan(LogicalPlan plan);


rulePlan is repetitive (the method only accepts LogicalPlans) - how about doRule or analyze?

matriv

LGTM. Nice set of tests!

matriv · 2020-04-30T11:43:32Z

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

@@ -1312,4 +1303,17 @@ protected boolean skipResolved() {
            return true;
        }
    }
+
+    abstract static class LogicalPlanAnalyzeRule extends AnalyzeRule<LogicalPlan> {


I like BaseAnalyzeRule.

matriv · 2020-04-30T11:46:10Z

x-pack/plugin/sql/qa/src/main/resources/subselect.sql-spec

@@ -0,0 +1,53 @@
+


Please add the following comment in case someone needs to mute a test in the future:
// To mute tests follow example in file: example.sql-spec

…o subselect_unresolved_bugfix

* Resolve the missing refs only after the aggregate tree is resolved (cherry picked from commit 10167b1)

* Resolve the missing refs only after the aggregate tree is resolved

astefan added 2 commits April 29, 2020 20:58

Resolve the missing refs only after the aggregate tree is resolved

c869ff0

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

aaeeb13

…o subselect_unresolved_bugfix

astefan added >bug :Analytics/SQL SQL querying v8.0.0 v7.6.3 v7.8.0 v7.7.1 labels Apr 29, 2020

astefan requested review from costin, bpintea and matriv April 29, 2020 18:03

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Apr 29, 2020

bpintea reviewed Apr 29, 2020

View reviewed changes

bpintea approved these changes Apr 29, 2020

View reviewed changes

costin approved these changes Apr 29, 2020

View reviewed changes

astefan added 2 commits April 30, 2020 03:45

Polishing

180b198

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

cab35be

…o subselect_unresolved_bugfix

astefan requested a review from costin April 30, 2020 00:47

costin approved these changes Apr 30, 2020

View reviewed changes

matriv approved these changes Apr 30, 2020

View reviewed changes

matriv reviewed Apr 30, 2020

View reviewed changes

astefan added 2 commits April 30, 2020 14:53

Address reviews comments

aac7a83

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

90ff276

…o subselect_unresolved_bugfix

astefan merged commit 10167b1 into elastic:master Apr 30, 2020

astefan deleted the subselect_unresolved_bugfix branch April 30, 2020 17:30

astefan added a commit to astefan/elasticsearch that referenced this pull request Apr 30, 2020

SQL: SubSelect unresolved bugfix (elastic#55956)

794fb84

* Resolve the missing refs only after the aggregate tree is resolved (cherry picked from commit 10167b1)

astefan added a commit to astefan/elasticsearch that referenced this pull request Apr 30, 2020

SQL: SubSelect unresolved bugfix (elastic#55956)

d813cd8

* Resolve the missing refs only after the aggregate tree is resolved (cherry picked from commit 10167b1)

astefan added a commit to astefan/elasticsearch that referenced this pull request Apr 30, 2020

SQL: SubSelect unresolved bugfix (elastic#55956)

d4922cb

* Resolve the missing refs only after the aggregate tree is resolved (cherry picked from commit 10167b1)

astefan added the backport pending label Apr 30, 2020

This was referenced Apr 30, 2020

SQL: SubSelect unresolved bugfix (#55956) #56055

Merged

SQL: SubSelect unresolved bugfix (#55956) #56056

Merged

SQL: SubSelect unresolved bugfix (#55956) #56057

Merged

astefan added a commit that referenced this pull request May 1, 2020

SQL: SubSelect unresolved bugfix (#55956) (#56055)

fbba65d

* Resolve the missing refs only after the aggregate tree is resolved (cherry picked from commit 10167b1)

astefan added a commit that referenced this pull request May 1, 2020

SQL: SubSelect unresolved bugfix (#55956) (#56056)

c057120

* Resolve the missing refs only after the aggregate tree is resolved (cherry picked from commit 10167b1)

astefan added a commit that referenced this pull request May 1, 2020

SQL: SubSelect unresolved bugfix (#55956) (#56057)

1745788

* Resolve the missing refs only after the aggregate tree is resolved (cherry picked from commit 10167b1)

astefan removed the backport pending label May 1, 2020

breskeby pushed a commit that referenced this pull request May 7, 2020

SQL: SubSelect unresolved bugfix (#55956)

a4c39b1

* Resolve the missing refs only after the aggregate tree is resolved

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: SubSelect unresolved bugfix #55956

SQL: SubSelect unresolved bugfix #55956

astefan commented Apr 29, 2020

elasticmachine commented Apr 29, 2020

bpintea Apr 29, 2020

costin Apr 29, 2020

bpintea left a comment

costin left a comment

costin Apr 29, 2020

costin left a comment

costin Apr 30, 2020

costin Apr 30, 2020

astefan Apr 30, 2020

matriv Apr 30, 2020

costin Apr 30, 2020

matriv left a comment

matriv Apr 30, 2020

matriv Apr 30, 2020

	if (plan instanceof Aggregate && !plan.resolved() && plan.childrenResolved()) {
	if (plan instanceof Aggregate && plan.resolved() == false && plan.childrenResolved()) {

SQL: SubSelect unresolved bugfix #55956

SQL: SubSelect unresolved bugfix #55956

Conversation

astefan commented Apr 29, 2020

elasticmachine commented Apr 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment