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

Jump to bottom

fix: avoid dependency loop when selecting from view. #10650

Merged
merged 2 commits into from
Mar 19, 2023
Merged

fix: avoid dependency loop when selecting from view. #10650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3a881d
Avoid dependency loop when selecting from view.
RinChanNOWWW Mar 19, 2023
922073b
Merge branch 'main' into check-view-dep
mergify[bot] Mar 19, 2023
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
9 changes: 6 additions & 3 deletions src/query/sql/src/planner/binder/bind_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,10 @@ pub struct BindContext {

pub ctes_map: Box<DashMap<String, CteInfo>>,

pub is_view: bool,
/// If current binding table is a view, record its database and name.
///
/// It's used to check if the view has a loop dependency.
pub view_info: Option<(String, String)>,
}

#[derive(Clone, Debug)]
Expand All @@ -154,7 +157,7 @@ impl BindContext {
windows: Vec::new(),
in_grouping: false,
ctes_map: Box::new(DashMap::new()),
is_view: false,
view_info: None,
}
}

Expand All @@ -167,7 +170,7 @@ impl BindContext {
windows: vec![],
in_grouping: false,
ctes_map: parent.ctes_map.clone(),
is_view: false,
view_info: None,
}
}

Expand Down
27 changes: 24 additions & 3 deletions src/query/sql/src/planner/binder/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,25 @@ impl Binder {
.await
}

fn check_view_dep(bind_context: &BindContext, database: &str, view_name: &str) -> Result<()> {
match &bind_context.parent {
Some(parent) => match &parent.view_info {
Some((db, v)) => {
if db == database && v == view_name {
Err(ErrorCode::Internal(format!(
"View dependency loop detected (view: {}.{})",
database, view_name
)))
} else {
Self::check_view_dep(parent, database, view_name)
}
}
_ => Ok(()),
},
_ => Ok(()),
}
}

#[async_recursion]
async fn bind_single_table(
&mut self,
Expand Down Expand Up @@ -191,6 +210,7 @@ impl Binder {

match table_meta.engine() {
"VIEW" => {
Self::check_view_dep(bind_context, &database, &table_name)?;
let query = table_meta
.options()
.get(QUERY)
Expand All @@ -200,7 +220,8 @@ impl Binder {
// For view, we need use a new context to bind it.
let mut new_bind_context =
BindContext::with_parent(Box::new(bind_context.clone()));
new_bind_context.is_view = true;
new_bind_context.view_info =
Some((database.clone(), table_name.to_string()));
if let Statement::Query(query) = &stmt {
self.metadata.write().add_table(
catalog,
Expand Down Expand Up @@ -239,7 +260,7 @@ impl Binder {
database.clone(),
table_meta,
table_alias_name,
bind_context.is_view,
bind_context.view_info.is_some(),
);

let (s_expr, mut bind_context) = self
Expand Down Expand Up @@ -529,7 +550,7 @@ impl Binder {
windows: vec![],
in_grouping: false,
ctes_map: Box::new(DashMap::new()),
is_view: false,
view_info: None,
};
let (s_expr, mut new_bind_context) =
self.bind_query(&new_bind_context, &cte_info.query).await?;
Expand Down
36 changes: 36 additions & 0 deletions tests/sqllogictests/suites/base/05_ddl/05_0019_ddl_create_view
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,39 @@ AS
SELECT *
FROM
's3://testbucket/admin/data/tuple.parquet' (FILES => ('tuple.parquet','test.parquet'),FILE_FORMAT => 'parquet',PATTERN => '.*.parquet', aws_key_id => 'minioadmin', aws_secret_key => 'minioadmin', endpoint_url => 'http://127.0.0.1:9900/')

statement ok
drop view if exists loop_view1;

statement ok
drop view if exists loop_view2;

statement ok
drop view if exists loop_view3;

statement ok
create view loop_view1 as select * from loop_view3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we check the loop when create view ?

postgres=# create view v_t as select * from v_t;
ERROR:  relation "v_t" does not exist
LINE 1: create view v_t as select * from v_t;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid not in current implementation. Because when creating a view, we just store the SQL string.


statement ok
create view loop_view2 as select * from loop_view1;

statement ok
create view loop_view3 as select * from loop_view2;

statement error 1001
select * from loop_view1;

statement error 1001
select * from loop_view2;

statement error 1001
select * from loop_view2;

statement ok
drop view loop_view1;

statement ok
drop view loop_view2;

statement ok
drop view loop_view3;