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
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 |
|
|
||
| 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 ?
There was a problem hiding this comment.
I've added an extra guard to avoid duplicates. I think I might reconsider this 'system catalog' idea in the followup where all the SQL stuff disappears. I can get away with a simpler global/partitioned toggle there.
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?
There was a problem hiding this comment.
I've polished #26057 and I think it's now in good enough shape for review. Leaving it marked as draft for now since it builds on the work done here and GitHub can't do cross fork stacked PRs yet.
|
Still some compilation issues when |
|
@alamb I think we're good to go now. I ran |
|
@alamb while working through @martin-g's review comments I came to the realisation that in the followup #26057 PR we can drop the 'system catalog' concept entirely. That was only really necessary to support a partitioned information schema at the table level, but still allow the I've gone ahead and removed the concept in #26057. This PR is then an intermediate step. I'm not sure what the best approach is here. On the one hand I think it would be easiest from a review point-of-view to merge this work already, and then make sure that the other PR is merged before the next release ships. On the other hand, that does put some timing pressure on the review. If we want to avoid that it might be better to consolidate the two PRs and merge the work in one go. Wdyt? |
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.