laskoviymishka commented on issue #997:
URL: https://github.com/apache/iceberg-go/issues/997#issuecomment-4486002848
3-PR split makes sense to me.
Two design calls I agree with:
1. Putting the writer under `table/dv/` keeps the codec, reader, and writer
co-located.
2. Writing one Puffin per `Flush`, with multiple blobs inside it — one blob
per data file — matches Java and avoids file fan-out for wide deletes.
Given that shape, returning `[]iceberg.DataFile` from `Flush` feels natural.
Also, splitting out `SerializeDV` next to `DeserializeDV` is a nice cleanup
beyond the original v3 plan. It makes the codec easier to test in isolation,
and the writer can compose it cleanly.
Two gaps I’d flag, neither blocking PR (1):
1. **In-snapshot DV merging.**
The spec allows at most one DV per data file per snapshot. So if a second
delete targets the same data file in the same commit, we need to read the
existing DV, OR in the new positions, and rewrite it.
The proposed writer produces a fresh DV per `Flush` and doesn’t read
existing state. If two writers target the same data file in one transaction,
#1050 catches this on the scan side later, but the bad write itself could still
commit.
Easiest path for PR (1)/(2) is probably to call this explicitly out of
scope and file a follow-up. Later, we can support it with something like
`DVWriter.AddExisting(blobBytes)`.
2. **v2 position-delete → v3 DV upgrade path.**
If a v3 table still has v2-era position-delete files, and a new DV is
created for the same data file, the spec says those must be merged in the same
snapshot: read the existing positions, OR them into the DV, and drop the old
position-delete file.
Same flavor as (1), and also fine as a follow-up after the basic writer.
I’d just like us to acknowledge it as a known gap so nobody has to rediscover
it later.
One small thing worth checking before finalizing PR (2): see whether Java
has a flush-trigger knob, maybe something like
`write.delete.target-file-size-bytes`. If Java has a soft cap with auto-flush,
parity should be cheap. If not, caller-driven `Flush` is fine for v1.
The cross-client test in PR (3) is the part where I'd put my most attention,
since that’s the spec-binding piece.
Go ahead with PR (1) whenever you’re ready.
--
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]
