Skip to content

Conversation

@bryanck
Copy link
Contributor

@bryanck bryanck commented Jun 20, 2022

This PR adds the table format version in the list of metadata updates when starting a create table transaction with the REST catalog client.

@github-actions github-actions bot added the core label Jun 20, 2022
Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

This is great. Thank you @bryanck!

Left one comment.

We should add a test as well, ideally / likely within CatalogTests, that when a V2 table is created we get a V2 table on load.

private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();

changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be set only if the format version is not the default?

Copy link
Contributor Author

@bryanck bryanck Jun 20, 2022

Choose a reason for hiding this comment

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

I think the server-side default and the client-side default may not match? The issue I originally encountered was the Trino plugin had v2 as the default and set the initial sequence number to 1, but the server didn't receive a format version so treated it as v1, and failed because the sequence number was > 0. If the client and server are on different Iceberg versions, there could potentially be a mismatch in the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I see what you mean.

Copy link
Contributor

@kbendick kbendick Jun 20, 2022

Choose a reason for hiding this comment

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

+1.

I could have sworn I left an almost identical comment when I reviewed this before (the single comment I was referring to). I can possibly help with adding a unit test for this sequence in CatalogTests if we’d like. Feel free to reach out on Slack too.

I too see what you mean.

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thank you @bryanck!

@bryanck
Copy link
Contributor Author

bryanck commented Jun 20, 2022

I added a test for creating a V2 table via a create transaction, which fails without the change.

@rdblue rdblue merged commit 4959e54 into apache:master Jun 21, 2022
namrathamyske pushed a commit to namrathamyske/iceberg that referenced this pull request Jul 10, 2022
namrathamyske pushed a commit to namrathamyske/iceberg that referenced this pull request Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants