feat(binder): bind RCTE #15522

TennyZhuang · 2024-03-07T12:14:37Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

#15135

Close #15681

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: TennyZhuang <[email protected]>

wangrunji0408 · 2024-03-14T05:36:53Z

src/frontend/src/binder/relation/cte_ref.rs

Intuitively, if we have decoupled the declaration (knowing its schema) and definition (binding the recursive query) stage of CTE, we can use the existing BoundShare to refer to all CTEs no matter they are recursive or not. I feel that this new structure is not necessary.

No, it's a back reference. If you take a look at the definition of BoundShare, you'll know it's impossible to use that, otherwise the loop will not end.

DuckDB also have two kind of CTE nodes.

wangrunji0408 · 2024-03-14T05:45:56Z

src/frontend/src/binder/set_expr.rs

+    /// UNION in recursive CTE definition
+    RecursiveUnion {
+        base: Box<BoundSetExpr>,
+        recursive: Box<BoundSetExpr>,
+    },


I feel this is not necessary as well. Consider recursive functions in common programming languages, compiler won't have any special representations of the expressions in this function just because it is recursive, right? 🤔

But it's a special operator.

RecursiveUnion has extremely different schema/stream-key derivation logical, and have unique executor implementations in all databases.

https://github.com/postgres/postgres/blob/master/src/backend/executor/nodeRecursiveunion.c

In postgres

https://github.com/duckdb/duckdb/blob/main/src/execution/operator/set/physical_recursive_cte.cpp

In duckdb (UNION is executed in the operator)

You will find that in RCTE definition, UNION is special enough. The top level operator of RCTE must be a UNION or UNION ALL.

I am on the other side of the spectrum 😂 It is too special to be part of BoundSetExpr. Most places accepting BoundSetExpr would not accept this variant, while the place that is designated for this special variant would not allow other variants.

In my mind it would look like the following:

pub enum BindingCteState { /// We know nothing about the CTE before resolve the body. #[default] Init, /// We know the schema from after the base term resolved. BaseResolved { schema: Schema }, /// We get the whole bound result. Bound { query: Either<BoundQuery, RecursiveUnion> }, } struct RecursiveUnion { all: bool, base: Box<BoundQuery>, recursive: Box<BoundQuery>, }

(or the Either can be flatten into BoundNonRecursiveQuery and BoundRecursiveUnion)

Note the difference between Query and SetExpr is the former can accept order/limit/etc. I have tried in PostgreSQL that:

test=# with recursive t as (select 1 union all select n+1 from t as t(n) where n < 5 order by 1) select * from t; ERROR: ORDER BY in a recursive query is not implemented LINE 1: ...ll select n+1 from t as t(n) where n < 5 order by 1) select ... ^ test=# with recursive t as ((select 1 limit 1) union all (select n+1 from t as t(n) where n < 5 limit 3)) select * from t; ?column? ---------- 1 2 3 4 5 (5 rows)

That is, a recursive cte cannot have order/limit/etc at root, but can have it as part of base or recursive term.

src/frontend/src/binder/bind_context.rs

xiangjinwu

rest lgtm

src/frontend/src/expr/mod.rs

xiangjinwu · 2024-03-14T07:40:33Z

src/frontend/src/binder/set_expr.rs

+    /// UNION in recursive CTE definition
+    RecursiveUnion {
+        base: Box<BoundSetExpr>,
+        recursive: Box<BoundSetExpr>,
+    },


I am on the other side of the spectrum 😂 It is too special to be part of BoundSetExpr. Most places accepting BoundSetExpr would not accept this variant, while the place that is designated for this special variant would not allow other variants.

In my mind it would look like the following:

pub enum BindingCteState { /// We know nothing about the CTE before resolve the body. #[default] Init, /// We know the schema from after the base term resolved. BaseResolved { schema: Schema }, /// We get the whole bound result. Bound { query: Either<BoundQuery, RecursiveUnion> }, } struct RecursiveUnion { all: bool, base: Box<BoundQuery>, recursive: Box<BoundQuery>, }

(or the Either can be flatten into BoundNonRecursiveQuery and BoundRecursiveUnion)

Note the difference between Query and SetExpr is the former can accept order/limit/etc. I have tried in PostgreSQL that:

test=# with recursive t as (select 1 union all select n+1 from t as t(n) where n < 5 order by 1) select * from t; ERROR: ORDER BY in a recursive query is not implemented LINE 1: ...ll select n+1 from t as t(n) where n < 5 order by 1) select ... ^ test=# with recursive t as ((select 1 limit 1) union all (select n+1 from t as t(n) where n < 5 limit 3)) select * from t; ?column? ---------- 1 2 3 4 5 (5 rows)

That is, a recursive cte cannot have order/limit/etc at root, but can have it as part of base or recursive term.

xiangjinwu · 2024-03-14T07:50:08Z

src/frontend/src/binder/query.rs

+            let share_id = self.next_share_id();
+            let Cte { alias, query, .. } = cte_table;
+            let table_name = alias.name.real_value();
+            if with.recursive {


Unfortunately this recursive is a single toggle for all CTEs (doc). When absent it allows a binder shortcut, but when specified we need to ~~attempt in both directions~~ for all CTEs. (Update on April 2nd: attempt and recover context is hard)

Example in PostgreSQL:

test=# with recursive t as (select 1) select * from t; -- unnecessary `recursive` allowed ?column? ---------- 1 (1 row)

src/frontend/src/binder/query.rs

Co-authored-by: xiangjinwu <[email protected]>

Signed-off-by: TennyZhuang <[email protected]>

…name` (#16023)

xzhseh · 2024-04-03T18:40:51Z

need some further review(s) to merge this pr, before things getting too large...
cc @wangrunji0408, @xiangjinwu.

ps. some additional contexts / discussions / reviews / to-do(s) can be found in #16023.

xiangjinwu · 2024-04-04T05:57:10Z

src/frontend/src/planner/relation.rs

+            // todo: ensure this will always be wrapped in a `Relation::Share`
+            // so that it will not be explicitly planned here
+            Relation::RecursiveUnion(..) => {
+                bail_not_implemented!(issue = 15135, "recursive CTE is not supported")
+            }


This suggest RecursiveUnion shall not be a variant of Relation, similar to how it is not a variant of SetExpr.

It seems this variant is mainly introduced so that it can fit into BoundShare { input: Relation }. And also similarly, I feel like it shall be updated to BoundShare { input: Either<BoundQuery, RecursiveUnion> }.

All existing BoundShare.input before this PR are always Relation::Subquery(BoundSubquery{ lateral: false, query: BoundQuery}) which can be simplified into input: BoundQuery. @chenzl25 Could you help clarify whether this simplification is valid?

Even if the simplification is not valid, it might be possible to use Either<Relation, RecursiveUnion> (so that relations other than inside BoundShare are impossible to include RecursiveUnion).

Furthermore, it looks unnecessary to have both bind_context::RecursiveUnion and relation::BoundRecursiveUnion. The one in bind_context is already bound (maybe update its name) and can be reused in BoundShare.

All existing BoundShare.input before this PR are always Relation::Subquery(BoundSubquery{ lateral: false, query: BoundQuery}) which can be simplified into input: BoundQuery. @chenzl25 Could you help clarify whether this simplification is valid?

I have revisited the usage of BoundShare.input and indeed the input right now could only be BoundQuery . Previously, this struct was designed to support source sharing as well, however, source sharing is implemented in the optimizer phase, so we can use BoundQuery to replace the Relation and finally, we could update it to BoundShare { input: Either<BoundQuery, RecursiveUnion> } to support recursive CTE.

thanks 🩵 - this make sense to me, let me do some refactor later.

src/frontend/src/binder/relation/mod.rs

xiangjinwu · 2024-04-04T06:03:34Z

src/frontend/src/binder/bind_context.rs

+                    if let BindingCteState::Bound { .. } =
+                        self.cte_to_relation.get(&k).unwrap().borrow().state.clone()
+                    {
+                        // do nothing


#16023 (comment)

We shall add the test case to showcase the necessity of this special check. (Btw I am still trying to understand it at this time. The ask for test case is not an endorsement of it being valid 😂 )

Panicked here:

thread 'rw-main' panicked at src/frontend/src/binder/bind_context.rs:368:54: called `Option::unwrap()` on a `None` value

should be fixed now - just update the logic when checking if merge_context is applying for rcte.

It seems this special bypass just suppress something that is actually wrong behind the scene:

with recursive t(n) as ( values (1) union all select n + 1 from t where n < 100 ) select * from t; ______________^ here

Without the update here, the error is saying that t is already in BindContext before binding t. This error is TRUE, and adding this bypass does not fix the actual dirty-context problem: before binding t, the context is supposed to be empty.

As an example, having a dirty initial context makes the following malformed query to pass binder validation (and panic in planner):

with recursive t(n) as ( values (1) union all select n + 1 from t where n < 100 ) select n; -- n is not supposed to be in context, but the binder implementation here contains it somehow

I am still tracing down how the context become dirty.

risingwave/src/frontend/src/binder/query.rs

Lines 378 to 379 in 3f50c60

} else {

let bound_query = self.bind_query(query)?;

The nonrecursive branch's bind_query pushes a new context upon entry and clears it before returning, while the recursive branch's two bind_set_expr calls write directly into current context. Maybe (?) this is the reason 🤔

The problem would not arise if both sides of RecursiveUnion were considered Query rather than SetExpr

Fix available for review: #16282

src/frontend/src/binder/query.rs

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang · 2024-04-07T05:45:56Z

Added a test

xzhseh · 2024-04-08T21:14:31Z

ready to merge, cc @chenzl25 @TennyZhuang @xiangjinwu.

xiangjinwu

Noting down limitations to be refined in the future:

recursive keyword may be followed by multiple CTEs, some of which may not be actual recursive.
align_schema may either promote lhs (base) or rhs (recursive) data type to reach a common type. However, given the lhs schema is already persisted in BindingCteState::BaseResolved, it shall not be promoted in the later stage. As in PostgreSQL:

// lhs bigint larger than rhs int, OK
test=# WITH RECURSIVE t1 AS (SELECT 1::bigint AS a UNION ALL SELECT (a + 1)::int as b FROM t1 WHERE a < 5) SELECT * FROM t1;
 a 
---
 1
 2
 3
 4
 5
(5 rows)

// lhs int smaller than rhs bigint, Err
test=# WITH RECURSIVE t1 AS (SELECT 1::int AS a UNION ALL SELECT (a + 1)::bigint as b FROM t1 WHERE a < 5) SELECT * FROM t1;
ERROR:  recursive query "t1" column 1 has type integer in non-recursive term but type bigint overall
LINE 1: WITH RECURSIVE t1 AS (SELECT 1::int AS a UNION ALL SELECT (a...
                                     ^
HINT:  Cast the output of the non-recursive term to the correct type.

// lhs null not inferred as rhs int but fall back to text
test=# WITH RECURSIVE t1 AS (SELECT null AS a UNION ALL SELECT (a + 1)::int as b FROM t1 WHERE a < 5) SELECT * FROM t1;
ERROR:  operator does not exist: text + integer
LINE 1: ...RSIVE t1 AS (SELECT null AS a UNION ALL SELECT (a + 1)::int ...
                                                             ^
HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.

// When nonrecursive, lhs int allowed to be smaller than rhs bigint
test=# WITH t1 AS (SELECT 1::int AS a UNION ALL SELECT 2::bigint as b) SELECT * FROM t1;
 a 
---
 1
 2
(2 rows)

// When nonrecursive, lhs null inferred as rhs int
test=# WITH t1 AS (SELECT null AS a UNION ALL SELECT 2::int as b) SELECT * FROM t1;
 a 
---
  
 2
(2 rows)

feat(binder): bind RCTE

27b5439

Signed-off-by: TennyZhuang <[email protected]>

github-actions bot added the type/feature label Mar 7, 2024

Merge branch 'main' into bind_rcte

24d763b

TennyZhuang marked this pull request as ready for review March 13, 2024 07:06

TennyZhuang requested review from chenzl25, wangrunji0408, xiangjinwu and BugenZhao March 13, 2024 07:07

Merge branch 'main' into bind_rcte

51cefe3

wangrunji0408 reviewed Mar 14, 2024

View reviewed changes

TennyZhuang commented Mar 14, 2024

View reviewed changes

src/frontend/src/binder/bind_context.rs Outdated Show resolved Hide resolved

Update src/frontend/src/binder/bind_context.rs

20493ab

xiangjinwu reviewed Mar 14, 2024

View reviewed changes

TennyZhuang and others added 2 commits March 14, 2024 18:01

Update src/frontend/src/expr/mod.rs

cfb9920

Co-authored-by: xiangjinwu <[email protected]>

Merge branch 'main' into bind_rcte

a5c7d81

xzhseh self-assigned this Mar 24, 2024

TennyZhuang and others added 4 commits March 29, 2024 14:22

Merge remote-tracking branch 'origin/main' into bind_rcte

f8e3788

update

76cc79c

Signed-off-by: TennyZhuang <[email protected]>

refine

8e5b68a

Signed-off-by: TennyZhuang <[email protected]>

update comment; fix typo(s)

e2292a3

xiangjinwu mentioned this pull request Apr 2, 2024

feat(binder): correctly bind rcte in bind_with & bind_relation_by_name #16023

Merged

9 tasks

xzhseh added 2 commits April 3, 2024 14:38

feat(binder): correctly bind rcte in bind_with & `bind_relation_by_…

78a2422

…name` (#16023)

Merge branch 'main' into bind_rcte

ced2f59

xzhseh requested review from wangrunji0408 and xiangjinwu April 3, 2024 18:41

xiangjinwu reviewed Apr 4, 2024

View reviewed changes

xzhseh and others added 3 commits April 4, 2024 11:17

fix nit; cleanup comment

6b84174

fix warning

8ada38b

Signed-off-by: TennyZhuang <[email protected]>

add a binder test

3175084

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang requested a review from a team as a code owner April 7, 2024 05:44

TennyZhuang and others added 7 commits April 7, 2024 16:33

Merge branch 'main' into bind_rcte

b0f1446

fix check in bind_context when merge_context with rcte

65379b0

remove BoundRecursiveUnion; refactor input field for BoundShare

f81531e

update fmt

4044d54

add bail_not_implemented to prevent stack overflow in plan_relation

da9776c

update comments

ebaa6c2

update unit test accordingly

79b7015

xzhseh requested a review from xiangjinwu April 8, 2024 21:14

update test's format to fix ci

249408e

xzhseh force-pushed the bind_rcte branch from 62a25af to 249408e Compare April 9, 2024 01:53

refactor(binder): update bind_rcte to be more rusty (#16214)

aa6cdb4

xiangjinwu approved these changes Apr 9, 2024

View reviewed changes

TennyZhuang enabled auto-merge April 9, 2024 08:02

TennyZhuang added this pull request to the merge queue Apr 9, 2024

Merged via the queue into main with commit 0c8371a Apr 9, 2024
28 of 29 checks passed

TennyZhuang deleted the bind_rcte branch April 9, 2024 08:30

xiangjinwu mentioned this pull request Apr 12, 2024

fix(binder): RCTE context shall not leak to main query #16282

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(binder): bind RCTE #15522

feat(binder): bind RCTE #15522

TennyZhuang commented Mar 7, 2024 •

edited

Loading

wangrunji0408 Mar 14, 2024

TennyZhuang Mar 14, 2024

TennyZhuang Mar 14, 2024 •

edited

Loading

wangrunji0408 Mar 14, 2024

TennyZhuang Mar 14, 2024

TennyZhuang Mar 14, 2024

TennyZhuang Mar 14, 2024

TennyZhuang Mar 14, 2024

xiangjinwu Mar 14, 2024 •

edited

Loading

xiangjinwu left a comment

xiangjinwu Mar 14, 2024 •

edited

Loading

xiangjinwu Mar 14, 2024 •

edited

Loading

xzhseh commented Apr 3, 2024 •

edited

Loading

xiangjinwu Apr 4, 2024

chenzl25 Apr 6, 2024

xzhseh Apr 7, 2024 •

edited

Loading

xiangjinwu Apr 4, 2024

TennyZhuang Apr 7, 2024

xzhseh Apr 7, 2024

xiangjinwu Apr 9, 2024

xiangjinwu Apr 9, 2024 •

edited

Loading

xiangjinwu Apr 12, 2024

TennyZhuang commented Apr 7, 2024

xzhseh commented Apr 8, 2024

xiangjinwu left a comment •

edited

Loading

feat(binder): bind RCTE #15522

feat(binder): bind RCTE #15522

Conversation

TennyZhuang commented Mar 7, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TennyZhuang Mar 14, 2024 • edited Loading

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

xiangjinwu Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

xiangjinwu left a comment

Choose a reason for hiding this comment

xiangjinwu Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

xiangjinwu Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

xzhseh commented Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhseh Apr 7, 2024 • edited Loading

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

xiangjinwu Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TennyZhuang commented Apr 7, 2024

xzhseh commented Apr 8, 2024

xiangjinwu left a comment • edited Loading

Choose a reason for hiding this comment

TennyZhuang commented Mar 7, 2024 •

edited

Loading

TennyZhuang Mar 14, 2024 •

edited

Loading

xiangjinwu Mar 14, 2024 •

edited

Loading

xiangjinwu Mar 14, 2024 •

edited

Loading

xiangjinwu Mar 14, 2024 •

edited

Loading

xzhseh commented Apr 3, 2024 •

edited

Loading

xzhseh Apr 7, 2024 •

edited

Loading

xiangjinwu Apr 9, 2024 •

edited

Loading

xiangjinwu left a comment •

edited

Loading