-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Improvements to list_files_cache table function
#19703
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?
Conversation
|
|
||
| let schema = Arc::new(Schema::new(vec![ | ||
| Field::new("table", DataType::Utf8, false), | ||
| Field::new("table", DataType::Utf8, true), |
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.
This uses NULL (rather than "NULL") in list_files_cache
| Pattern::new(glob).map_err(|e| DataFusionError::External(Box::new(e)))?; | ||
| Self::try_new(self.url, Some(glob)) | ||
| pub fn with_glob(mut self, glob: &str) -> Result<Self> { | ||
| self.glob = |
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.
There is no need to call the constructor again as self.path hasn't changed and the constructor doesn't do anything with glob
datafusion/datafusion/datasource/src/url.rs
Lines 149 to 157 in 8a9bf43
| pub fn try_new(url: Url, glob: Option<Pattern>) -> Result<Self> { | |
| let prefix = Path::from_url_path(url.path())?; | |
| Ok(Self { | |
| url, | |
| prefix, | |
| glob, | |
| table_ref: None, | |
| }) | |
| } |
This then simplifies the code in datafusion/core/src/datasource/listing_table_factory.rs
| table_path = table_path | ||
| .with_glob(glob.as_ref())? | ||
| .with_table_ref(cmd.name.clone()); | ||
| table_path = table_path.with_glob(glob.as_ref())?; |
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.
This can be simplified due to the change in with_glob
comphead
left a comment
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.
Thanks @alamb it is lgtm!
|
thanks a lot! |
Which issue does this PR close?
Rationale for this change
I had a few minor comments / suggestions while reviewing #19616 from @jizezhang but they weren't needed to do the initial merge, so I would like to propose them in a follow up PR
What changes are included in this PR?
table_refin ListingTableURL"NULL"inlist_files_cachetable functionI can break this into separate PRs if that would help
Are these changes tested?
Yes by CI
Are there any user-facing changes?
The
list_files_cachefunction now might return null