jopdorp commented on PR #3564: URL: https://github.com/apache/iceberg-rust/pull/3564#issuecomment-6607212076
@CTTY we put #3585 (`RewriteFilesAction`, continuing #3046) on top of this branch and the producer carries it as is, remove-only rewrites included. Two things the rewrite tests ran into that I think belong in the producer rather than the action: 1. A manifest left with only deleted entries is carried forward on every later commit. `current_manifests` keeps it since #2545, and the filter rewrites the source manifest so the #2148 case can't come back, but nothing drops it afterwards. Java keeps a manifest in `MergingSnapshotProducer.apply` only when it has added or existing files or was written by this commit (MergingSnapshotProducer.java:1057-1062). Could `apply` do the same after `deletes.manifests`? We hit this in pyiceberg, where the same carry-forward made every commit slower over time. 2. `MergingIntent` has no data sequence number for added files, so the entries always inherit the snapshot's. A compaction needs to keep the number that existing equality deletes apply to. `ManifestWriter::add_file(file, seq)` is already there for it, plus Java's check that it is not above the new snapshot's. Fine as a follow-up, just flagging that RewriteFiles needs it. Happy to open either as a PR on top of this once it lands. -- 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]
