moomindani commented on PR #3631: URL: https://github.com/apache/iceberg-python/pull/3631#issuecomment-5522739824
@hedger9487 thanks for the careful read. One piece of context that shapes my answer: in @kevinjqliu's plan on #3840 (31 August), step 3 is exactly this API — a maintenance operation that rewrites the manifests of the current snapshot so the legacy `equality_ids: list<long>` schema gets re-emitted as `int`. So I want to keep this PR small enough to land, and pull in the piece that plan needs. Of your four, the one I want here is (1) — and I think your (2) is the right way to express it, rather than a second feature. Today `rewrite_manifests()` takes no arguments at all, and `_group_by_target_size` short-circuits single-manifest groups into `kept_manifests`, so "rewrite a manifest that does not need size merging" cannot be requested through the public API. Java says that with `RewriteManifests.rewriteIf(Predicate<ManifestFile>)`, and #3840 wants to target precisely the legacy manifests, so a predicate fits the use case better than an unconditional force would. If your local implementation already has it in that shape, that is the change I would take. (3) bin-packing and concurrency via `ListPacker` / `ExecutorFactory`, and (4) `_validate_files_counts`, I would rather do as follow-ups. Both are worth having, and neither is needed by #3840; this PR has been open since 8 July without a review, and I do not want to grow it while it waits. I will review those promptly once this lands, and I am happy for them to be queued against it in the meantime. A PR against my branch is very welcome — `moomindani:moomindani/rewrite-manifests` — and you will be co-author either way. If you would rather I write the predicate change myself from your description, that works too; tell me which you prefer. @kevinjqliu one thing worth flagging from your #3840 plan: step 3 is this PR. CI is green and it has been open since 8 July with no review, and with the `rewrite_if` predicate above it covers the "users can correct the avro files" step directly. Would you have time to look, or is there someone you would rather route it to? -- 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]
