laskoviymishka commented on code in PR #3232:
URL: https://github.com/apache/iceberg-rust/pull/3232#discussion_r4020320674
##########
crates/iceberg/src/delete_vector.rs:
##########
@@ -402,41 +441,122 @@ mod tests {
positions
}
+ // Serializes `dv`, asserts the blob carries well-formed
`deletion-vector-v1` framing, and that
+ // decoding it recovers the same set of positions. This is the round-trip
property:
+ // `deserialize(serialize(dv))` yields the same positions as `dv`.
+ fn assert_roundtrips(dv: &DeleteVector) {
+ let blob = dv.serialize();
+
+ // The magic sits immediately after the 4-byte length prefix.
+ assert_eq!(
+ &blob[DV_LENGTH_PREFIX_BYTES..DV_LENGTH_PREFIX_BYTES +
DV_MAGIC_BYTES],
+ &DV_MAGIC,
+ "magic must follow the length prefix"
+ );
+ // The length prefix counts the magic and vector, i.e. everything but
itself and the CRC.
+ let declared =
u32::from_be_bytes(blob[..DV_LENGTH_PREFIX_BYTES].try_into().unwrap());
+ assert_eq!(
+ declared as usize,
+ blob.len() - DV_LENGTH_PREFIX_BYTES - DV_CRC_BYTES,
+ "length prefix must cover the magic and vector"
+ );
+
+ // Decoding validates the CRC and reconstructs the vector.
+ let decoded = DeleteVector::deserialize(&blob).expect("serialized blob
must decode");
+ assert_eq!(decoded.len(), dv.len());
+ assert_eq!(sorted(&decoded), sorted(dv));
+ }
+
+ #[test]
+ fn test_serialize_roundtrip_empty() {
+ assert_roundtrips(&DeleteVector::default());
+ }
+
+ #[test]
+ fn test_serialize_roundtrip_single_position() {
+ assert_roundtrips(&dv_of([42]));
+ }
+
#[test]
- fn test_deserialize_roundtrip_empty() {
- let blob = encode_dv_blob(&DeleteVector::default());
- assert_eq!(DeleteVector::deserialize(&blob).unwrap().len(), 0);
+ fn test_serialize_roundtrip_small() {
+ assert_roundtrips(&dv_of([0u64, 5, 100, 1000]));
}
+ // Many positions within a single 2^32 container, so the directory holds
exactly one bitmap.
#[test]
- fn test_deserialize_roundtrip_small() {
- let positions = [0u64, 5, 100, 1000];
- let dv =
DeleteVector::deserialize(&encode_dv_blob(&dv_of(positions))).unwrap();
- assert_eq!(sorted(&dv), positions);
+ fn test_serialize_roundtrip_many_in_one_container() {
+ assert_roundtrips(&dv_of((0..5_000).map(|i| i * 3)));
}
+ // Positions spanning multiple 2^32 containers, so the directory holds
several keyed bitmaps
+ // that must be emitted in ascending key order.
#[test]
- fn test_deserialize_roundtrip_spanning_64bit_keys() {
- let positions = [1u64, 1 << 33, (1 << 33) + 5, 1 << 34];
- let dv =
DeleteVector::deserialize(&encode_dv_blob(&dv_of(positions))).unwrap();
- assert_eq!(sorted(&dv), positions);
+ fn test_serialize_roundtrip_spanning_64bit_keys() {
+ assert_roundtrips(&dv_of([1u64, (1 << 32) + 5, (1 << 33) + 9]));
}
// Java run-optimizes every deletion vector before writing, so real blobs
carry RUN
- // containers, which use the SERIAL_COOKIE roaring layout. Force that
layout so decode
+ // containers, which use the SERIAL_COOKIE roaring layout. Force that
layout so the round trip
// exercises the run-container path rather than only array and bitmap
containers.
#[test]
- fn test_deserialize_roundtrip_run_optimized() {
+ fn test_serialize_roundtrip_run_optimized_dense_range() {
let mut dv = dv_of(0..10_000);
assert!(
dv.inner.optimize(),
"expected a dense range to run-length encode"
);
- let decoded = DeleteVector::deserialize(&encode_dv_blob(&dv)).unwrap();
- assert_eq!(decoded.len(), 10_000);
- let positions = sorted(&decoded);
- assert_eq!(positions.first(), Some(&0));
- assert_eq!(positions.last(), Some(&9_999));
+ assert_roundtrips(&dv);
+ }
+
+ // Spells out the `deletion-vector-v1` framing with literal offsets and
magic bytes, guarding
+ // against a drift in the layout that a round trip through our own decoder
could mask.
+ #[test]
+ fn test_serialize_golden_framing() {
+ let blob = dv_of([1u64, 2, 3]).serialize();
+
+ assert_eq!(&blob[4..8], &[0xD1, 0xD3, 0x39, 0x64]);
+ let declared = u32::from_be_bytes(blob[..4].try_into().unwrap());
+ // Everything but the length prefix and the trailing CRC is covered by
`declared`.
+ let framing_overhead = DV_LENGTH_PREFIX_BYTES + DV_CRC_BYTES;
Review Comment:
to make this a bit more self-explanatory, just having 8 means not much, but
this expression has meaning, alternative would be just a comment i guess.
--
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]
