laskoviymishka commented on PR #866: URL: https://github.com/apache/iceberg-go/pull/866#issuecomment-4405292894
@zeroshade, I rebased onto main, it should be mergeable now. All issues from reviews addressed in the current state: `uint64` typing across `Set` / `Contains` / `count`, `[]byte` parameter on `DeserializeRoaringPositionBitmap`, `slices.Sorted(maps.Keys(...))`, `for i := range count`, plus the `PuffinFile`-only-with-`EntryContentPosDeletes` gate in the manifest builder. Three threads from your review never got formally closed — would love your read on whether these are good as-is: 1. `DVMagicNumber = 0x6439D3D1` — matches Java’s `1681511377` byte-for-byte. `uint32` has no inherent endianness, so the source form is style. Happy to switch to the decimal literal if you want source-level parity with Java. 2. `length` field excluding CRC — documented inline now as “size of magic + bitmap data, excluding CRC-32”, per spec. 3. Puffin reader `Close` — the reader doesn’t own the file IO; the caller passes it in, and `ReadDV` does `defer f.Close()` at the file level. Happy to add a no-op `Close()` on the reader if you’d prefer the symmetry. Context for the nudge: C2 (#996, scanner integration) and C3 (#997, DV writer) are both blocked on this landing. I plan to focus on this next week, would love to start as soon as possible. Once #866 is in, scanner should be a small wiring PR, and the DV writer becomes a real first-non-Java client item for v3. Can we merge as-is? -- 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]
