Skip to content

Commit 1c7229d

Browse files
committed
less
Signed-off-by: Robert Kruszewski <github@robertk.io>
1 parent d948bfc commit 1c7229d

2 files changed

Lines changed: 18 additions & 26 deletions

File tree

‎encodings/zstd/src/array.rs‎

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ where
347347
let (values, num_bytes) = slice.value_bytes()?;
348348
// Each value is length-prefixed, so the frames can only be walked in order — which is the
349349
// order `append_valid_slices` visits the valid rows in. A walk that runs out early hands back
350-
// its undecoded remainder, which the builder rejects as a byte-count mismatch.
350+
// its remainder, which the builder rejects as a byte-count mismatch.
351351
let mut values = ZstdValues::new(values);
352352
builder.append_valid_slices(num_bytes, mask, |_| values.next_value())
353353
}
@@ -804,12 +804,11 @@ impl DecompressedSlice {
804804
/// slice's values.
805805
///
806806
/// Walking the length prefixes is a dependent load chain, so both ends are derived from the
807-
/// slice metadata where possible: an unsliced array skips both walks. That makes the far end a
808-
/// claim by the frame metadata rather than a checked fact, so a caller must still hold the
809-
/// values it reads to the count [`Self::value_range`] gives — by walking them with
810-
/// [`ZstdValues`], whose shortfall shows up in the byte total from [`Self::value_bytes`], or by
811-
/// counting the ones it decodes, as [`try_reconstruct_views`] does. A region that ends part way
812-
/// through a value is otherwise free to pass its trailing bytes off as values of their own.
807+
/// slice metadata where possible: an unsliced array skips both walks. The far end is then a
808+
/// claim rather than a checked fact, so a caller must hold the values it reads to the count
809+
/// from [`Self::value_range`] — with [`ZstdValues`], whose shortfall shows up in the byte total
810+
/// from [`Self::value_bytes`], or by counting decoded values as [`try_reconstruct_views`] does.
811+
/// Otherwise a region ending part way through a value passes its trailing bytes off as values.
813812
fn value_byte_range(&self) -> VortexResult<Range<usize>> {
814813
let Range { start, end } = self.value_range()?;
815814
let buffer = self.bytes.as_slice();
@@ -903,13 +902,11 @@ impl<'a> ZstdValues<'a> {
903902

904903
/// The next value, or every byte the walk could not decode once a prefix leaves the region.
905904
///
906-
/// Handing back the remainder is what lets a caller that knows how many values the region holds
907-
/// detect a walk that fell short purely from the byte total, without a second walk to validate
908-
/// the region up front. A caller sizes that total as the region minus one length prefix per
909-
/// value, so `k` of `n` values decoded leaves it expecting the `n - k` prefixes the walk
910-
/// abandoned as well: the remainder either covers them and more, or is empty because the region
911-
/// ended exactly and the decoded values already fall short of the total. Neither can add up, so
912-
/// a shortfall is always rejected rather than passed off as trailing empty values.
905+
/// The remainder is what makes a shortfall visible in the byte total alone, without a second
906+
/// walk to validate the region up front. A caller sizes that total as the region minus one
907+
/// prefix per value, so `k` of `n` values decoded leaves it still expecting the `n - k`
908+
/// prefixes the walk abandoned; the remainder covers those and more, or is empty only because
909+
/// the region ended exactly and the decoded bytes already fall short. Neither can add up.
913910
fn next_value(&mut self) -> &'a [u8] {
914911
let value_start = self.offset + size_of::<ViewLen>();
915912
let value = zstd_value_len(self.buffer, self.offset)
@@ -1783,17 +1780,14 @@ mod tests {
17831780
Ok(())
17841781
}
17851782

1786-
/// Both ends of the region a slice reads come from the frame metadata where they can, so the
1787-
/// values in it have to be held to the value count the metadata declared. Otherwise a buffer
1788-
/// whose last value is only a length prefix reads back as a trailing empty value.
17891783
#[test]
17901784
fn test_append_to_varbin_rejects_a_dangling_length_prefix() {
17911785
let mut buffer = Vec::new();
17921786
buffer.extend_from_slice(&3u32.to_le_bytes());
17931787
buffer.extend_from_slice(b"cat");
1794-
// A prefix with no value after it. It takes up exactly the four bytes the second value's
1795-
// own prefix would have, so treating that value as empty would still total the byte count
1796-
// the metadata implies and append ["cat", ""].
1788+
// A prefix with no value after it. It takes up exactly the four bytes the missing value's
1789+
// own prefix would have, so treating that value as empty still totals the byte count the
1790+
// metadata implies and appends ["cat", ""].
17971791
buffer.extend_from_slice(&1u32.to_le_bytes());
17981792

17991793
let slice = decompressed_slice(ByteBuffer::copy_from(buffer.as_slice()), 0, 2, 0, 2);

‎encodings/zstd/src/test.rs‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -383,18 +383,16 @@ fn test_zstd_rejects_corrupt_frame_metadata(
383383
Ok(())
384384
}
385385

386-
/// Frame bytes are as untrusted as the metadata describing them, so a frame whose last value is
387-
/// nothing but a length prefix has to surface as an error from every read path rather than as a
388-
/// trailing empty value.
386+
/// Frame bytes are as untrusted as the metadata describing them: a frame whose last value is
387+
/// nothing but a length prefix has to error from every read path, not read back as an empty value.
389388
#[test]
390389
fn test_zstd_rejects_a_frame_ending_in_a_dangling_length_prefix() -> VortexResult<()> {
391390
let mut ctx = array_session().create_execution_ctx();
392391
let mut values = Vec::new();
393392
values.extend_from_slice(&3u32.to_le_bytes());
394393
values.extend_from_slice(b"cat");
395-
// A prefix with no value after it. It takes up exactly the four bytes the second value's own
396-
// prefix would have, so treating that value as empty totals the byte count the metadata
397-
// implies: two values holding three bytes between them.
394+
// A prefix with no value after it. Treating the missing value as empty still totals the three
395+
// bytes the metadata implies for two values.
398396
values.extend_from_slice(&1u32.to_le_bytes());
399397

400398
let dtype = DType::Utf8(Nullability::NonNullable);

0 commit comments

Comments
 (0)