fix(encoding): validate variable-width Arrow offsets - #8382
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The direct variable-width path is protected, but validation must recurse through nested Arrow children before either writer can dispatch work. Applying the same full offset scan to supported packed structs, fixed-size lists, and dictionary values—and covering a nested case through the writer boundary—would close the remaining pre-dispatch validation gap.
| let validation = array_data | ||
| .validate() | ||
| .map_err(|error| error.to_string()) | ||
| .and_then(|_| match array_data.data_type() { |
There was a problem hiding this comment.
The full offset scan is selected only from the root datatype, so malformed variable-width children of supported packed Struct, FixedSizeList, and Dictionary inputs still pass this pre-task boundary. Arrow 58.4's preceding ArrayData::validate() checks only first/last variable-width offsets; it does not prove interior monotonicity. Recursively apply validate_variable_width_offsets to child ArrayData before returning Ok.
Reproducer
I added this test in data.rs on the observed head:
#[test]
fn test_nested_string_offsets_rejected_before_encoding() {
let child_data = unsafe {
ArrayData::builder(DataType::Utf8)
.len(3)
.add_buffer(Buffer::from_slice_ref([0_i32, 2, 1, 3]))
.add_buffer(Buffer::from(b"abc"))
.build_unchecked()
};
let struct_data = unsafe {
ArrayData::builder(DataType::Struct(Fields::from(vec![Field::new(
"child", DataType::Utf8, false,
)])))
.len(3)
.add_child_data(child_data)
.build_unchecked()
};
DataBlock::validate_array_data(&struct_data, "payload", 0).unwrap_err();
}CARGO_HOME=/home/agent/tmp/cargo-pr8382-nested CARGO_TARGET_DIR=/home/agent/tmp/target-pr8382-nested cargo test -p lance-encoding data::tests::test_nested_string_offsets_rejected_before_encoding -- --exact
Expected InvalidInput; observed Ok(()), so unwrap_err() failed.
There was a problem hiding this comment.
Addressed in 1d0f801. Variable-width validation now recursively scans every child ArrayData, so malformed offsets nested in packed structs, fixed-size lists, and dictionary values are rejected synchronously before task dispatch.
|
Addressed in 1d0f801. Recursive validation now covers nested Arrow child layouts, and a writer-boundary regression exercises malformed |
wjones127
left a comment
There was a problem hiding this comment.
Good fix, but need additional test coverage before merging.
| assert_eq!(data.offsets, LanceBuffer::reinterpret_vec(vec![0_i32, 5])); | ||
| assert_eq!(data.data, LanceBuffer::copy_slice(b"world")); | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (blocking): All three new tests only exercise the 32-bit (Utf8/i32) path, but this PR's fix and validation logic are equally applicable to LargeUtf8/LargeBinary (i64) — please parameterize (or duplicate) these tests over both offset widths so the 64-bit byte-scaling math is actually verified; acceptance: a passing test asserting correct slicing and a rejected malformed-offset case for LargeUtf8.
There was a problem hiding this comment.
Addressed in 11a2e49. The offset-slicing and malformed-offset tests now cover both Utf8/i32 and LargeUtf8/i64, and the nested pre-dispatch regression covers both types across the array and structural writers.
| } | ||
|
|
||
| impl DataBlock { | ||
| fn validate_variable_width_offsets<T: ArrowNativeType + Ord>( |
There was a problem hiding this comment.
question (non-blocking): Why hand-roll offset monotonicity/bounds checking here instead of calling ArrayData::validate_full(), which already validates offset ordering and value-buffer bounds for variable-width arrays — is this to avoid the extra cost of validate_full's UTF-8 boundary checks on every flush?
There was a problem hiding this comment.
Addressed in 11a2e49. Yes: validate_full() would additionally rescan UTF-8 contents and character boundaries on every flush. This boundary only needs complete offset monotonicity and bounds validation to make slicing safe, so it reuses the lighter shared scan; that rationale is now documented beside the helper.
| DataType::LargeBinary | DataType::LargeUtf8 => { | ||
| Self::validate_variable_width_offsets::<i64>(array_data)?; | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
thought: This match only covers Binary/Utf8/LargeBinary/LargeUtf8 — Utf8View/BinaryView arrays would skip offset/view-buffer validation entirely; not a regression from this PR but worth a follow-up if those types can reach this path.
There was a problem hiding this comment.
No code change: neither writer field strategy admits Utf8View or BinaryView as primitive field types, so those arrays cannot reach this pre-dispatch validation boundary. Their direct DataBlock conversion remains separate from the writer entry points.
Summary
InvalidInputerror instead of allowing a buffer-slice panicArrayDataoffsets with regression testsRoot cause
The writer trusted Arrow variable-width offset buffers until encoding ran in a spawned task. Negative, non-monotonic, or out-of-bounds offsets could therefore reach unchecked offset stitching and buffer slicing, producing the reported Arrow panic. The conversion also passed the element-based
ArrayData::offset()directly to a byte-based buffer slice.Validation
cargo test -p lance-encoding data::tests::(34 passed)cargo test -p lance-encoding --lib -- --skip test_sparse_large_string_list(554 passed, 5 ignored, 2 filtered)cargo clippy --all --tests --benches -- -D warningscargo fmt --allgit diff --checkThe unfiltered crate run was stopped after the existing
test_sparse_large_string_listminiblock stress case ran for several minutes; the library suite was then rerun with its two parameterized cases filtered as shown above.Fixes #5303