Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support CreateTableTransaction in Glue and Rest #498
Support CreateTableTransaction in Glue and Rest #498
Changes from 23 commits
998c6f1
8eace7c
64e6346
ffb8ff6
3a579cd
049e0e2
df0c5ed
755aebf
c98b3b4
09b60ca
04ef8df
211de32
d57ac1c
978a0aa
a413c2e
ad840d5
47ce986
1f5cc28
9ac2f7f
d2617fb
44df2d7
1a4d262
f7c04cf
cecf1c0
2152542
c449fb0
8fc1562
b99c619
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 recommend creating a V2 table by default. This is also the case for Java. Partition evolution is very awkward for V1 tables (keeping the old partitions as null-transforms).
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.
The default is still V2 table. When creating a createTableTransaction, we first call
new_table_metadata
which by default gives a V2 metadata. Then, we parse the initial metadata to a sequence ofTableUpdates
, including anUpgradeFormatVersionUpdate
. Finally when we re-build the metadata in_commit_table
, theUpgradeFormatVersionUpdate
will bump the metadata to the correct version.If we use V2Metadata here, we won't be able to create any V1 table since we cannot downgrade from V2 to V1. I think this is the same issue in
iceberg-rest
that prevents us from creating V1 table via create-table-transaction.#498 (comment) I should follow-up that later.I added some comments to explain the purpose and also make it private.
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 for the detailed explanation, that makes sense to me 👍
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.
The inheritance feels off here. I would just expect to
create_table_transaction
. The current_create_staged_table
on theCatalog
is now very specific for non-REST catalogs. Should we introduce another layer in the OOP hierarchy there? Then we can overridecreate_table_transaction
there.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! This is a great suggestion! After going through the current code, I find that not only
_create_staged_table
but also many other helper functions inCatalog
are specific to non-Rest Catalogs. Hence, I add a new class namedMetastoreCatalog
(appreciate any suggestions on naming) and make all non-Rest Catalogs inherit from it instead.Since it is a big refactoring, please let me know if you want this to happen in a follow-up PR.