jackye1995 commented on pull request #2961: URL: https://github.com/apache/iceberg/pull/2961#issuecomment-897834916
@szehon-ho @RussellSpitzer Thanks for the timely review! Around the documentation, because this will technically be v3 change, we need to add one more column for all tables in the spec doc, I will update that later. For the update snapshot changes like `replaceCurrentSnapshot`, `addStagedSnashot`, I am not updating those in this PR and trying to only consolidate on the changes to the spec. One potential alternative that I would like to just present is to directly have a `Map<String, Long>` snapshot tags map in the `TableMetadata` spec. The advantages are: 1. we do not need to build the index which is beneficial when there are many snapshots in the table. 2. updates to the tag only updates the tag map itself and do not need to rewrite the snapshot information. The disadvantages are: 1. we prefer to update individual components in the spec if possible instead of adding more top-level fields in the table metadata spec. This was the principle we followed when adding row identifier in schema instead of table metadata. 2. not as easy to update the snapshots system table to add the tags field 3. the index is already built anyway for snapshot id mapping, so the marginal cost for indexing tags is not too bad 4. changes like snapshot expiration needs to also update the tags information Based on these considerations I think adding the tags directly in the snapshot spec sounds more reasonable to me. Please let me know if anyone thinks otherwise or has any better approaches. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
