You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kysshsy (via GitHub)" <gi...@apache.org> on 2025/12/28 10:41:19 UTC
[PR] fix: enhance CTE resolution with identifier normalization [datafusion]
kysshsy opened a new pull request, #19519:
URL: https://github.com/apache/datafusion/pull/19519
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #18932
## Rationale for this change
- Quoted CTE references (e.g., `WITH barbaz AS (...) SELECT * FROM "barbaz"`) were being collected as table refs and could fall through to catalog lookup instead of resolving to the CTE, especially with custom providers.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
- Normalize CTE and table references consistently during collection (preserve quote style; only lowercase unquoted idents when enabled).
- Avoid double-normalization when converting to `TableReference` in the resolve path.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
- Behavior fix for quoted CTE references; no API changes.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2672532009
##########
datafusion/sql/src/resolve.rs:
##########
@@ -193,19 +213,20 @@ pub fn resolve_table_references(
relations: BTreeSet::new(),
all_ctes: BTreeSet::new(),
ctes_in_scope: vec![],
+ normalizer: IdentNormalizer::new(enable_ident_normalization),
};
visit_statement(statement, &mut visitor);
let table_refs = visitor
.relations
.into_iter()
- .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+ .map(|x| object_name_to_table_reference(x, false))
Review Comment:
Can `RelationVisitor` directly collect normalized table references instead of raw object names so that there is no need for a second normalization here?
##########
datafusion/sql/src/resolve.rs:
##########
@@ -193,19 +213,20 @@ pub fn resolve_table_references(
relations: BTreeSet::new(),
all_ctes: BTreeSet::new(),
ctes_in_scope: vec![],
+ normalizer: IdentNormalizer::new(enable_ident_normalization),
};
visit_statement(statement, &mut visitor);
let table_refs = visitor
.relations
.into_iter()
- .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+ .map(|x| object_name_to_table_reference(x, false))
Review Comment:
Can `RelationVisitor` directly collect normalized table references instead of object names so that there is no need for a second normalization here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "kysshsy (via GitHub)" <gi...@apache.org>.
kysshsy commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2679815125
##########
datafusion/sql/src/resolve.rs:
##########
@@ -193,19 +213,20 @@ pub fn resolve_table_references(
relations: BTreeSet::new(),
all_ctes: BTreeSet::new(),
ctes_in_scope: vec![],
+ normalizer: IdentNormalizer::new(enable_ident_normalization),
};
visit_statement(statement, &mut visitor);
let table_refs = visitor
.relations
.into_iter()
- .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+ .map(|x| object_name_to_table_reference(x, false))
Review Comment:
I updated `RelationVisitor` to collect normalized `TableReferences` directly, so we don’t need a second normalization pass.
The only downside is error handling becomes a bit more involved, since converting to `TableReference` can fail and we now need to propagate that `Error` during visiting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "kysshsy (via GitHub)" <gi...@apache.org>.
kysshsy commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2679802296
##########
datafusion/sql/src/resolve.rs:
##########
@@ -270,4 +291,57 @@ mod tests {
assert_eq!(ctes.len(), 1);
assert_eq!(ctes[0].to_string(), "nodes");
}
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference() {
+ use crate::parser::DFParser;
+
+ let query = r#"with barbaz as (select 1) select * from "barbaz""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, true).unwrap();
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "barbaz");
+ // Quoted reference should still resolve to the CTE when normalization is on
+ assert_eq!(table_refs.len(), 0);
+ }
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference_normalization_off() {
+ use crate::parser::DFParser;
+
+ let query = r#"with barbaz as (select 1) select * from "barbaz""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, false).unwrap();
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "barbaz");
+ // Even with normalization off, quoted reference matches same-case CTE name
+ assert_eq!(table_refs.len(), 0);
+ }
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference_uppercase_normalization_on() {
+ use crate::parser::DFParser;
+
+ let query = r#"with FOObar as (select 1) select * from "FOObar""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, true).unwrap();
+ // CTE name is normalized to lowercase, quoted reference preserves case, so they differ
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "foobar");
+ assert_eq!(table_refs.len(), 1);
+ assert_eq!(table_refs[0].to_string(), "FOObar");
+ }
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference_uppercase_normalization_off() {
+ use crate::parser::DFParser;
+
+ let query = r#"with FOObar as (select 1) select * from "FOObar""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, false).unwrap();
+ // Without normalization, cases match exactly, so quoted reference resolves to the CTE
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "FOObar");
+ assert_eq!(table_refs.len(), 0);
+ }
Review Comment:
Thanks for the suggestion — I’ve added an end-to-end .slt regression
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2681301601
##########
datafusion/sqllogictest/test_files/cte_quoted_reference.slt:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Since this test is small, I think we can consolidate it into cte.slt. Maybe in next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#issuecomment-3743897213
Thanks again @kysshsy and @jonahgao
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "kysshsy (via GitHub)" <gi...@apache.org>.
kysshsy commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2679816379
##########
datafusion/sql/src/resolve.rs:
##########
@@ -49,13 +49,31 @@ struct RelationVisitor {
relations: BTreeSet<ObjectName>,
all_ctes: BTreeSet<ObjectName>,
ctes_in_scope: Vec<ObjectName>,
+ normalizer: IdentNormalizer,
}
impl RelationVisitor {
+ fn normalize_object_name(&self, name: &ObjectName) -> ObjectName {
Review Comment:
updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "kysshsy (via GitHub)" <gi...@apache.org>.
kysshsy commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2679809802
##########
datafusion/sql/src/resolve.rs:
##########
@@ -193,19 +213,20 @@ pub fn resolve_table_references(
relations: BTreeSet::new(),
all_ctes: BTreeSet::new(),
ctes_in_scope: vec![],
+ normalizer: IdentNormalizer::new(enable_ident_normalization),
};
visit_statement(statement, &mut visitor);
let table_refs = visitor
.relations
.into_iter()
- .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+ .map(|x| object_name_to_table_reference(x, false))
Review Comment:
It’s `false` because references are already normalized when collected, so we only need to convert to `TableReference` here and avoid normalizing twice.
Per the suggestion, the visitor now collects `TableReference` directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2666116159
##########
datafusion/sql/src/resolve.rs:
##########
@@ -193,19 +213,20 @@ pub fn resolve_table_references(
relations: BTreeSet::new(),
all_ctes: BTreeSet::new(),
ctes_in_scope: vec![],
+ normalizer: IdentNormalizer::new(enable_ident_normalization),
};
visit_statement(statement, &mut visitor);
let table_refs = visitor
.relations
.into_iter()
- .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+ .map(|x| object_name_to_table_reference(x, false))
Review Comment:
why is this always false now?
##########
datafusion/sql/src/resolve.rs:
##########
@@ -49,13 +49,31 @@ struct RelationVisitor {
relations: BTreeSet<ObjectName>,
all_ctes: BTreeSet<ObjectName>,
ctes_in_scope: Vec<ObjectName>,
+ normalizer: IdentNormalizer,
}
impl RelationVisitor {
+ fn normalize_object_name(&self, name: &ObjectName) -> ObjectName {
Review Comment:
Could you please add some comments that explain what this is doing and the rationale (the WHY)? I think you can adapt your PR description.
##########
datafusion/sql/src/resolve.rs:
##########
@@ -270,4 +291,57 @@ mod tests {
assert_eq!(ctes.len(), 1);
assert_eq!(ctes[0].to_string(), "nodes");
}
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference() {
+ use crate::parser::DFParser;
+
+ let query = r#"with barbaz as (select 1) select * from "barbaz""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, true).unwrap();
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "barbaz");
+ // Quoted reference should still resolve to the CTE when normalization is on
+ assert_eq!(table_refs.len(), 0);
+ }
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference_normalization_off() {
+ use crate::parser::DFParser;
+
+ let query = r#"with barbaz as (select 1) select * from "barbaz""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, false).unwrap();
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "barbaz");
+ // Even with normalization off, quoted reference matches same-case CTE name
+ assert_eq!(table_refs.len(), 0);
+ }
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference_uppercase_normalization_on() {
+ use crate::parser::DFParser;
+
+ let query = r#"with FOObar as (select 1) select * from "FOObar""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, true).unwrap();
+ // CTE name is normalized to lowercase, quoted reference preserves case, so they differ
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "foobar");
+ assert_eq!(table_refs.len(), 1);
+ assert_eq!(table_refs[0].to_string(), "FOObar");
+ }
+
+ #[test]
+ fn resolve_table_references_cte_with_quoted_reference_uppercase_normalization_off() {
+ use crate::parser::DFParser;
+
+ let query = r#"with FOObar as (select 1) select * from "FOObar""#;
+ let statement = DFParser::parse_sql(query).unwrap().pop_back().unwrap();
+ let (table_refs, ctes) = resolve_table_references(&statement, false).unwrap();
+ // Without normalization, cases match exactly, so quoted reference resolves to the CTE
+ assert_eq!(ctes.len(), 1);
+ assert_eq!(ctes[0].to_string(), "FOObar");
+ assert_eq!(table_refs.len(), 0);
+ }
Review Comment:
Can you also please add some .slt tests so we can see this working when running end to end in queries?
Instructions are here: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "kysshsy (via GitHub)" <gi...@apache.org>.
kysshsy commented on PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#issuecomment-3706891851
@alamb @comphead Could you take a look or suggest an alternative approach? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #19519:
URL: https://github.com/apache/datafusion/pull/19519
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "kysshsy (via GitHub)" <gi...@apache.org>.
kysshsy commented on PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#issuecomment-3734920127
Thanks for the thorough review and the helpful suggestions. @alamb @jonahgao
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2686064404
##########
datafusion/sqllogictest/test_files/cte_quoted_reference.slt:
##########
@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Good idea -- I filed this ticket to track the idea:
- https://github.com/apache/datafusion/issues/19786
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enhance CTE resolution with identifier normalization [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #19519:
URL: https://github.com/apache/datafusion/pull/19519#discussion_r2672532009
##########
datafusion/sql/src/resolve.rs:
##########
@@ -193,19 +213,20 @@ pub fn resolve_table_references(
relations: BTreeSet::new(),
all_ctes: BTreeSet::new(),
ctes_in_scope: vec![],
+ normalizer: IdentNormalizer::new(enable_ident_normalization),
};
visit_statement(statement, &mut visitor);
let table_refs = visitor
.relations
.into_iter()
- .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+ .map(|x| object_name_to_table_reference(x, false))
Review Comment:
Can `RelationVisitor` directly collect table references instead of object names so that there is no need for a second normalization here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org