viirya commented on PR #2631: URL: https://github.com/apache/iceberg-rust/pull/2631#issuecomment-4703217734
Thanks @kevinjqliu — I looked into whether the Rust side can mirror Java's split here, and I think this PR is actually validating at the same layer Java does, just with less ceremony. The key difference: Rust has no equivalent of Java's raw `PositionDelete` row type. `parse_positional_deletes_record_batch_stream` has a single caller and its output type *is* `HashMap<String, DeleteVector>` — i.e. it goes straight from Arrow rows to the roaring bitmap. That makes this function the analogue of Java's `Deletes.toPositionIndexes` → `RoaringPositionBitmap.set`, which is exactly where Java *does* validate `pos >= 0`. There's no "raw read" path in Rust today that this validation would shadow — the only path that exists is the apply path. So a malformed delete file isn't being made "impossible to inspect" by a path that previously could; no such inspect/repair path exists yet. Building one would mean adding a separate reader that yields raw signed `i64` rows instead of a `DeleteVector`, which I'd argue is a distinct feature rather than something this wraparound fix should carry. That said, two of your suggestions are worth folding in regardless: - Use `u64::try_from(pos)` instead of `pos as u64` so the conversion itself is checked, rather than relying on the separate `pos < 0` guard. (Same end result, but the cast can't silently wrap if the guard is ever moved/removed.) - When a raw read/repair path is eventually added, that's the place to preserve the signed value — worth a tracking issue if there's appetite for it. -- 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]
