anoopj commented on code in PR #3232:
URL: https://github.com/apache/iceberg-rust/pull/3232#discussion_r4031925837
##########
crates/iceberg/src/delete_vector.rs:
##########
@@ -309,15 +352,17 @@ impl BitOrAssign for DeleteVector {
}
}
-// Reproduces Iceberg-Java's `deletion-vector-v1` framing so tests can
round-trip through
-// `deserialize` without a Java writer, and so other test modules can build
blob fixtures.
-// Cross-implementation golden fixtures produced by Iceberg-Java are tracked
separately; this
-// only checks that our decode matches our encode.
-#[cfg(test)]
-pub(crate) fn frame_dv_blob(vector: &[u8]) -> Vec<u8> {
+// The single implementation of the `deletion-vector-v1` framing: it prepends
the big-endian
+// length prefix and magic, appends the big-endian CRC-32 over the magic and
vector, and is shared
+// by `serialize` and by the tests that craft raw roaring directories
`serialize` can never emit.
+// Only reachable through `serialize` (dead in non-test builds) and the tests,
hence `allow(unused)`.
+#[allow(unused)]
+fn frame_dv_blob(vector: &[u8]) -> Vec<u8> {
let body_len = DV_MAGIC_BYTES + vector.len();
let mut blob = Vec::with_capacity(DV_LENGTH_PREFIX_BYTES + body_len +
DV_CRC_BYTES);
- blob.extend_from_slice(&(body_len as u32).to_be_bytes());
+ let body_len =
+ u32::try_from(body_len).expect("deletion-vector-v1 body length exceeds
u32::MAX");
Review Comment:
That is a good catch. But should we do a panic here? why not return an
error? I agree it is highly unlikely to get to this though, but it would be
good to avoid panics.
--
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]
