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.
| schema: schema.into(), | ||
| table: table.into(), | ||
| fn resolve_info_table(&self, table: &str) -> Option<TableReference> { | ||
| let table_reference = if let Some(system_catalog) = |
There was a problem hiding this comment.
maybe we can adjust the comments here to explain the scoping rules:
Looks first in the system catalog, if defined, and otherwise treats it as unqualified reference
There was a problem hiding this comment.
See other comment. Might be best to peek ahead at PR #26057 where this goes away entirely. In a nutshell, what I've done there is to change the planning of show ... statements so that they're not dependent on information_schema at all.
| { | ||
| TableReference::Full { | ||
| catalog: system_catalog.deref().into(), | ||
| schema: "information_schema".into(), |
There was a problem hiding this comment.
This information_schema reference feels wrong to me -- I thought the design was that the core of Datafusion / the sql parser didn't have any special case for the information_schema and information schema was handled with the same API as the other catalog providers
There was a problem hiding this comment.
In this PR and more importantly in main, the references to information_schema, the various tables and their columns all exist here. As an example, see the show_tables_to_plan function on main.
I agree with you that the hardcoding of the information_schema table schema in the sql crate feels wrong. Getting rid of that is something I'm trying in followup PR #26057. This one is smaller in scope and only tries to land the work done in PR #24200 which seems to have stalled.
| WHERE table_schema <> 'information_schema'; | ||
| ---- | ||
| my_other_catalog my_other_schema t3 | ||
|
|
||
| query TTTT rowsort | ||
| SELECT * from information_schema.tables; |
There was a problem hiding this comment.
why don't all the other tables appear now? That seems ilke people would treat it as a regression 🤔
There was a problem hiding this comment.
What I've tried to implement in this PR is what's described at https://docs.databricks.com/aws/en/sql/language-manual/sql-ref-information-schema
In the SYSTEM catalog, the INFORMATION_SCHEMA is a SQL standard schema that provides metadata about objects across all catalogs in the metastore. It does not contain metadata about hive_metastore objects.
Separately, each catalog created in Unity Catalog also automatically includes an information_schema that describes metadata about objects in that catalog only.
The behaviour you're seeing here is that the default catalog was set to my_other_catalog. information_schema.tables then resolves to my_other_catalog.information_schema.tables which only describes the objects in my_other_catalog.
There was a problem hiding this comment.
I was curious about how this was theoretically supposed to work per the SQL specification. In ISO 9075-11:2023 at least, the information schema views are indeed segregated per catalog. There's a theoretical notion of a 'definition schema' containing base tables which are allowed to contain everything, and then the information schema view are defined in terms of those.
First we have
CREATE VIEW INFORMATION_SCHEMA_CATALOG_NAME AS
SELECT CATALOG_NAME
FROM DEFINITION_SCHEMA.CATALOG_NAME
WHERE CATALOG_NAME = 'CN';
where CN is the name of the catalog in which this Information Schema resides.
The tables view is defined as (shortened for legibility)
CREATE VIEW TABLES AS
SELECT TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, TABLE_TYPE, ...
FROM DEFINITION_SCHEMA.TABLES
WHERE ...
AND
TABLE_CATALOG
= ( SELECT CATALOG_NAME
FROM INFORMATION_SCHEMA_CATALOG_NAME );
The last predicate is what restricts the set of tables to only those that exist in the same catalog as the information schema itself. I did not check all of them exhaustively, but the same seems to hold for most of the other object views as well.
This is of course just the standards view on things. Logging this here as FYI.
|
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
|
Sorry -- this PR is on my list to review / respond to mroe carefully. I will try and do so soon |
|
@neilconway since you were reviewing the original version of this PR, I was wondering if you had any opinions on the global vs segregated information schema question above. |
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? |
4 participants
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.