-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix CTE reference resolution slt tests #21049
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,9 +98,9 @@ impl TestContext { | |
|
|
||
| let file_name = relative_path.file_name().unwrap().to_str().unwrap(); | ||
| match file_name { | ||
| "cte_quoted_reference.slt" => { | ||
| info!("Registering strict catalog provider for CTE tests"); | ||
| register_strict_orders_catalog(test_ctx.session_ctx()); | ||
| "cte.slt" => { | ||
| info!("Registering strict schema provider for CTE tests"); | ||
| register_strict_schema_provider(test_ctx.session_ctx()); | ||
| } | ||
| "information_schema_table_types.slt" => { | ||
| info!("Registering local temporary table"); | ||
|
|
@@ -178,10 +178,10 @@ impl TestContext { | |
| } | ||
|
|
||
| // ============================================================================== | ||
| // Strict Catalog / Schema Provider (sqllogictest-only) | ||
| // Strict Schema Provider (sqllogictest-only) | ||
| // ============================================================================== | ||
| // | ||
| // The goal of `cte_quoted_reference.slt` is to exercise end-to-end query planning | ||
| // The goal of `StrictOrdersSchema` is to exercise end-to-end query planning | ||
| // while detecting *unexpected* catalog lookups. | ||
| // | ||
| // Specifically, if DataFusion incorrectly treats a CTE reference (e.g. `"barbaz"`) | ||
|
|
@@ -192,26 +192,6 @@ impl TestContext { | |
| // This makes the "extra provider lookup" bug observable in an end-to-end test, | ||
| // rather than being silently ignored by default providers that return `Ok(None)` | ||
| // for unknown tables. | ||
|
|
||
| #[derive(Debug)] | ||
| struct StrictOrdersCatalog { | ||
| schema: Arc<dyn SchemaProvider>, | ||
| } | ||
|
|
||
| impl CatalogProvider for StrictOrdersCatalog { | ||
| fn as_any(&self) -> &dyn Any { | ||
| self | ||
| } | ||
|
|
||
| fn schema_names(&self) -> Vec<String> { | ||
| vec!["public".to_string()] | ||
| } | ||
|
|
||
| fn schema(&self, name: &str) -> Option<Arc<dyn SchemaProvider>> { | ||
| (name == "public").then(|| Arc::clone(&self.schema)) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct StrictOrdersSchema { | ||
| orders: Arc<dyn TableProvider>, | ||
|
|
@@ -245,7 +225,7 @@ impl SchemaProvider for StrictOrdersSchema { | |
| } | ||
| } | ||
|
|
||
| fn register_strict_orders_catalog(ctx: &SessionContext) { | ||
| fn register_strict_schema_provider(ctx: &SessionContext) { | ||
| let schema = Arc::new(Schema::new(vec![Field::new( | ||
| "order_id", | ||
| DataType::Int32, | ||
|
|
@@ -265,13 +245,14 @@ fn register_strict_orders_catalog(ctx: &SessionContext) { | |
| orders: Arc::new(orders), | ||
| }); | ||
|
|
||
| // Override the default "datafusion" catalog for this test file so that any | ||
| // unexpected lookup is caught immediately. | ||
| ctx.register_catalog( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Register a new schema instead of overriding the default datafusion catalog to avoid impacting other CTE tests. |
||
| "datafusion", | ||
| Arc::new(StrictOrdersCatalog { | ||
| schema: schema_provider, | ||
| }), | ||
| let previous = ctx | ||
| .catalog("datafusion") | ||
| .expect("default catalog should exist") | ||
| .register_schema("strict_schema", schema_provider) | ||
| .expect("strict schema registration should succeed"); | ||
| assert!( | ||
| previous.is_none(), | ||
| "strict_schema unexpectedly already existed in datafusion catalog" | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,8 +42,6 @@ physical_plan | |
| statement error DataFusion error: Error during planning: WITH query name "a" specified more than once | ||
| WITH a AS (SELECT 1), a AS (SELECT 2) SELECT * FROM a; | ||
|
|
||
| statement ok | ||
| CREATE TABLE orders AS VALUES (1), (2); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should use the |
||
|
|
||
| ########## | ||
| ## CTE Reference Resolution | ||
|
|
@@ -59,10 +57,14 @@ CREATE TABLE orders AS VALUES (1), (2); | |
| # | ||
| # Refs: https://github.com/apache/datafusion/issues/18932 | ||
| # | ||
| # NOTE: This test relies on a strict catalog/schema provider registered in | ||
| # NOTE: This test relies on a strict schema provider registered in | ||
| # `datafusion/sqllogictest/src/test_context.rs` that provides only the `orders` | ||
| # table and panics on unexpected lookups. | ||
|
|
||
| # Use the strict schema provider | ||
| statement ok | ||
| set datafusion.catalog.default_schema = "strict_schema"; | ||
|
|
||
| statement ok | ||
| set datafusion.sql_parser.enable_ident_normalization = true; | ||
|
|
||
|
|
@@ -99,6 +101,17 @@ with barbaz as (select * from orders) select * from barbaz; | |
| 1 | ||
| 2 | ||
|
|
||
| # Reset to default configs | ||
| statement ok | ||
| set datafusion.sql_parser.enable_ident_normalization = true; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to #19862 (review) |
||
|
|
||
| statement ok | ||
| set datafusion.catalog.default_schema = "public"; | ||
|
|
||
| ## CTE reference resolution tests end ## | ||
|
|
||
|
|
||
|
|
||
| # Test disabling recursive CTE | ||
| statement ok | ||
| set datafusion.execution.enable_recursive_ctes = false; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cte_quoted_reference.slthas been consolidated intocte.slt, and no longer exists.