Skip to content

Commit 5432766

Browse files
committed
Fix Misaligned buffer on read by reordering OnPair buffers
Root cause: Vortex's flat-layout segment writer aligns each segment to the alignment of its *first* buffer only. With the old buffer order [dict_bytes, dict_offsets, codes, codes_offsets] `dict_bytes` is variable-length and has no alignment requirement, so the segment was written u8-aligned. The next buffer (`dict_offsets`) was a u32 array but ended up at an offset that was only u8-aligned in the file, and on read `PrimitiveArray<u32>::deserialize` rejected it with `Misaligned buffer cannot be used to build PrimitiveArray of u32`. Single-column tests happened to pass because typical OnPair dictionaries are coincidentally a multiple of 4 bytes; ClickBench's wide string tables (and TPC-H's `supplier` post-encoding) hit the bad case. New buffer order: Buffer 0 dict_offsets u32[] ← segment alignment = 4 Buffer 1 codes_offsets u32[] ← length already 4-multiple Buffer 2 codes u16[] ← starts at 4-aligned offset, OK for u16 Buffer 3 dict_bytes u8[] ← variable length, no alignment needed Each buffer's natural length is a multiple of its alignment, so every buffer inside the segment stays correctly aligned. The 16-byte over-copy padding on `dict_bytes` still applies for the decoder. Verified * `cargo test -p vortex-onpair -p vortex-btrblocks -p vortex-file` all green (5 new file-roundtrip tests pass, including a new `odd_dict_length_alignment` test specifically exercising the previously-broken case). * `datafusion-bench tpch --opt scale-factor=0.01 --formats vortex --queries 1,2,3,6 --iterations 1` runs all four queries successfully end-to-end (Parquet → Vortex with OnPair → DataFusion). Signed-off-by: Claude <noreply@anthropic.com>
1 parent d229d6e commit 5432766

2 files changed

Lines changed: 80 additions & 17 deletions

File tree

encodings/onpair/src/array.rs

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -59,16 +59,21 @@ pub const DEFAULT_BITS: u32 = 12;
5959
///
6060
/// On disk the layout is:
6161
///
62-
/// * Buffer 0 — `dict_bytes`: dictionary blob built by the C++ trainer.
63-
/// * Buffer 1 — `dict_offsets`: `dict_size + 1` u32 offsets into `dict_bytes`,
64-
/// stored as raw little-endian bytes.
65-
/// * Buffer 2 — `codes`: per-token `u16` ids, stored as raw little-endian
66-
/// bytes. Each value only uses its low `bits` bits, but we keep the u16
67-
/// width on disk so the decode loop is a straight indexed lookup without
68-
/// bit-unpacking. Downstream compaction can still re-encode this buffer
69-
/// externally.
70-
/// * Buffer 3 — `codes_offsets`: `num_rows + 1` u32 offsets into `codes`,
71-
/// stored as raw little-endian bytes.
62+
/// * Buffer 0 — `dict_offsets`: `dict_size + 1` u32 offsets into `dict_bytes`,
63+
/// stored as raw little-endian bytes. **First so the segment-level
64+
/// alignment is u32 (4 bytes).**
65+
/// * Buffer 1 — `codes_offsets`: `num_rows + 1` u32 offsets into `codes`.
66+
/// Lengths of buffers 0 and 1 are both multiples of 4, so buffer 2 starts
67+
/// at a 4-aligned (and thus 2-aligned) offset within the segment.
68+
/// * Buffer 2 — `codes`: per-token `u16` ids. Each value only uses its low
69+
/// `bits` bits, but we keep the u16 width on disk so the decode loop is
70+
/// a straight indexed lookup without bit-unpacking. Downstream compaction
71+
/// can still re-encode this buffer externally.
72+
/// * Buffer 3 — `dict_bytes`: dictionary blob built by the C++ trainer,
73+
/// padded with [`MAX_TOKEN_SIZE`][crate::MAX_TOKEN_SIZE] trailing zero
74+
/// bytes so the over-copy decoder can safely read 16 bytes past the last
75+
/// token. **Last because its length is variable and it has no alignment
76+
/// requirement; any padding pressure on later buffers is moot.**
7277
/// * Slot 0 — `uncompressed_lengths`: `PrimitiveArray<integer>`.
7378
/// * Slot 1 — optional validity child.
7479
///
@@ -103,10 +108,19 @@ pub(crate) const NUM_SLOTS: usize = 2;
103108
pub(crate) const SLOT_NAMES: [&str; NUM_SLOTS] = ["uncompressed_lengths", "validity"];
104109

105110
/// Buffer indices.
106-
pub(crate) const DICT_BYTES_BUF: usize = 0;
107-
pub(crate) const DICT_OFFSETS_BUF: usize = 1;
111+
///
112+
/// Order matters for on-disk alignment: the Vortex flat-segment writer
113+
/// aligns each segment to the first buffer's alignment only, so we put the
114+
/// strictest-alignment buffers first. Both `u32` offsets buffers have
115+
/// length-multiple-of-4 by construction, and `codes` has
116+
/// length-multiple-of-2; that means every later buffer's relative offset
117+
/// inside the segment stays aligned to its own type's requirement. The
118+
/// variable-length `dict_bytes` blob (no alignment) is last so nothing
119+
/// downstream can be tripped up by its length.
120+
pub(crate) const DICT_OFFSETS_BUF: usize = 0;
121+
pub(crate) const CODES_OFFSETS_BUF: usize = 1;
108122
pub(crate) const CODES_BUF: usize = 2;
109-
pub(crate) const CODES_OFFSETS_BUF: usize = 3;
123+
pub(crate) const DICT_BYTES_BUF: usize = 3;
110124

111125
/// Inner data for an OnPair-encoded array.
112126
///
@@ -388,20 +402,20 @@ impl VTable for OnPair {
388402

389403
fn buffer(array: ArrayView<'_, Self>, idx: usize) -> BufferHandle {
390404
match idx {
391-
DICT_BYTES_BUF => array.dict_bytes_handle().clone(),
392405
DICT_OFFSETS_BUF => array.dict_offsets_handle().clone(),
393-
CODES_BUF => array.codes_handle().clone(),
394406
CODES_OFFSETS_BUF => array.codes_offsets_handle().clone(),
407+
CODES_BUF => array.codes_handle().clone(),
408+
DICT_BYTES_BUF => array.dict_bytes_handle().clone(),
395409
_ => vortex_panic!("OnPairArray buffer index {idx} out of bounds"),
396410
}
397411
}
398412

399413
fn buffer_name(_array: ArrayView<'_, Self>, idx: usize) -> Option<String> {
400414
match idx {
401-
DICT_BYTES_BUF => Some("dict_bytes".to_string()),
402415
DICT_OFFSETS_BUF => Some("dict_offsets".to_string()),
403-
CODES_BUF => Some("codes".to_string()),
404416
CODES_OFFSETS_BUF => Some("codes_offsets".to_string()),
417+
CODES_BUF => Some("codes".to_string()),
418+
DICT_BYTES_BUF => Some("dict_bytes".to_string()),
405419
_ => vortex_panic!("OnPairArray buffer_name index {idx} out of bounds"),
406420
}
407421
}

vortex-file/tests/test_onpair_string_roundtrip.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,55 @@ async fn tpch_supplier_shape() {
301301
assert_eq!(row, n);
302302
}
303303

304+
/// 30 short fixed strings where the dictionary blob length is unlikely to
305+
/// be a multiple of 4. Earlier buffer orderings (dict_bytes first) tripped
306+
/// the segment writer's first-buffer-only alignment, surfacing
307+
/// `Misaligned buffer cannot be used to build PrimitiveArray of u32` on
308+
/// read.
309+
#[tokio::test]
310+
async fn odd_dict_length_alignment() {
311+
let words: &[&str] = &[
312+
"a", "bb", "ccc", "dddd", "eeeee", "fffff", "ggggggg", "h", "ii", "jjj",
313+
];
314+
let n = 20_000usize;
315+
let strings: Vec<&str> = (0..n).map(|i| words[i % words.len()]).collect();
316+
let str_array = VarBinViewArray::from_iter(
317+
strings.iter().map(|s| Some(*s)),
318+
DType::Utf8(Nullability::NonNullable),
319+
)
320+
.into_array();
321+
let data = StructArray::new(
322+
FieldNames::from(["w"]),
323+
vec![str_array],
324+
n,
325+
Validity::NonNullable,
326+
)
327+
.into_array();
328+
329+
let chunks = write_and_read_back(data).await;
330+
let mut row = 0;
331+
for chunk in chunks {
332+
let strct = chunk
333+
.try_downcast::<vortex_array::arrays::Struct>()
334+
.expect("Struct");
335+
let mut ctx = SESSION.create_execution_ctx();
336+
let s = strct
337+
.unmasked_field(0)
338+
.clone()
339+
.execute::<VarBinViewArray>(&mut ctx)
340+
.unwrap();
341+
s.with_iterator(|iter| {
342+
for b in iter {
343+
assert_eq!(b, Some(strings[row].as_bytes()), "row {row}");
344+
row += 1;
345+
}
346+
Ok::<_, vortex_error::VortexError>(())
347+
})
348+
.unwrap();
349+
}
350+
assert_eq!(row, n);
351+
}
352+
304353
/// Mixed-shape strings: empty, short, very long, with a fair chunk of nulls
305354
/// — exercising the validity child + edge offsets.
306355
#[tokio::test]

0 commit comments

Comments
 (0)