Skip to content
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

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

tshauck
Copy link
Contributor

@tshauck tshauck commented Sep 4, 2023

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 the ListingTableFactory's create method. I haven't tested this approach however.

What changes are included in this PR?

  • Update the if logic to check the exact types that aren't allowed for a more generic "not"
  • Update the tests to check that the new case passes
  • Update the error test table to include ARROW as I think it should've been in the logic.

Are these changes tested?

Add/updated tests to include more cases.

Are there any user-facing changes?

No

@github-actions github-actions bot added the sql SQL Planner label Sep 4, 2023
@tshauck tshauck marked this pull request as ready for review September 4, 2023 23:38
Copy link
Contributor

@alamb alamb left a 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

Copy link
Member

@jackwener jackwener left a 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`
Copy link
Member

@jackwener jackwener Sep 6, 2023

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed change in #7489

@alamb alamb merged commit dfad0af into apache:main Sep 6, 2023
24 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 6, 2023

Thanks again @tshauck and @jackwener

@tshauck tshauck deleted the update-filetype-check branch September 6, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create External Table with Custom File Type and Compression
3 participants