Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Nov 17, 2019

This fixes two cases when assigning UUIDs to tables without them:

  1. If the table is loaded, then assigned a UUID by a concurrent writer
  2. If the table is written with a null UUID because no UUID was assigned

@rdblue
Copy link
Contributor Author

rdblue commented Nov 18, 2019

@jzhuge FYI

@jzhuge
Copy link
Member

jzhuge commented Nov 18, 2019

+1 LGTM

A bit of context:

  • This is to handle an metadata JSON file created before Add table UUID #264. JSON field table-uuid does not exist, thus metadata.uuid() returns null.
  • If you still have pre Add table UUID #264 writer running (not recommended), you might have to disable the checkState temporarily.

@aokolnychyi
Copy link
Contributor

aokolnychyi commented Nov 19, 2019

I think this PR would also resolve the problem when users upgraded to an Iceberg version that started assigning table UUIDs but their first operation wasn't a child of SnapshotProducer (e.g. update table properties). Such operations would write an explicit null as the table UUID that will corrupt the old logic.

@aokolnychyi
Copy link
Contributor

@rdblue, do we want to bring this in? There is only one open question on HadoopTableOperations.

@rdblue
Copy link
Contributor Author

rdblue commented Jan 14, 2020

Yes, I just need to update it after your review. Sorry I'm taking so long getting back to it.

@rdblue rdblue added this to the Java 0.8.0 Release milestone Feb 16, 2020
@rdblue rdblue force-pushed the fix-uuid-assignment branch from 355d9c0 to 4576866 Compare February 17, 2020 22:54
@rdblue
Copy link
Contributor Author

rdblue commented Feb 17, 2020

@aokolnychyi, I updated HadoopTableOperations. Can you take another look at this?

Copy link
Contributor

@aokolnychyi aokolnychyi left a comment

Choose a reason for hiding this comment

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

+1

There is a relevant checkstyle violation. @rdblue, could you fix it and merge this PR?

@rdblue rdblue merged commit bd4f236 into apache:master Feb 18, 2020
@rdblue
Copy link
Contributor Author

rdblue commented Feb 18, 2020

Merged. Thanks for the review, @aokolnychyi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants