Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Apr 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
24b7e4d
remove RecursiveUnion in BoundSetExpr; make it Either<BoundQuery, Rec…
xzhseh Mar 30, 2024
c7c4489
update fmt
xzhseh Mar 30, 2024
5e7cd1c
update bind_relation_by_name
xzhseh Mar 31, 2024
fb8fe85
update comments; introduce BoundRecursiveUnion for bind_relation_by_name
xzhseh Apr 1, 2024
89580d3
update fmt
xzhseh Apr 1, 2024
78e5841
add aligned schema to RecursiveUnion
xzhseh Apr 1, 2024
d6ea53d
udpate fmt
xzhseh Apr 1, 2024
d7c0931
refactor bind_set_expr
xzhseh Apr 2, 2024
3fc27bf
more comments; properly align the schema in bind_with
xzhseh Apr 2, 2024
7b4b3d3
update fmt
xzhseh Apr 2, 2024
9870b10
change to BoundSetExpr for lhs and rhs of union
xzhseh Apr 3, 2024
4b00693
update fmt
xzhseh Apr 3, 2024
2bccdce
make rewrite_exprs_recursive blanket to base and recursion exprs in B…
xzhseh Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/frontend/src/binder/bind_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use std::collections::hash_map::Entry;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::rc::Rc;

use either::Either;
use parse_display::Display;
use risingwave_common::catalog::{Field, Schema};
use risingwave_common::types::DataType;
Expand Down Expand Up @@ -93,7 +94,18 @@ pub enum BindingCteState {
/// We know the schema form after the base term resolved.
BaseResolved { schema: Schema },
/// We get the whole bound result of the (recursive) CTE.
Bound { query: BoundQuery },
Bound {
query: Either<BoundQuery, RecursiveUnion>,
},
}

/// the entire `RecursiveUnion` represents a *bound* recursive cte.
/// reference: <https://github.com/risingwavelabs/risingwave/pull/15522/files#r1524367781>
#[derive(Debug, Clone)]
pub struct RecursiveUnion {
pub all: bool,
pub base: Box<BoundQuery>,
pub recursive: Box<BoundQuery>,
}

#[derive(Clone, Debug)]
Expand Down
59 changes: 39 additions & 20 deletions src/frontend/src/binder/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use thiserror_ext::AsReport;
use super::bind_context::BindingCteState;
use super::statement::RewriteExprsRecursive;
use super::BoundValues;
use crate::binder::bind_context::BindingCte;
use crate::binder::bind_context::{BindingCte, RecursiveUnion};
use crate::binder::{Binder, BoundSetExpr};
use crate::error::{ErrorCode, Result};
use crate::expr::{CorrelatedId, Depth, ExprImpl, ExprRewriter};
Expand Down Expand Up @@ -287,6 +287,7 @@ impl Binder {
let share_id = self.next_share_id();
let Cte { alias, query, .. } = cte_table;
let table_name = alias.name.real_value();

if with.recursive {
let Query {
with,
Expand All @@ -296,6 +297,8 @@ impl Binder {
offset,
fetch,
} = query;

/// the input clause should not be supported.
fn should_be_empty<T>(v: Option<T>, clause: &str) -> Result<()> {
if v.is_some() {
return Err(ErrorCode::BindError(format!(
Expand All @@ -305,6 +308,7 @@ impl Binder {
}
Ok(())
}

should_be_empty(order_by.first(), "ORDER BY")?;
should_be_empty(limit, "LIMIT")?;
should_be_empty(offset, "OFFSET")?;
Expand Down Expand Up @@ -346,37 +350,52 @@ impl Binder {
self.bind_with(with)?;
}

// We assume `left` is base term, otherwise the implementation may be very hard.
// The behavior is same as PostgreSQL.
// 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 {
Copy link
Contributor Author

@xzhseh xzhseh Mar 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @chenzl25, @xiangjinwu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then let's stick with BoundSetExpr first.

Query {
body: s,
order_by: vec![],
limit: None,
offset: None,
with: None,
fetch: None,
}
}

// todo: how should we deal with separate order by / limit clause?
let left = gen_query(*left);
let right = gen_query(*right);

// We assume `left` is the base term, otherwise the implementation may be very hard.
// The behavior is the same as PostgreSQL's.
// reference: <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 base = self.bind_query(left)?;

entry.borrow_mut().state = BindingCteState::BaseResolved {
schema: bound_base.schema().clone(),
schema: base.schema().clone(),
};

let bound_recursive = self.bind_set_expr(*right)?;

let bound_query = BoundQuery {
body: BoundSetExpr::RecursiveUnion {
base: Box::new(bound_base),
recursive: Box::new(bound_recursive),
},
order: vec![],
limit: None,
offset: None,
with_ties: false,
extra_order_exprs: vec![],
// bind the rest of the recursive cte
let recursive = self.bind_query(right)?;

let recursive_union = RecursiveUnion {
all,
base: Box::new(base),
recursive: Box::new(recursive),
};

entry.borrow_mut().state = BindingCteState::Bound { query: bound_query };
entry.borrow_mut().state = BindingCteState::Bound {
query: either::Either::Right(recursive_union),
};
} else {
let bound_query = self.bind_query(query)?;
self.context.cte_to_relation.insert(
table_name,
Rc::new(RefCell::new(BindingCte {
share_id,
state: BindingCteState::Bound { query: bound_query },
state: BindingCteState::Bound {
query: either::Either::Left(bound_query),
},
alias,
})),
);
Expand Down
41 changes: 21 additions & 20 deletions src/frontend/src/binder/relation/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,26 +377,27 @@ impl Binder {
Ok(Relation::BackCteRef(Box::new(BoundBackCteRef { share_id })))
}
BindingCteState::Bound { query } => {
self.bind_table_to_context(
query
.body
.schema()
.fields
.iter()
.map(|f| (false, f.clone())),
table_name.clone(),
Some(original_alias),
)?;
// Share the CTE.
let input_relation = Relation::Subquery(Box::new(BoundSubquery {
query,
lateral: false,
}));
let share_relation = Relation::Share(Box::new(BoundShare {
share_id,
input: input_relation,
}));
Ok(share_relation)
todo!()
// self.bind_table_to_context(
// query
// .body
// .schema()
// .fields
// .iter()
// .map(|f| (false, f.clone())),
// table_name.clone(),
// Some(original_alias),
// )?;
// // Share the CTE.
// let input_relation = Relation::Subquery(Box::new(BoundSubquery {
// query,
// lateral: false,
// }));
// let share_relation = Relation::Share(Box::new(BoundShare {
// share_id,
// input: input_relation,
// }));
// Ok(share_relation)
}
}
} else {
Expand Down
24 changes: 0 additions & 24 deletions src/frontend/src/binder/set_expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,6 @@ pub enum BoundSetExpr {
left: Box<BoundSetExpr>,
right: Box<BoundSetExpr>,
},
/// UNION in recursive CTE definition
RecursiveUnion {
base: Box<BoundSetExpr>,
recursive: Box<BoundSetExpr>,
},
}

impl RewriteExprsRecursive for BoundSetExpr {
Expand All @@ -53,10 +48,6 @@ impl RewriteExprsRecursive for BoundSetExpr {
left.rewrite_exprs_recursive(rewriter);
right.rewrite_exprs_recursive(rewriter);
}
BoundSetExpr::RecursiveUnion { base, recursive } => {
base.rewrite_exprs_recursive(rewriter);
recursive.rewrite_exprs_recursive(rewriter);
}
}
}
}
Expand Down Expand Up @@ -87,7 +78,6 @@ impl BoundSetExpr {
BoundSetExpr::Values(v) => v.schema(),
BoundSetExpr::Query(q) => q.schema(),
BoundSetExpr::SetOperation { left, .. } => left.schema(),
BoundSetExpr::RecursiveUnion { base, .. } => base.schema(),
}
}

Expand All @@ -99,9 +89,6 @@ impl BoundSetExpr {
BoundSetExpr::SetOperation { left, right, .. } => {
left.is_correlated(depth) || right.is_correlated(depth)
}
BoundSetExpr::RecursiveUnion { base, recursive } => {
base.is_correlated(depth) || recursive.is_correlated(depth)
}
}
}

Expand Down Expand Up @@ -130,17 +117,6 @@ impl BoundSetExpr {
);
correlated_indices
}
BoundSetExpr::RecursiveUnion { base, recursive } => {
let mut correlated_indices = vec![];
correlated_indices.extend(
base.collect_correlated_indices_by_depth_and_assign_id(depth, correlated_id),
);
correlated_indices.extend(
recursive
.collect_correlated_indices_by_depth_and_assign_id(depth, correlated_id),
);
correlated_indices
}
}
}
}
Expand Down
12 changes: 0 additions & 12 deletions src/frontend/src/expr/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,10 +483,6 @@ impl ExprImpl {
self.visit_bound_set_expr(left);
self.visit_bound_set_expr(right);
}
BoundSetExpr::RecursiveUnion { base, recursive } => {
self.visit_bound_set_expr(base);
self.visit_bound_set_expr(recursive);
}
};
}
}
Expand Down Expand Up @@ -528,10 +524,6 @@ impl ExprImpl {
self.visit_bound_set_expr(left);
self.visit_bound_set_expr(right);
}
BoundSetExpr::RecursiveUnion { base, recursive } => {
self.visit_bound_set_expr(base);
self.visit_bound_set_expr(recursive);
}
}
}
}
Expand Down Expand Up @@ -601,10 +593,6 @@ impl ExprImpl {
self.visit_bound_set_expr(&mut *left);
self.visit_bound_set_expr(&mut *right);
}
BoundSetExpr::RecursiveUnion { base, recursive } => {
self.visit_bound_set_expr(&mut *base);
self.visit_bound_set_expr(&mut *recursive);
}
}
}
}
Expand Down
4 changes: 0 additions & 4 deletions src/frontend/src/planner/set_expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use risingwave_common::bail_not_implemented;
use risingwave_common::util::sort_util::ColumnOrder;

use crate::binder::BoundSetExpr;
Expand All @@ -38,9 +37,6 @@ impl Planner {
left,
right,
} => self.plan_set_operation(op, all, *left, *right),
BoundSetExpr::RecursiveUnion { .. } => {
bail_not_implemented!(issue = 15135, "recursive CTE is not supported")
}
}
}
}
Loading