sql: correct lateral scoping in the face of outer joins by benesch · Pull Request #9623 · MaterializeInc/materialize

benesch · 2021-12-16T06:06:53Z

Rework join planning so that we can handle lateral references that are
mixed with right and full outer joins. The implementation here largely
follows what @asenac laid out in @6875, except that the existing outer
scope design was found to be sufficient; that is, there is no concept of
a "sibling" scope introduced here.

The gist is that we now always plan joins with the RHS as a child scope
of the LHS. (Previously, we only created the new scope for joins if we
could eagerly notice that the RHS had a lateral element.) If the join is
a right or full join, we ban referencing any of the elements in the left
scope, as required by SQL:2008. When we encounter a non-lateral
subquery, we hide any lateral scope items entirely. Yes, it's weird that
non-lateral subqueries pretend lateral scope items don't exit, while
subqueries on the RHS of a right or full join error if they access a
lateral scope item, but that's what SQL demands.

WARNING: The lowering code is amended to always treat inner and left joins as
correlated, even if they are not. This is correct, but it may produce
worse plans. I haven't investigated the movement of plans in much detail
yet.

Fix MaterializeInc/database-issues#2143.

Motivation

This PR fixes a recognized bug.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR adds a release note for any user-facing behavior changes.

asenac · 2021-12-16T08:42:59Z

This is brilliant. It's more or less what I tried to do back in the days, but much better and better organized :)

For the changes in the plans, as mentioned in slack, we could bring the old behavior back if re-introduced the old lateral flag as correlated, indicating whether the RHS of the join references any column from the LHS use that flag during lowering. This flag could be computed within plan_join. Alternatively, we could compute that on demand during lowering. The following patch is a quick and dirty implementation of that which leaves the plans as they were:

diff --git a/src/sql/src/plan/expr.rs b/src/sql/src/plan/expr.rs
index fecb56da7..aa87062db 100644
--- a/src/sql/src/plan/expr.rs
+++ b/src/sql/src/plan/expr.rs
@@ -805,6 +805,28 @@ impl HirRelationExpr {
         }
     }
 
+    pub fn is_correlated_join(&self) -> bool {
+        match self {
+            HirRelationExpr::Join {
+                left: _,
+                right,
+                on: _,
+                kind,
+            } if kind.is_lateral() => {
+                let mut correlated = false;
+                right.clone().visit_columns(0, &mut |depth, col| {
+                    if col.level > depth {
+                        if col.level - depth == 1 {
+                            correlated = true;
+                        }
+                    }
+                });
+                correlated
+            }
+            _ => false,
+        }
+    }
+
     pub fn project(self, outputs: Vec<usize>) -> Self {
         if outputs.iter().copied().eq(0..self.arity()) {
             // The projection is trivial. Suppress it.
diff --git a/src/sql/src/plan/lowering.rs b/src/sql/src/plan/lowering.rs
index 6df8c2000..875275387 100644
--- a/src/sql/src/plan/lowering.rs
+++ b/src/sql/src/plan/lowering.rs
@@ -296,7 +296,7 @@ impl HirRelationExpr {
                     right,
                     on,
                     kind,
-                } if kind.is_lateral() => {
+                } if self.is_correlated_join() => {
                     // A LATERAL join is a join in which the right expression has
                     // access to the columns in the left expression. It turns out
                     // this is *exactly* our branch operator, plus some additional

The code above would be acceptable if just added a non-mutable version of visit_columns method.

asenac · 2021-12-16T08:48:24Z

Note that the QGM tests need to be re-generated to catch up with the changes in the input HIR. That be done by running the following command within src/sql directory:

REWRITE=1 cargo test  -- test_hir_generator

Although I think we should tweak the HIR->QGM generation code so that we remove those noisy joins with the join identity at generation time.

sploiselle · 2021-12-16T14:41:30Z

Glad to cede review to @asenac; I'll need a day or two to parse through this.

asenac

This is so good that it makes a task I spent quite a few days and failed at look very simple, even though it is not.

I'd fix the lowering regression before merging this though. At this point my preference would be to add an extra commit with a version of the patch mentioned in one of my previous comments.

asenac · 2021-12-16T15:49:46Z

+    //
+    // It's easiest to understand with an example. Consider this query:
+    //
+    //     SELECT (SELECT * FROM tab1, tab2, (SELECT tab1.a, tab3.a) FROM tab3, tab4


I think the query above is missing a second closing parenthesis after tab3.a, ie:

SELECT (SELECT * FROM tab1, tab2, (SELECT tab1.a, tab3.a)) FROM tab3, tab4

Good catch! Fixed.

asenac · 2021-12-16T15:52:15Z

+    //     Scope 4:                                                             ----
+    //     Scope 5:                                                       ----
+    //
+    // Note that the because the subquery is not marked `LATERAL`, its reference


I tend to prefer limiting the term subquery for queries in expression positions, and call nested queries in join positions as derived tables. I know there is no consensus in the literature around that though :)

Sure thing. Fixed.

benesch

I'd fix the lowering regression before merging this though. At this point my preference would be to add an extra commit with a version of the patch mentioned in one of my previous comments.

Done! I taught the planner to elide any no-op joins, and taught lowering to use the correlated join path only if the RHS is actually correlated. There are no longer any plan regressions, and in fact a few plan improvements because we're now a bit smarter about detecting no-op joins.

benesch · 2021-12-16T19:02:44Z

+    //     Scope 4:                                                             ----
+    //     Scope 5:                                                       ----
+    //
+    // Note that the because the subquery is not marked `LATERAL`, its reference


Sure thing. Fixed.

benesch · 2021-12-16T19:02:54Z

+    //
+    // It's easiest to understand with an example. Consider this query:
+    //
+    //     SELECT (SELECT * FROM tab1, tab2, (SELECT tab1.a, tab3.a) FROM tab3, tab4


Good catch! Fixed.

asenac

Sweet!

asenac · 2021-12-16T19:23:36Z

                    kind,
-                } if kind.is_lateral() => {
-                    // A LATERAL join is a join in which the right expression has
+                } if right.is_correlated() => {


The commit message is no longer correct, since now lowering is smarter than it was.

Thanks, fixed that and added a release note.

This becomes necessary in a future commit where we want to determine whether an expression contains outer column references but we do not have mutable access to that expression.

This gives us a place to elide no-op joins in a future commit.

Add some additional ScopeItem constructors, and then force use of those constructors by adding an empty private field to ScopeItem. This ensures there is exactly one place with the correct default values for the growing list of ScopeItem properties, like nameable and visible_to_wildcard.

Collapse the use of priorities and the `visible_to_wildcard` bool for handling outer join scoping rules into one new bool called `allow_unqualified_references`. This plays better with the LATERAL scoping fixes coming down the pipe; I couldn't get the priority setting/resetting to work out with the new LATERAL code.

@asenac

Rework join planning so that we can handle lateral references that are mixed with right and full outer joins. The implementation here largely follows what @asenac laid out in @6875, except that the existing outer scope design was found to be sufficient; that is, there is no concept of a "sibling" scope introduced here. The gist is that we now always plan joins with the RHS as a child scope of the LHS. (Previously, we only created the new scope for joins if we could eagerly notice that the RHS had a lateral element.) If the join is a right or full join, we ban referencing any of the elements in the left scope, as required by SQL:2008. When we encounter a non-lateral subquery, we hide any lateral scope items entirely. Yes, it's weird that non-lateral subqueries pretend lateral scope items don't exit, while subqueries on the RHS of a right or full join *error* if they access a lateral scope item, but that's what SQL demands. Fix #6875.

The code that resolved column references in USING clauses was incorrectly claiming that it was an internal error if the column resolved to an item in an outer scope, when in fact this is a user error. Correct this error message, and also generally ensure that our invalid USING reference error messages match PostgreSQL.

benesch · 2021-12-17T04:05:34Z

@asenac, thank you so much for all the help, and the super fast reviews! Really glad to be finally putting this issue to bed.

Glad to cede review to @asenac; I'll need a day or two to parse through this.

Ack, wanted to at least give ya a heads up that I had to tweak the USING priority stuff since I couldn't get the priorities to stick in the new (correct) join implementation. The bool that used to be called visible_to_wildcard does all the work now, under a new name allow_unqualified_reference.

After MaterializeInc#9623 a new scope is always defined for the RHS of any join, LATERAL or not. If a relation is pushed down to the RHS of a join after name resolution has already happened we must update the column references leaving the scope of that relation to catch up with the scope of the join. That is what happens when creating the FULL OUTER used for supporting ROWS FROM.

…results (#35414) Introduced in #9623. Leads to wrong results, see the attached SLT, which fails like this without the product code change: ``` SELECT * FROM (SELECT) AS sub LEFT JOIN empty_t ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:35 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- SELECT * FROM empty_t RIGHT JOIN (SELECT) AS sub ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:50 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- SELECT * FROM (SELECT) AS sub FULL JOIN empty_t ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:58 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- SELECT * FROM empty_t FULL JOIN (SELECT) AS sub ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:66 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- FAIL: output-failure=4 success=5 total=9 ```

…results (MaterializeInc#35414) Introduced in MaterializeInc#9623. Leads to wrong results, see the attached SLT, which fails like this without the product code change: ``` SELECT * FROM (SELECT) AS sub LEFT JOIN empty_t ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:35 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- SELECT * FROM empty_t RIGHT JOIN (SELECT) AS sub ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:50 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- SELECT * FROM (SELECT) AS sub FULL JOIN empty_t ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:58 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- SELECT * FROM empty_t FULL JOIN (SELECT) AS sub ON true OutputFailure:test/sqllogictest/join-identity-elision.slt:66 expected: Values(["NULL", "NULL"]) actually: Values([]) actual raw: [] ---- FAIL: output-failure=4 success=5 total=9 ```

This comes from #9623

benesch requested review from asenac and sploiselle December 16, 2021 06:06

benesch force-pushed the lateral-scoping branch 2 times, most recently from 24bd6af to 2d468e7 Compare December 16, 2021 07:01

asenac reviewed Dec 16, 2021

View reviewed changes

benesch force-pushed the lateral-scoping branch 2 times, most recently from fb837ad to a21fe69 Compare December 16, 2021 19:04

benesch commented Dec 16, 2021

View reviewed changes

asenac approved these changes Dec 16, 2021

View reviewed changes

benesch added 4 commits December 16, 2021 15:00

sql: introduce non-mutable version of visit_columns

e6c1504

This becomes necessary in a future commit where we want to determine whether an expression contains outer column references but we do not have mutable access to that expression.

sql: introduce HirRelationExpr::Join constructor method

5b25326

This gives us a place to elide no-op joins in a future commit.

benesch force-pushed the lateral-scoping branch from a21fe69 to adea2cc Compare December 16, 2021 20:03

benesch added 2 commits December 16, 2021 17:47

benesch force-pushed the lateral-scoping branch from adea2cc to 56e75e7 Compare December 16, 2021 22:48

benesch merged commit a79d41d into MaterializeInc:main Dec 17, 2021

benesch deleted the lateral-scoping branch December 17, 2021 04:06

asenac mentioned this pull request Dec 17, 2021

Fix lowering panic with LATERAL references in ROWS FROM #9654

Merged

2 tasks

def- mentioned this pull request Mar 11, 2026

sql: Fix: Don't do join identity elision when it would produce wrong results #35414

Merged

def- mentioned this pull request Apr 14, 2026

sql: Simplify quantified comparisons in JOIN ON clauses #36065

Merged

def- added a commit that referenced this pull request Apr 15, 2026

sql: Simplify quantified comparisons in JOIN ON clauses (#36065)

6168aa9

This comes from #9623

Conversation

benesch commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Checklist

Uh oh!

asenac commented Dec 16, 2021

Uh oh!

asenac commented Dec 16, 2021

Uh oh!

sploiselle commented Dec 16, 2021

Uh oh!

asenac left a comment

Choose a reason for hiding this comment

Uh oh!

asenac Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

benesch Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

asenac Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

benesch Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

benesch left a comment

Choose a reason for hiding this comment

Uh oh!

benesch Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

benesch Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

asenac left a comment

Choose a reason for hiding this comment

Uh oh!

asenac Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

benesch Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesch commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benesch commented Dec 16, 2021 •

edited

Loading

benesch Dec 16, 2021 •

edited

Loading