-
Notifications
You must be signed in to change notification settings - Fork 139
feat: reuse schemas & increment their IDS #495
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
feat: reuse schemas & increment their IDS #495
Conversation
9a16a1c to
bad1740
Compare
zeroshade
left a comment
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.
In general, looks good. just some nitpicks
132f50c to
2e68dd9
Compare
2e68dd9 to
577cd2f
Compare
zeroshade
left a comment
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.
Just a single nitpick
zeroshade
left a comment
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! Looks good
Aligns us a bit more with the Java & Rust implementations where functionally equivalent schemas don't get added a second time. It also aligns renumbering of schemas to be equivalent to java/rust.
Java source: https://github.com/apache/iceberg/blob/41c0b17a20c522e4df519bcc429f413e6a2855e5/core/src/main/java/org/apache/iceberg/TableMetadata.java#L117-L138
Rust source: https://github.com/apache/iceberg-rust/blob/c4c006939a3cb3bf684a0d10ba0e66cac484befe/crates/iceberg/src/spec/table_metadata_builder.rs#L1068