jiwen624 commented on PR #18747: URL: https://github.com/apache/iceberg/pull/18747#issuecomment-6648676716
I'm considering closing this implementation in favor of documenting the limitation and a verified workaround (docs PR to follow). #18748 stays open. Here is a summary: **Design.** When every partition spec is partitioned only by equality fields, the writer also emits each equality delete for every older spec it saw at startup, as long as the key column types match. An unpartitioned older spec gets a global delete. Checkpoint state keeps one delete manifest per spec (serializer v3), and a delete that cancels an insert from the same checkpoint stays a position delete. This fixes deletes against rows committed in earlier checkpoints for those tables, but every variant runs into a tension between covering older specs, read cost, and the spec lifecycle: - **Read cost.** For unpartitioned → bucketed evolution, every upsert adds a global equality delete. That adds delete-processing work across all partitions, and as long as those deletes are emitted and live, they keep the old spec referenced. Skipping specs that no longer hold data needs a liveness check in the writer, which loads metadata in every subtask and races with concurrent commits. - **Spec removal.** The writer only knows the specs it saw at startup. If an older spec is removed (for example by `expireSnapshots().cleanExpiredMetadata(true)`), pending deletes point at a missing spec. Skipping them silently loses global deletes. Failing on them, as the current version does, blocks commits. If checkpoint state already contains pending manifests referencing the removed spec, restoring that checkpoint also fails (NPE in `SnapshotSummary` / `ManifestReader`). Resubmitting with fresh writer metadata doesn't repair those pending manifests. - **Remaining gaps.** Any spec in the table's history partitioned on a column outside the equality key turns the fix off, because deleting across such specs over-deletes in CDC mode. In the dynamic sink, a spec change within one checkpoint isn't covered, because equality deletes don't apply to data from the same commit (#18675 coordinates this only within one spec). Cached writer factories with stale specs aren't covered either. - **Size.** About 520 lines, a checkpoint-format change, and memory that grows with the number of older specs times partitions. **Thoughts.** I haven't found a bounded version of this approach whose correctness guarantees, read overhead and recovery behavior justify the added complexity. Resolving keys to row positions and writing position deletes / DVs is another direction worth discussing separately, including its lookup and checkpoint-coordination costs. **Workaround** (verified on main with FlinkSink and Flink `RewriteDataFiles`, for upsert, a bucket-count change and CDC): 1. Stop all writers to the table and make sure their pending writes are committed. 2. Change the spec. 3. Rewrite all data into the new spec (`rewriteAll(true)`). 4. Restart the writers so they pick up the new metadata. This prevents new errors but doesn't repair duplicates or missed deletes already in the table. Upserts and deletes written between the spec change and the rewrite stay incorrect. -- 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]
