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

xzhseh · 2024-03-30T04:26:33Z

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

What's changed and what's your intention?

some subsequent work(s) of #15522 for binding rcte.

related: #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.

…ursiveUnion>

xzhseh · 2024-03-30T04:32:36Z

src/frontend/src/binder/query.rs

-                // https://www.postgresql.org/docs/16/sql-select.html#:~:text=the%20recursive%20self%2Dreference%20must%20appear%20on%20the%20right%2Dhand%20side%20of%20the%20UNION
-                let bound_base = self.bind_set_expr(*left)?;
+                // todo: to be further reviewed
+                fn gen_query(s: SetExpr) -> Query {


ps. the below example is copied from https://github.com/risingwavelabs/risingwave/pull/15522/files#r1524367781.

with recursive t as ((select 1 limit 1) union all (select n+1 from t as t(n) where n < 5 limit 3));

since under current definition of union, we can only get SetExpr rather than an entire Query for left and right.

thus, should we use BoundSetExpr as a workaround (and also KISS) for RecursiveUnion at present, or just initially conforming to postgres's behavior - which of course, need to modify some more stuff.

cc @chenzl25, @xiangjinwu.

Using BoundSetExpr LGTM, because BoundQuery and BoundSetExpr contains each other actually. 🥵

then let's stick with BoundSetExpr first.

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

xzhseh · 2024-03-31T05:06:02Z

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

@@ -377,16 +378,23 @@ impl Binder {
                    Ok(Relation::BackCteRef(Box::new(BoundBackCteRef { share_id })))
                }
                BindingCteState::Bound { query } => {
+                    let schema = match query.clone() {
+                        Left(normal) => normal.body.schema().clone(),
+                        Right(recursive) => recursive.recursive.body.schema().clone(),


same, could we just use the recursive schema here?

RecursiveUnion is also a union, so we need to align its inputs' schema, after that, we can use any input's schema as the union's schema.
See for more details

risingwave/src/frontend/src/binder/set_expr.rs

Line 125 in a55ad16

pub(super) fn bind_set_expr(&mut self, set_expr: SetExpr) -> Result<BoundSetExpr> {

plus, for the case like below (e.g., <value> union all <select stmt>),
we might directly use rhs's schema as the final schema.

since align_schema only handles select expr(s).

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

TennyZhuang · 2024-04-01T11:17:31Z

What will happen if we write a CTE with recursive keyword but not really have recursive reference? It seems that it's acceptable in pg.

dev=# with recursive t AS (select 1) select * from t;
 ?column?
----------
        1
(1 row)

dev=#
dev=# with recursive t AS (select 1 UNION ALL select 1) select * from t;
 ?column?
----------
        1
        1
(2 rows)

cc @xiangjinwu Can you give some ideas?

xzhseh · 2024-04-01T13:27:35Z

What will happen if we write a CTE with recursive keyword but not really have recursive reference? It seems that it's acceptable in pg.
dev=# with recursive t AS (select 1) select * from t;
 ?column?
----------
        1
(1 row)

dev=#
dev=# with recursive t AS (select 1 UNION ALL select 1) select * from t;
 ?column?
----------
        1
        1
(2 rows)
cc @xiangjinwu Can you give some ideas?

one solution might be - treat it as normal union iff we find the base and recursive parts are both constant-like expr;

but,

this involves extra (and potentially large) conditional checking paths to detect these cases - since they are already a BoundQuery.
will there be any other corner case?

TennyZhuang · 2024-04-01T15:28:07Z

treat it as normal union iff we find the base and recursive parts are both constant-like expr;

Currently, we’ll reject recursive union without a top-level UNION. I guess we can try bind it as non recursive CTE when met this.
For CTE with recursive definition but non-recursive body, may we can convert it as a normal UNION in logical planner?

cc @dylan

Not necessary to resolve the case in the PR.

xzhseh · 2024-04-01T18:03:49Z

src/frontend/src/planner/relation.rs

@@ -58,6 +58,11 @@ impl Planner {
            Relation::BackCteRef(..) => {
                bail_not_implemented!(issue = 15135, "recursive CTE is not supported")
            }
+            // todo: ensure this will always be wrapped in a `Relation::Share`


correct me if this is not true.

chenzl25 · 2024-04-02T03:11:13Z

treat it as normal union iff we find the base and recursive parts are both constant-like expr;

Currently, we’ll reject recursive union without a top-level UNION. I guess we can try bind it as non recursive CTE when met this.

For CTE with recursive definition but non-recursive body, may we can convert it as a normal UNION in logical planner?

cc @dylan

Not necessary to resolve the case in the PR.

Yes, I think we can just bind it to a normal UNION operator when it is proved that it is not a recursive CTE.

xiangjinwu · 2024-04-02T05:09:33Z

What will happen if we write a CTE with recursive keyword but not really have recursive reference? It seems that it's acceptable in pg.

cc @xiangjinwu Can you give some ideas?

#15522 (comment)

If we know, at the binding phase, it is not recursive, we can just bind to a normal union (wrapped inside SetExpr and Query). As per my comment in that PR, the top level would be Either<BoundQuery, RecursiveUnion>.

TennyZhuang · 2024-04-02T05:19:21Z

If we know, at the binding phase, it is not recursive

I guess it’s a little hard for our binder to know this, since we don’t have visitor on binder structure?

Or we can just store a flag in context.

xzhseh · 2024-04-02T17:06:00Z

I guess it’s a little hard for our binder to know this, since we don’t have visitor on binder structure?

a somewhat "hacky" solution would be, (with recursive flag enabled) check if the pattern like select (...) from <cte_name> appears in the rhs (a.k.a. the recursive term) for our currently binding union - if so, treat this as a recursvie cte, vice versa.

problem is - this is definitely error-prone and add extra (potentially ugly) hard-coded checking when bind_with; and the semantic may not be exactly aligned with postgres's.

Or we can just store a flag in context.

could you elaborate on this approach? I don't quite get the idea.

TennyZhuang · 2024-04-03T06:02:37Z

src/frontend/src/binder/relation/recursive_union.rs

+}
+
+impl RewriteExprsRecursive for BoundRecursiveUnion {
+    fn rewrite_exprs_recursive(&mut self, _rewriter: &mut impl crate::expr::ExprRewriter) {}


The implementation seems should be blanket?

what does it mean by "blanket"?

ps. this part is the same as BoundBackRef.

The operator should be similar to BoundSetExpr, but not BoundBackRef

updated - now we will rewrite the two BoundSetExpr respectively.

TennyZhuang

It's really hard to test binder in our codebase, the codes LGTM. We can refine it later when finish the planner part.

chenzl25 · 2024-04-03T08:14:34Z

src/frontend/src/binder/query.rs

-                // https://www.postgresql.org/docs/16/sql-select.html#:~:text=the%20recursive%20self%2Dreference%20must%20appear%20on%20the%20right%2Dhand%20side%20of%20the%20UNION
-                let bound_base = self.bind_set_expr(*left)?;
+                // todo: to be further reviewed
+                fn gen_query(s: SetExpr) -> Query {


Using BoundSetExpr LGTM, because BoundQuery and BoundSetExpr contains each other actually. 🥵

xzhseh · 2024-04-03T18:31:10Z

src/frontend/src/binder/bind_context.rs

-                        e.key()
-                    ))
-                    .into());
+                    if let BindingCteState::Bound { .. } =


the special check here, is because the lateral context typically contains the same stuff of the new bound context, when binding the final statement for a rcte - that's why we don't what to return error for the case like this.

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

ps. need further review, cc @chenzl25 @TennyZhuang.
pss. I'll merge this pr first.

Updated here: #16282

…oundRecursiveUnion

remove RecursiveUnion in BoundSetExpr; make it Either<BoundQuery, Rec…

24b7e4d

…ursiveUnion>

xzhseh requested review from TennyZhuang, chenzl25 and xiangjinwu March 30, 2024 04:26

xzhseh self-assigned this Mar 30, 2024

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

update fmt

c7c4489

xzhseh commented Mar 30, 2024

View reviewed changes

update bind_relation_by_name

5e7cd1c

xzhseh commented Mar 31, 2024

View reviewed changes

src/frontend/src/binder/relation/mod.rs Outdated Show resolved Hide resolved

xzhseh commented Mar 31, 2024

View reviewed changes

xzhseh added 2 commits April 1, 2024 14:02

update comments; introduce BoundRecursiveUnion for bind_relation_by_name

fb8fe85

update fmt

89580d3

xzhseh commented Apr 1, 2024

View reviewed changes

xzhseh added 2 commits April 1, 2024 14:23

add aligned schema to RecursiveUnion

78e5841

udpate fmt

d6ea53d

xzhseh added 3 commits April 2, 2024 17:04

refactor bind_set_expr

d7c0931

more comments; properly align the schema in bind_with

3fc27bf

update fmt

7b4b3d3

xzhseh changed the title ~~feat(binder): update RecursiveUnion and related function(s)~~ feat(binder): correctly bind rcte in bind_with & bind_relation_by_name Apr 2, 2024

TennyZhuang reviewed Apr 3, 2024

View reviewed changes

TennyZhuang approved these changes Apr 3, 2024

View reviewed changes

chenzl25 approved these changes Apr 3, 2024

View reviewed changes

xzhseh added 2 commits April 3, 2024 14:25

change to BoundSetExpr for lhs and rhs of union

9870b10

update fmt

4b00693

xzhseh commented Apr 3, 2024

View reviewed changes

make rewrite_exprs_recursive blanket to base and recursion exprs in B…

2bccdce

…oundRecursiveUnion

xzhseh merged commit 78a2422 into bind_rcte Apr 3, 2024
5 of 15 checks passed

xzhseh deleted the bind_rcte_tmp_111 branch April 3, 2024 18:38

xzhseh mentioned this pull request Apr 3, 2024

feat(binder): bind RCTE #15522

Merged

9 tasks

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): correctly bind rcte in `bind_with` & `bind_relation_by_name` #16023

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

xzhseh commented Mar 30, 2024 •

edited

Loading

xzhseh Mar 30, 2024 •

edited

Loading

xzhseh Apr 2, 2024

chenzl25 Apr 3, 2024

xzhseh Apr 3, 2024

xzhseh Mar 31, 2024

chenzl25 Apr 1, 2024

This comment was marked as outdated.

xzhseh Apr 2, 2024 •

edited

Loading

TennyZhuang commented Apr 1, 2024

xzhseh commented Apr 1, 2024 •

edited

Loading

TennyZhuang commented Apr 1, 2024

xzhseh Apr 1, 2024

chenzl25 commented Apr 2, 2024

xiangjinwu commented Apr 2, 2024

TennyZhuang commented Apr 2, 2024

xzhseh commented Apr 2, 2024

TennyZhuang Apr 3, 2024

xzhseh Apr 3, 2024

TennyZhuang Apr 3, 2024

xzhseh Apr 3, 2024

TennyZhuang left a comment

chenzl25 Apr 3, 2024

xzhseh Apr 3, 2024 •

edited

Loading

xiangjinwu Apr 12, 2024

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

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

Conversation

xzhseh commented Mar 30, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

xzhseh Mar 30, 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

This comment was marked as outdated.

xzhseh Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

TennyZhuang commented Apr 1, 2024

xzhseh commented Apr 1, 2024 • edited Loading

TennyZhuang commented Apr 1, 2024

Choose a reason for hiding this comment

chenzl25 commented Apr 2, 2024

xiangjinwu commented Apr 2, 2024

TennyZhuang commented Apr 2, 2024

xzhseh commented Apr 2, 2024

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

TennyZhuang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhseh Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

xzhseh commented Mar 30, 2024 •

edited

Loading

xzhseh Mar 30, 2024 •

edited

Loading

xzhseh Apr 2, 2024 •

edited

Loading

xzhseh commented Apr 1, 2024 •

edited

Loading

xzhseh Apr 3, 2024 •

edited

Loading