-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
refactor: change file type logic for create table #7477
Conversation
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 seems like a good change to me. Thank you @tshauck
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.
Make sense to me👍
@@ -79,7 +79,7 @@ LOCATION <literal> | |||
|
|||
`file_type` is one of `CSV`, `PARQUET`, `AVRO` or `JSON` |
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.
I think we need to add Arrow
file_type
, and add a example in doc
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.
Proposed change in #7489
Thanks again @tshauck and @jackwener |
Which issue does this PR close?
Closes #7445
Rationale for this change
The goal of this PR is to make it possible to create external tables using compression that are not in the built-in table factory, but are still valid. I.e. the logic now throws are error if you pass it a file type that's not CSV or JSON with a file compression type that's not uncompressed. Put another way, I have a
TableProviderFactory
with a custom file type that can be gzipped, and I'd like to create a table using the SQL syntax.Another possible change is that this
plan_err
could just be removed, and the logic could be moved into theListingTableFactory
'screate
method. I haven't tested this approach however.What changes are included in this PR?
Are these changes tested?
Add/updated tests to include more cases.
Are there any user-facing changes?
No