fix: enhance CTE resolution with identifier normalization by kysshsy · Pull Request #19519 · apache/datafusion

kysshsy · 2025-12-28T10:41:17Z

Which issue does this PR close?

Closes Implementing CatalogProvider breaks quoted CTE names #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.

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.

Are these changes tested?

Yes

Are there any user-facing changes?

Behavior fix for quoted CTE references; no API changes.

kysshsy · 2026-01-03T08:27:52Z

@alamb @comphead Could you take a look or suggest an alternative approach? Thanks!

alamb

Thanks @kysshsy -- this looks like a good start.

FYI @jonahgao -- perhaps you would have some time to review this PR

alamb · 2026-01-06T19:57:00Z

 }

 impl RelationVisitor {
+    fn normalize_object_name(&self, name: &ObjectName) -> ObjectName {


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.

alamb · 2026-01-06T19:57:45Z

        .relations
        .into_iter()
-        .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+        .map(|x| object_name_to_table_reference(x, false))


why is this always false now?

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.

alamb · 2026-01-06T19:58:21Z

+        assert_eq!(ctes.len(), 1);
+        assert_eq!(ctes[0].to_string(), "FOObar");
+        assert_eq!(table_refs.len(), 0);
+    }


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

Thanks for the suggestion — I’ve added an end-to-end .slt regression

jonahgao · 2026-01-08T14:15:35Z

        .relations
        .into_iter()
-        .map(|x| object_name_to_table_reference(x, enable_ident_normalization))
+        .map(|x| object_name_to_table_reference(x, false))


Can RelationVisitor directly collect normalized table references instead of object names so that there is no need for a second normalization here?

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.

… error management

Add `cte_quoted_reference.slt` and a strict `CatalogProvider`/`SchemaProvider` used only for that file. The provider panics on unexpected table lookups so we can observe when a CTE reference is maybe incorrectly treated as a catalog lookup. Refs: apache#18932

kysshsy · 2026-01-11T16:15:15Z

Thanks for the thorough review and the helpful suggestions. @alamb @jonahgao

jonahgao

LGTM, thank you @kysshsy . I have fixed the ci failures in 168baba

jonahgao · 2026-01-12T08:42:02Z

@@ -0,0 +1,70 @@
+# Licensed to the Apache Software Foundation (ASF) under one


Since this test is small, I think we can consolidate it into cte.slt. Maybe in next PR.

Good idea -- I filed this ticket to track the idea:

Consolidate datafusion/sqllogictest/test_files/cte_quoted_reference.slt #19786

alamb · 2026-01-13T11:48:55Z

Thanks again @kysshsy and @jonahgao

## Which issue does this PR close? N/A ## Rationale for this change PR #19519 introduces new tests to verify that DataFusion won't unexpectedly lookup CTE names from the catalog. It registers a strict SchemaProvider that panics with unexpected table lookups. https://github.com/apache/datafusion/blob/d138c36cb08c2dc028b29bbd20853b24cf0f3b8b/datafusion/sqllogictest/src/test_context.rs#L230-L241 The problem is that PR #19862 skips registering the strict SchemaProvider and bypasses the unexpected lookup check. Therefore, there tests become meaningless.  ## What changes are included in this PR? Re-register the strict SchemaProvider into cte.slt.  ## Are these changes tested? Yes. I also backported these new tests to tag 52.0.0, which does not include the fix in #19519, and they failed as expected. ```sh $ cargo test --profile release-nonlto --test sqllogictests -- cte Compiling datafusion-sqllogictest v52.0.0 (/Users/jonah/Work/datafusion/datafusion/sqllogictest) Finished `release-nonlto` profile [optimized] target(s) in 19.50s Running bin/sqllogictests.rs (/Users/jonah/Work/datafusion/target/release-nonlto/deps/sqllogictests-f9c15c7896eb5af9) [00:00:00] ####------------------------------------ 6/75 "cte.slt" thread 'tokio-runtime-worker' (486408) panicked at datafusion/sqllogictest/src/test_context.rs:215:22: unexpected table lookup: barbaz. This maybe indicates a CTE reference was incorrectly treated as a catalog table reference. note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Completed 3 test files in 0 seconds failure in cte.slt for sql with barbaz as (select * from orders) select * from "barbaz"; caused by External error: task 13 panicked with message "unexpected table lookup: barbaz. This maybe indicates a CTE reference was incorrectly treated as a catalog table reference." Error: Execution("1 failures") error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests` ```  ## Are there any user-facing changes? No.   --------- Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>

## Which issue does this PR close?  - Closes apache#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.  ## 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.  ## Are these changes tested? Yes  ## Are there any user-facing changes? - Behavior fix for quoted CTE references; no API changes.   --------- Co-authored-by: jonahgao <jonahgao@msn.com>

## Which issue does this PR close? N/A ## Rationale for this change PR apache#19519 introduces new tests to verify that DataFusion won't unexpectedly lookup CTE names from the catalog. It registers a strict SchemaProvider that panics with unexpected table lookups. https://github.com/apache/datafusion/blob/d138c36cb08c2dc028b29bbd20853b24cf0f3b8b/datafusion/sqllogictest/src/test_context.rs#L230-L241 The problem is that PR apache#19862 skips registering the strict SchemaProvider and bypasses the unexpected lookup check. Therefore, there tests become meaningless.  ## What changes are included in this PR? Re-register the strict SchemaProvider into cte.slt.  ## Are these changes tested? Yes. I also backported these new tests to tag 52.0.0, which does not include the fix in apache#19519, and they failed as expected. ```sh $ cargo test --profile release-nonlto --test sqllogictests -- cte Compiling datafusion-sqllogictest v52.0.0 (/Users/jonah/Work/datafusion/datafusion/sqllogictest) Finished `release-nonlto` profile [optimized] target(s) in 19.50s Running bin/sqllogictests.rs (/Users/jonah/Work/datafusion/target/release-nonlto/deps/sqllogictests-f9c15c7896eb5af9) [00:00:00] ####------------------------------------ 6/75 "cte.slt" thread 'tokio-runtime-worker' (486408) panicked at datafusion/sqllogictest/src/test_context.rs:215:22: unexpected table lookup: barbaz. This maybe indicates a CTE reference was incorrectly treated as a catalog table reference. note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Completed 3 test files in 0 seconds failure in cte.slt for sql with barbaz as (select * from orders) select * from "barbaz"; caused by External error: task 13 panicked with message "unexpected table lookup: barbaz. This maybe indicates a CTE reference was incorrectly treated as a catalog table reference." Error: Execution("1 failures") error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests` ```  ## Are there any user-facing changes? No.   --------- Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>

github-actions Bot added the sql SQL Planner label Dec 28, 2025

kysshsy force-pushed the fix_18932 branch from 61517fa to b21d8c4 Compare December 28, 2025 10:41

alamb reviewed Jan 6, 2026

View reviewed changes

jonahgao reviewed Jan 8, 2026

View reviewed changes

kysshsy added 3 commits January 11, 2026 22:13

fix: enhance CTE resolution with identifier normalization

ec54989

fix: improve table reference handling with enhanced normalization and…

86476df

… error management

kysshsy force-pushed the fix_18932 branch from 306d336 to 84e11d7 Compare January 11, 2026 16:03

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Jan 11, 2026

kysshsy requested review from alamb and jonahgao January 11, 2026 16:15

jonahgao added 2 commits January 12, 2026 16:05

Fix ci failures

168baba

Merge branch 'main' into fix_18932

8bd9367

jonahgao approved these changes Jan 12, 2026

View reviewed changes

alamb mentioned this pull request Jan 13, 2026

Consolidate datafusion/sqllogictest/test_files/cte_quoted_reference.slt #19786

Closed

alamb added this pull request to the merge queue Jan 13, 2026

Merged via the queue into apache:main with commit 4e1bc79 Jan 13, 2026
28 checks passed

jonahgao mentioned this pull request Mar 19, 2026

Fix CTE reference resolution slt tests #21049

Merged

		@@ -0,0 +1,70 @@
		# Licensed to the Apache Software Foundation (ASF) under one

Conversation

kysshsy commented Dec 28, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

kysshsy commented Jan 3, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahgao Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kysshsy commented Jan 11, 2026

Uh oh!

jonahgao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jan 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahgao Jan 8, 2026 •

edited

Loading