paultmathew commented on PR #3387: URL: https://github.com/apache/iceberg-python/pull/3387#issuecomment-4539210702
@abnobdoss Thanks for the review. I agree that this would break things for existing users. I'll update the PR to gate the augmentation on a structural check: augment only when every partition source column is in join_cols (directly or via a deterministic transform on a join column). When that holds, the partition value is a function of the join key, so the augmentation can never exclude a destination file holding the matching key — correctness is preserved by construction. When it doesn't, the row filter is left unchanged and tx.upsert falls back to today's full-table-scan behaviour. The optimisation still covers the workload that motivated the PR — high-cardinality state tables with bucket(N, key) + unique_keys=[key], where partition source ⊆ join_cols by construction. I'll swap the smoke-test scenario to that shape so the benchmarks reflect the case the augmentation is actually safe for. The underlying limitation — "find the row with key=X regardless of which partition holds it" — fundamentally requires either a full-table scan (today's tx.upsert behaviour without augmentation) or a delete-by-value primitive. Equality deletes by primary key are the only Iceberg-native mechanism I see that would let pyiceberg do partition-migration-correct upserts without scanning every file. I know the discussion at #3270 leans toward "don't write equality deletes, write deletion vectors instead", but deletion vectors are still position-keyed — they reduce the rewrite footprint once you've located a row, but they don't help with the lookup itself. So for the partition-migration class of upsert bug specifically, deletion vectors aren't the right tool. @Fokko @kevinjqliu I take the maintainer concerns about equality-delete writes (compaction obligation, read-side merge cost, the rewrite-equality-to-position story) — but I think for the upsert workload the trade-off goes the other way. Happy to take a stab at an equality-delete write path in a separate PR, scoped initially to the cases tx.upsert could opt into, if there's appetite for revisiting that direction. -- 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]
