Repository navigation
Conversation
…alog # Conflicts: # datafusion/core/src/execution/session_state.rs
# Conflicts: # datafusion/execution/src/config.rs
github-actions
Bot
added
sql
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
github-actions
Bot
added
auto detected api change
b5c8c7a to
1b1ed04
Compare
1ae5b90 to
1e5b40e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25887 +/- ##
==========================================
+ Coverage 82.92% 82.93% +0.01%
==========================================
Files 1155 1154 -1
Lines 458197 458155 -42
Branches 458197 458155 -42
==========================================
+ Hits 379968 379985 +17
+ Misses 55196 55140 -56
+ Partials 23033 23030 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@neilconway this branch is now clean. There's still quite a lot that can be improved in the information schema, but I've restricted it to just the scope change here. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @pepijnve -- I am worried about the behavior change in this PR
I thought postgres had some notion of "default search path" or something that would scope its name resolution rules (and presumably also information_schema visibility). Did you consider something like that for DataFusion?
| statement: Statement, | ||
| ) -> datafusion_common::Result<LogicalPlan> { | ||
| let references = self.resolve_table_references(&statement)?; | ||
| let mut references = self.resolve_table_references(&statement)?; |
There was a problem hiding this comment.
this is a nice change to move it here into the session state
There was a problem hiding this comment.
Thanks, that gives me the feeling I'm heading in the right direction.
|
I think you're referring to PostgreSQL's schema search path. I'm not familiar with that, but will look into it. |
|
I spent some time surveying what's out there with AI to get a feel for our options. We'll need to make a decision on a couple of things:
Here's what I got from Claude wrt other some other systems out there
The current state of DataFusion is somewhere in between all the choices listed above: Given that a cross-catalog information schema currently exists, I think we at least need to retain that capability. The question then is how we expose that to the outside world. What I did in this PR is to introduce a new magic catalog named An alternative solution could be to add a search path capability for unqualified identifier resolution. By default, we would not expose the per-catalog information schema and add A hybrid solution where we special case the unqualified resolution of |
|
@alamb sorry to bother you, but if you have a moment could you let me know which direction seems the most promising to you? I'm happy to continue refining this PR, but not entirely sure what an acceptable end goal looks like. |
pepijnve
changed the title
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @pepijnve
My core concern is that while the change proposed in this PR may likely be better / more standards compliant than what currently exists in DataFusion, it is likely to be treated as a major change by some downstream systems (for example InfluxDB would likely be impacted)
I think we need to add some way (e.g. an enum or InformationSchema view / registration API) to keep the existing behavior.
Another thing that might be more elegant maybe would be to update InformationSchemaProvider so that it doesn't require a catalog_list but instead just wraps a single Catalog.
We would still have to find some way to replicate the existing behavior of listing all the tables in all schemas
datafusion/datafusion/core/src/execution/session_state.rs
Lines 413 to 427 in 9551051
🤔
Another idea would be to make some sort of InformationSchema builder or something similar that allowed finer grain control
Perthaps you mean here some way to preserve the existing behavior |
Indeed. What I had in mind was that we would use the 'system catalog' configuration option for this. If it's undefined, then we stick to the current behaviour of every catalog containing being an identical, global one. If it is defined, then we make the per catalog instances so that they only describe objects in the same catalog and the global one provides the cross catalog variant. This way the change becomes opt-in. |
|
@alamb the most recent two commits change things so that the only remaining incompatible change is in the CLI where we'll need to decide what the default behaviour is. Should I revert that back to the current behaviour as well? |
yes I think we should revert the CLI behavior back I also tested again and found a few issues: EXPLAIN SHOW TABLES stopped working:On main: andrewlamb@Andrews-MacBook-Pro-3 ~ % datafusion-cli
DataFusion CLI v55.1.0
> explain show tables;
+---------------+-------------------------------+
| plan_type | plan |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
| | │ StreamingTableExec │ |
| | │ -------------------- │ |
| | │ limit: None │ |
| | └───────────────────────────┘ |
| | |
+---------------+-------------------------------+
1 row(s) fetched.
Elapsed 0.004 seconds.This PR: > EXPLAIN SHOW TABLES;
Error during planning: SHOW TABLES is not supported unless information_schema is enabled
> show datafusion.catalog.information_schema;
+---------------------------------------+-------+
| name | value |
+---------------------------------------+-------+
| datafusion.catalog.information_schema | true |
+---------------------------------------+-------+
1 row(s) fetched.
Elapsed 0.012 seconds.There are a bunch of new
|
Will do
I'll look into that. I'll start by adding SLTs to cover the behaviour.
Would you prefer to split that part off? The information_schema schema itself is included in the information_schema views since they do exist and can describe themselves. The view definition is a bit tricky to actually write though since these are implemented as virtual tables. We can fill in a pseudo definition if that's preferred over the null value, but it won't be SQL you can actually execute. We would have to describe what the underlying definition schema is then. Example from postgresql |
If it is easy I recommend we do, if not it is ok to keep here
Yeah I agree we shouldn't make up some SQL for a view that is dynamically created |
|
FYI, checking what 55.2.0 does reminded me of why I had fixed this. Resolves the inconsistency where querying |
|
I looked up the
Still compliant if we claim that the definition is so long that it would need to be truncated 😄 |
Just removing ~10 lines of code. All done. |
pepijnve
changed the title
| }; | ||
|
|
||
| if require_info_schema { | ||
| if Self::requires_information_schema(&statement) { |
| use datafusion_common::TableReference; | ||
| use sqlparser::ast::*; | ||
|
|
||
| // following constants are used in `resolve_table_references` |
There was a problem hiding this comment.
it is really nice to remove this stuff out of the sql module 💯 as it shouldn't really be in the sql parser
There was a problem hiding this comment.
You're going to like my next PR 😄
|
I had missed the fact that there was a compile error when the |
|
Is there a simple way to run the equivalent of |
| } | ||
|
|
||
| impl InformationSchemaProvider { | ||
| /// Creates a new [`InformationSchemaProvider`] for the provided `catalog_list` |
There was a problem hiding this comment.
| /// Creates a new [`InformationSchemaProvider`] for the provided `catalog_list` | |
| /// Creates a new [`InformationSchemaProvider`] for the provided `catalog` |
| sql_table_name: ObjectName, | ||
| ) -> Result<LogicalPlan> { | ||
| if !self.has_table("information_schema", "tables") { | ||
| let Some(tables_table_ref) = self.resolve_info_table("views") else { |
There was a problem hiding this comment.
nit:
| let Some(tables_table_ref) = self.resolve_info_table("views") else { | |
| let Some(views_table_ref) = self.resolve_info_table("views") else { |
to be clearer what it is about
| | SQLStatement::ShowTables { .. } | ||
| | SQLStatement::ShowColumns { .. } | ||
| | SQLStatement::ShowVariable { .. } | ||
| | SQLStatement::ShowFunctions { .. } |
There was a problem hiding this comment.
ShowStatus, ShowVariables and ShowCollation are no more matched. Intentional ?
There was a problem hiding this comment.
Yes, these don't actually have a backing implementation. Next PR will clean all this up properly.
|
|
||
| if let Some(system_catalog_name) = &self.system_catalog_name { | ||
| for table_name in INFORMATION_SCHEMA_TABLES { | ||
| builder.add_table( |
There was a problem hiding this comment.
according to https://github.com/apache/datafusion/pull/25887/changes#diff-8303a17e3c522b51c73c7fb0474b775746730873fc953e0c52c3fb7e67e84d5bR319 the system catalog could be a normal catalog this will add the same tables second time, no ?
| data_type, | ||
| rid | ||
| FROM | ||
| information_schema.parameters |
There was a problem hiding this comment.
shouldn't this use resolve_info_table("parameters") to resolve the correct catalog+schema+table ?
| data_type, | ||
| rid | ||
| FROM | ||
| information_schema.parameters |
| AND i.rid = o.rid | ||
| GROUP BY 1, 2, o.rid | ||
| ) as p | ||
| JOIN information_schema.routines r |
There was a problem hiding this comment.
resolve_info_table("routines") ?!
| ); | ||
| }; | ||
|
|
||
| let query = format!("SELECT * FROM {tables_table_ref};"); |
There was a problem hiding this comment.
| let query = format!("SELECT * FROM {tables_table_ref};"); | |
| let query = format!("SELECT * FROM {};", tables_table_ref.to_quoted_string()); |
quote that table name to support system catalog names like System
There was a problem hiding this comment.
Is it ok if I handle this via the followup PR that removes the SQL composition altogether?
|
Still some compilation issues when |
Which issue does this PR close?
sqlmodule #26057.Rationale for this change
Catalog-qualified information schema queries currently return metadata for object from every registered catalog.
If we follow the letter of the SQL specification,
my_catalog.information_schema.tablesshould describe onlymy_catalog. This is consistent with PostgreSQL's information schema representing the current database.When working with multiple catalogs, for instance multiple Iceberg REST catalogs, it is more practical to be able to query just the objects defined in one specific catalog without further filtering.
A special 'system' catalog is optionally provided to be able to query across catalogs. This can be seen as an approximation of SQL's definition schema.
What changes are included in this PR?
InformationSchemaProvideris now constructed dynamically based on the catalog being queried. An optional 'system' catalog provides the existing global catalog behaviour. All other catalogs are restricted to just their own content.What is the testing strategy for this PR?
Added multi-catalog SQL logic coverage for qualified and unqualified information schema queries.
Are there any user-facing changes?
Yes. Information schema results are now scoped to the resolved catalog.