rdblue commented on PR #16823: URL: https://github.com/apache/iceberg/pull/16823#issuecomment-4792873193
I agree that setting `dv_snapshot_id` is duplicative for these updates. We can still detect changes by checking if either `deleted_positions` or `replaced_positions` is set. I don't think this itself is a strong justification. There is still an argument for setting `dv_snapshot_id`: it would explicit state the snapshot when the `deleted_positions` and `replaced_positions` changes occurred. That has value because we could decide to carry forward what changed at that time and not have a hard requirement on dropping the bitmaps immediately, which are likely to be small. That also means that the format is insulated from implementations that accidentally keep the bitmaps around, although that is unlikely. In this case, the spec would state that the bitmaps reflect the changes in the snapshot identified by `dv_snapshot_id` for manifest entries. The argument against using `dv_snapshot_id` is that it is simpler: do not set this unless the (data) DV changed. But is this more clear? Another way of looking at this is that `dv_snapshot_id` is the snapshot when the tracked file's DV changed. If that tracked file is a manifest and the DV is in `ManifestInfo`, why is that different? After thinking through it, I'm not convinced that this is a good idea. I think I'd prefer keeping the update to `dv_snapshot_id` and make it required in the spec. -- 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]
