laskoviymishka commented on PR #18003: URL: https://github.com/apache/iceberg/pull/18003#issuecomment-6400983264
I dug into the v3 part a bit more after the review, and I think I was too strict there. There is a good reason to support this on v3 too, not only v4. Ryan’s point in the thread behind #17783 is that DV-based writes on v3 give writers a clean upgrade path before the table itself moves to v4 ([[thread](https://lists.apache.org/thread/wfd3s65dpyzto8g8poffcg1tcqpgk5k7)](https://lists.apache.org/thread/wfd3s65dpyzto8g8poffcg1tcqpgk5k7)). Flink’s converter also targets v3. v4 can come later once the converter writes [[colocated DVs](https://github.com/apache/iceberg/blob/3c69c30b244e320c08ba32362ee0c03ef968fe6f/format/spec.md?plain=1#L1572)](https://github.com/apache/iceberg/blob/3c69c30b244e320c08ba32362ee0c03ef968fe6f/format/spec.md?plain=1#L1572). The cost is on the write side. An equality-delete writer just writes keys. Here we have to read key columns from candidate files before commit. But after that, readers only need the DV instead of doing the equality-delete join on every read, and clients without equality-delete support can still read the table. For Kafka Connect I think that is probably the right trade. The cases I’d want to understand better in the dev@ thread are mostly the bad key distributions: - random keys don’t prune well by bounds, so a commit can end up reading key columns from most files in the touched partitions - with bucketed tables, touching many buckets can also mean planning the same manifests several times because planning currently runs once per partition group ([[here](https://github.com/apache/iceberg/blob/286bc46e511948e9320c8535e465d5a728130df4/kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/EqualityDeleteConverter.java#L168-L176)](https://github.com/apache/iceberg/blob/286bc46e511948e9320c8535e465d5a728130df4/kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/EqualityDeleteConverter.java#L168-L176)) - all of this runs on the coordinator, and a conflict means doing it again For part 2, I’d benchmark conversion time vs commit interval, plus bytes/manifests read. Recent/scattered/random keys are probably enough to show where this starts getting expensive. I’d also compare that with read latency after accumulating equality deletes for a while. Bloom filters on the key columns are worth trying too. For this part, I think I only need the concurrent in-key append test on a non-null base, plus a short benchmark plan in the description. The compaction case can wait for part 2 when there is a real `RowDelta`. The >1,000-key fallback is what I asked for in round 1, so I’m fine with it here. `existingDeletes()` also doesn’t block this part. -- 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]
