From d5d05bb49a77429c9175c7409d886765cea97e75 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Wed, 28 Oct 2026 17:02:47 -0400 Subject: [PATCH 1/2] Panic on out-of-bounds Interleave selectors `Interleave` cannot be serialized, so its selectors always come from code in the same process. A selector that names a missing value or row is a bug in that code, not bad input. The bool and fixed-width kernels now panic on it, which matches `Interleave::scalar_at` and the fixed-width take kernel that `Dict` execution uses. Signed-off-by: Connor Tsui --- .../src/arrays/interleave/execute/bool.rs | 40 ++++++----- .../src/arrays/interleave/execute/decimal.rs | 2 +- .../arrays/interleave/execute/fixed_width.rs | 68 +++++++++++-------- .../arrays/interleave/execute/primitive.rs | 4 +- vortex-array/src/arrays/interleave/mod.rs | 35 ++++------ 5 files changed, 77 insertions(+), 72 deletions(-) diff --git a/vortex-array/src/arrays/interleave/execute/bool.rs b/vortex-array/src/arrays/interleave/execute/bool.rs index e6f1aa5cbe3..05667261f36 100644 --- a/vortex-array/src/arrays/interleave/execute/bool.rs +++ b/vortex-array/src/arrays/interleave/execute/bool.rs @@ -8,8 +8,6 @@ use vortex_buffer::BitBuffer; use vortex_buffer::BitBufferMut; use vortex_buffer::BufferAllocatorRef; use vortex_error::VortexResult; -use vortex_error::vortex_ensure; -use vortex_error::vortex_ensure_eq; use super::super::Interleave; use super::super::InterleaveArrayExt; @@ -59,7 +57,7 @@ pub(super) fn execute( array_indices.as_slice::(), row_indices.as_slice::(), ctx.allocator(), - )? + ) }) }); @@ -76,27 +74,32 @@ fn gather, R: AsPrimitive>( branches: &[A], rows: &[R], allocator: &BufferAllocatorRef, -) -> VortexResult { - let len = validate_selectors(value_bits, branches, rows)?; +) -> BitBufferMut { + let len = validate_selectors(value_bits, branches, rows); // SAFETY: `validate_selectors` proved `branches.len() == rows.len() == len`, and for every // `i < len` that `branches[i] < value_bits.len()` and `rows[i] < value_bits[branches[i]].len()`. - Ok(unsafe { gather_bits(len, value_bits, branches, rows, allocator) }) + unsafe { gather_bits(len, value_bits, branches, rows, allocator) } } /// Validates the per-row selector bounds, returning the output length (`branches.len()`). /// -/// On success, `rows.len() == branches.len() == len` and, for every `i < len`, -/// `branches[i] < value_bits.len()` and `rows[i] < value_bits[branches[i]].len()` — exactly the -/// preconditions of [`gather_bits`]. Errors (rather than panics) on any out-of-bounds selector. +/// On return, `rows.len() == branches.len() == len` and, for every `i < len`, +/// `branches[i] < value_bits.len()` and `rows[i] < value_bits[branches[i]].len()`. These are +/// exactly the preconditions of [`gather_bits`]. +/// +/// # Panics +/// +/// Panics if the selectors have different lengths, or if a selector names a value or a row that +/// does not exist. fn validate_selectors, R: AsPrimitive>( value_bits: &[BitBuffer], branches: &[A], rows: &[R], -) -> VortexResult { +) -> usize { // The two selectors are validated to equal length at construction, which is the output length. let len = branches.len(); - vortex_ensure_eq!( + assert_eq!( rows.len(), len, "interleave row_indices length does not match array_indices length", @@ -104,17 +107,20 @@ fn validate_selectors, R: AsPrimitive>( for i in 0..len { let branch = branches[i].as_(); - vortex_ensure!( + assert!( branch < value_bits.len(), - "interleave array index out of bounds" + "interleave array index {branch} out of bounds for {} values", + value_bits.len() ); - vortex_ensure!( - rows[i].as_() < value_bits[branch].len(), - "interleave row index out of bounds" + let row = rows[i].as_(); + assert!( + row < value_bits[branch].len(), + "interleave row index {row} out of bounds for length {}", + value_bits[branch].len() ); } - Ok(len) + len } /// Gathers one bit per output from `bits[branches[i]]` at position `rows[i]`, packing 64 results per diff --git a/vortex-array/src/arrays/interleave/execute/decimal.rs b/vortex-array/src/arrays/interleave/execute/decimal.rs index 793b17e3cb4..55e4f5ce438 100644 --- a/vortex-array/src/arrays/interleave/execute/decimal.rs +++ b/vortex-array/src/arrays/interleave/execute/decimal.rs @@ -61,7 +61,7 @@ fn gather_values( .map(|i| prepare_source::(array.value(i), dtype, ctx)) .collect::>>()?; - gather(array, &sources, ctx.allocator()) + Ok(gather(array, &sources, ctx.allocator())) } /// Normalizes one decimal source while retaining a single payload for a constant. diff --git a/vortex-array/src/arrays/interleave/execute/fixed_width.rs b/vortex-array/src/arrays/interleave/execute/fixed_width.rs index dfd59fd87c7..5bb8c0adab1 100644 --- a/vortex-array/src/arrays/interleave/execute/fixed_width.rs +++ b/vortex-array/src/arrays/interleave/execute/fixed_width.rs @@ -11,10 +11,7 @@ use num_traits::AsPrimitive; use vortex_buffer::Buffer; use vortex_buffer::BufferAllocatorRef; use vortex_buffer::BufferMut; -use vortex_error::VortexResult; -use vortex_error::vortex_bail; -use vortex_error::vortex_ensure; -use vortex_error::vortex_ensure_eq; +use vortex_error::vortex_panic; use super::super::Interleave; use super::super::InterleaveArrayExt; @@ -61,15 +58,15 @@ impl Source { /// `sources` holds one [`Source`] for each value of `array`, in order. Both selectors of `array` /// **must** be [`Primitive`] arrays, which the kernels ensure with `require_child!`. /// -/// # Errors +/// # Panics /// -/// Returns an error if the selectors have different lengths, or if a selector names a value or a -/// row that does not exist. +/// Panics if the selectors have different lengths, or if a selector names a value or a row that +/// does not exist. pub(super) fn gather( array: &Array, sources: &[Source], allocator: &BufferAllocatorRef, -) -> VortexResult> { +) -> Buffer { let branches = array.array_indices().as_::(); let rows = array.row_indices().as_::(); @@ -90,14 +87,14 @@ fn gather_rows( branches: &[A], rows: &[R], allocator: &BufferAllocatorRef, -) -> VortexResult> +) -> Buffer where T: Copy, A: AsPrimitive, R: AsPrimitive, { // `zip` truncates to the shorter input. - vortex_ensure_eq!( + assert_eq!( rows.len(), branches.len(), "interleave row_indices length does not match array_indices length", @@ -110,48 +107,61 @@ where let mut output = BufferMut::with_capacity_in(len, allocator.clone()); let slots = &mut output.spare_capacity_mut()[..len]; for ((slot, branch), row) in slots.iter_mut().zip(branches).zip(rows) { - let Some(source) = sources.get((*branch).as_()) else { - vortex_bail!("interleave array index out of bounds"); + let branch = (*branch).as_(); + let Some(source) = sources.get(branch) else { + vortex_panic!( + "interleave array index {branch} out of bounds for {} values", + sources.len() + ); }; + // NB: `row & row_mask` is always in bounds for a constant, so this check is the only one + // that rejects a row past the end of a constant. let row = (*row).as_(); - vortex_ensure!(row < source.len, "interleave row index out of bounds"); + assert!( + row < source.len, + "interleave row index {row} out of bounds for length {}", + source.len + ); slot.write(source.data[row & source.row_mask]); } - // SAFETY: the loop returns on any error, so reaching here means it wrote all `len` slots: + // SAFETY: the loop panics on a bad selector, so reaching here means it wrote all `len` slots: // `slots`, `branches`, and `rows` all have length `len`. unsafe { output.set_len(len) }; - Ok(output.freeze()) + output.freeze() } #[cfg(test)] mod tests { use rstest::rstest; use vortex_buffer::buffer; - use vortex_error::VortexResult; use super::Source; use super::gather_rows; use crate::memory::BufferAllocatorRef; - #[rstest] - #[case::array_index(2, 0, "array index out of bounds")] - #[case::constant_row_index(0, 1, "row index out of bounds")] - #[case::decoded_row_index(1, 1, "row index out of bounds")] - fn rejects_out_of_bounds_selectors( - #[case] branch: u8, - #[case] row: u8, - #[case] message: &str, - ) -> VortexResult<()> { + /// Gathers row `row` of value `branch`. Value 0 is a one-row constant, and value 1 is a one-row + /// decoded buffer. + fn gather_one(branch: u8, row: u8) { let allocator = BufferAllocatorRef::statically_allocated(); let sources = [ Source::constant(1u32, 1, &allocator), Source::decoded(buffer![2u32]), ]; - let error = gather_rows(&sources, &[branch], &[row], &allocator) - .expect_err("every case names a value or a row that does not exist"); - assert!(error.to_string().contains(message), "{error}"); + drop(gather_rows(&sources, &[branch], &[row], &allocator)); + } - Ok(()) + #[test] + #[should_panic(expected = "interleave array index 2 out of bounds for 2 values")] + fn panics_on_out_of_bounds_array_index() { + gather_one(2, 0); + } + + #[rstest] + #[case::constant(0)] + #[case::decoded(1)] + #[should_panic(expected = "interleave row index 1 out of bounds for length 1")] + fn panics_on_out_of_bounds_row_index(#[case] branch: u8) { + gather_one(branch, 1); } } diff --git a/vortex-array/src/arrays/interleave/execute/primitive.rs b/vortex-array/src/arrays/interleave/execute/primitive.rs index c758d49b47b..d971481c8e1 100644 --- a/vortex-array/src/arrays/interleave/execute/primitive.rs +++ b/vortex-array/src/arrays/interleave/execute/primitive.rs @@ -42,7 +42,7 @@ pub(super) fn execute( let values = match_each_ptype_width!( ptype, |W| { - gather_values::(&array, ctx.allocator())? + gather_values::(&array, ctx.allocator()) .into_byte_buffer() .aligned(Alignment::new(W)) }, @@ -56,7 +56,7 @@ pub(super) fn execute( fn gather_values( array: &Array, allocator: &BufferAllocatorRef, -) -> VortexResult> { +) -> Buffer<[u8; W]> { let sources = (0..array.num_values()) .map(|i| { let value = array.value(i); diff --git a/vortex-array/src/arrays/interleave/mod.rs b/vortex-array/src/arrays/interleave/mod.rs index b9c5c7374ee..3076d59bbf0 100644 --- a/vortex-array/src/arrays/interleave/mod.rs +++ b/vortex-array/src/arrays/interleave/mod.rs @@ -26,8 +26,8 @@ //! nullability must be resolved into definite indices by the caller *before* the interleave is //! built. //! - `array_indices[i] < values.len()` and `row_indices[i] < values[array_indices[i]].len()` for -//! every `i`. These per-row bounds depend on the selector *values* and so are a runtime -//! precondition of the caller, checked in the execution kernels rather than at construction. +//! every `i`. These per-row bounds depend on the selector *values*, so construction does not +//! check them. They are a precondition of the caller, and execution panics when one fails. //! - All values share a logical type up to nullability. The output type is that shared type with //! the union of the values' nullabilities. This is orthogonal to the selectors: a row's *value* //! may be null even though its `(array_index, row_index)` is definite. @@ -639,42 +639,31 @@ mod tests { } #[test] - fn execute_rejects_out_of_bounds_array_index() -> VortexResult<()> { + #[should_panic(expected = "interleave array index 2 out of bounds for 2 values")] + fn execute_panics_on_out_of_bounds_array_index() { let value = BoolArray::from_iter([true]).into_array(); let array_indices = PrimitiveArray::from_iter([2u32]).into_array(); let row_indices = PrimitiveArray::from_iter([0u32]).into_array(); let interleaved = - InterleaveArray::try_new(vec![value.clone(), value], array_indices, row_indices)? + InterleaveArray::try_new(vec![value.clone(), value], array_indices, row_indices) + .vortex_expect("the selectors are non-nullable unsigned integers of equal length") .into_array(); - let mut ctx = SESSION.create_execution_ctx(); - let err = interleaved - .execute::(&mut ctx) - .err() - .vortex_expect("expected execution to reject out-of-bounds array index"); - assert!( - err.to_string().contains("array index out of bounds"), - "{err}" - ); - Ok(()) + drop(interleaved.execute::(&mut SESSION.create_execution_ctx())); } #[test] - fn execute_rejects_out_of_bounds_row_index() -> VortexResult<()> { + #[should_panic(expected = "interleave row index 1 out of bounds for length 1")] + fn execute_panics_on_out_of_bounds_row_index() { let value = BoolArray::from_iter([true]).into_array(); let array_indices = PrimitiveArray::from_iter([0u32]).into_array(); let row_indices = PrimitiveArray::from_iter([1u32]).into_array(); let interleaved = - InterleaveArray::try_new(vec![value.clone(), value], array_indices, row_indices)? + InterleaveArray::try_new(vec![value.clone(), value], array_indices, row_indices) + .vortex_expect("the selectors are non-nullable unsigned integers of equal length") .into_array(); - let mut ctx = SESSION.create_execution_ctx(); - let err = interleaved - .execute::(&mut ctx) - .err() - .vortex_expect("expected execution to reject out-of-bounds row index"); - assert!(err.to_string().contains("row index out of bounds"), "{err}"); - Ok(()) + drop(interleaved.execute::(&mut SESSION.create_execution_ctx())); } #[test] From b0c7c8fbd3ba0e9e5a229c5502091006945ae9f4 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Wed, 28 Oct 2026 17:02:50 -0400 Subject: [PATCH 2/2] Panic on out-of-bounds RunEnd and Chunked take indices The fixed-width take kernel already panics on an index past the end of the array. `RunEnd` and `Chunked` take returned an error instead, so the result of a bad index depended on the encoding of the array. Both now panic with the message of the fixed-width kernel. Signed-off-by: Connor Tsui --- .../src/arrays/chunked/compute/take.rs | 93 ++++++++----------- .../src/arrays/runend/compute/take.rs | 51 +++++----- 2 files changed, 68 insertions(+), 76 deletions(-) diff --git a/vortex-array/src/arrays/chunked/compute/take.rs b/vortex-array/src/arrays/chunked/compute/take.rs index 5602d051b23..bf0e8802cbe 100644 --- a/vortex-array/src/arrays/chunked/compute/take.rs +++ b/vortex-array/src/arrays/chunked/compute/take.rs @@ -7,9 +7,9 @@ use vortex_buffer::Buffer; use vortex_buffer::BufferAllocatorRef; use vortex_buffer::BufferMut; use vortex_error::VortexResult; -use vortex_error::vortex_bail; use vortex_error::vortex_ensure_eq; use vortex_error::vortex_err; +use vortex_error::vortex_panic; use vortex_mask::Mask; use crate::ArrayRef; @@ -96,14 +96,11 @@ fn take_chunked_via_sort( if let Some(&(index, _)) = pairs.last() { let index = usize::try_from(index)?; - if index >= array.len() { - vortex_bail!( - "index {} out of bounds from {} to {}", - index, - 0, - array.len() - ); - } + let len = array.len(); + assert!( + index < len, + "take index {index} out of bounds for length {len}" + ); } let chunk_offsets = array.chunk_offset_values(); @@ -213,14 +210,11 @@ fn take_chunked( } let index = usize::try_from(index)?; - if index >= array.len() { - vortex_bail!( - "index {} out of bounds from {} to {}", - index, - 0, - array.len() - ); - } + let len = array.len(); + assert!( + index < len, + "take index {index} out of bounds for length {len}" + ); let still_sorted = last_index.is_none_or(|last_index| index >= last_index); let chunk_idx = if monotonic && still_sorted { @@ -322,13 +316,10 @@ fn take_chunked_sorted( } cursor = range_end; } - if cursor != indices_values.len() { - vortex_bail!( - "index {} out of bounds from {} to {}", - usize::try_from(indices_values[cursor])?, - 0, - array.as_ref().len() - ); + // The chunk loop stops at the first index that no chunk holds. + if let Some(&index) = indices_values.get(cursor) { + let len = array.as_ref().len(); + vortex_panic!("take index {index} out of bounds for length {len}"); } if chunks.len() == 1 { @@ -446,14 +437,11 @@ fn take_piecewise_chunked( let end = start .checked_add(length) .ok_or_else(|| vortex_err!("PiecewiseSequenceArray range overflows usize"))?; - if end > array_len { - vortex_bail!( - "index {} out of bounds from {} to {}", - end - 1, - 0, - array_len - ); - } + assert!( + end <= array_len, + "take index {} out of bounds for length {array_len}", + end - 1 + ); total_len = total_len .checked_add(length) .ok_or_else(|| vortex_err!("PiecewiseSequenceArray output length overflows usize"))?; @@ -579,9 +567,11 @@ fn contiguous_runs(starts: Vec, lengths: Vec, total: usize) -> ArrayRe mod tests { use std::sync::LazyLock; + use rstest::rstest; use vortex_buffer::Buffer; use vortex_buffer::bitbuffer; use vortex_buffer::buffer; + use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_session::VortexSession; @@ -743,17 +733,16 @@ mod tests { } #[test] - fn test_take_piecewise_out_of_bounds() -> VortexResult<()> { + #[should_panic(expected = "take index 16 out of bounds for length 15")] + fn test_take_piecewise_out_of_bounds() { let mut ctx = SESSION.create_execution_ctx(); - let arr = chunked_i32()?; - - let indices = contiguous_pieces(&[12], &[5])?; - let result = arr - .take(indices) - .and_then(|taken| taken.execute::(&mut ctx)); + let arr = chunked_i32().vortex_expect("the chunks have the dtype of the array"); + let indices = contiguous_pieces(&[12], &[5]).vortex_expect("the pieces are valid"); - assert!(result.is_err()); - Ok(()) + drop( + arr.take(indices) + .and_then(|taken| taken.execute::(&mut ctx)), + ); } #[test] @@ -1351,19 +1340,19 @@ mod tests { Ok(()) } - #[test] - fn test_take_out_of_bounds() -> VortexResult<()> { + /// Sorted indices take the sorted path, and unsorted indices take the generic path. + #[rstest] + #[case::sorted(buffer![0u64, 100])] + #[case::unsorted(buffer![100u64, 0])] + #[should_panic(expected = "take index 100 out of bounds for length 15")] + fn test_take_out_of_bounds(#[case] indices: Buffer) { let mut ctx = SESSION.create_execution_ctx(); - let arr = chunked_i32()?; + let arr = chunked_i32().vortex_expect("the chunks have the dtype of the array"); - // Sorted indices hit the fast path, unsorted the generic path; both must fail. - for indices in [buffer![0u64, 100], buffer![100u64, 0]] { - let result = arr - .take(indices.into_array()) - .and_then(|taken| taken.execute::(&mut ctx)); - assert!(result.is_err(), "expected out-of-bounds error"); - } - Ok(()) + drop( + arr.take(indices.into_array()) + .and_then(|taken| taken.execute::(&mut ctx)), + ); } #[test] diff --git a/vortex-array/src/arrays/runend/compute/take.rs b/vortex-array/src/arrays/runend/compute/take.rs index c7a568fba6f..939e2c87530 100644 --- a/vortex-array/src/arrays/runend/compute/take.rs +++ b/vortex-array/src/arrays/runend/compute/take.rs @@ -6,7 +6,6 @@ use num_traits::NumCast; use vortex_buffer::Buffer; use vortex_buffer::BufferMut; use vortex_error::VortexResult; -use vortex_error::vortex_bail; use vortex_mask::AllOr; use vortex_mask::Mask; @@ -90,7 +89,7 @@ fn take_indices>( ); } - let stats = valid_indices_stats(indices, validity_mask, array.len(), check_bounds)?; + let stats = valid_indices_stats(indices, validity_mask, array.len(), check_bounds); let ends = array.ends().clone().execute::(ctx)?; let physical_indices = match_each_unsigned_integer_ptype!(ends.ptype(), |I| { @@ -183,54 +182,58 @@ where /// Count the valid indices and determine whether they are sorted, bounds-checking each valid /// index against `array_len` when `check_bounds` is set. +/// +/// # Panics +/// +/// Panics if `check_bounds` is set and a valid index is not below `array_len`. fn valid_indices_stats>( indices: &[T], validity_mask: &Mask, array_len: usize, check_bounds: bool, -) -> VortexResult { +) -> ValidIndicesStats { debug_assert_eq!(indices.len(), validity_mask.len()); let count = validity_mask.true_count(); if count == 0 { - return Ok(ValidIndicesStats { + return ValidIndicesStats { count, sorted: true, - }); + }; } let sorted = match validity_mask.bit_buffer() { - AllOr::All => valid_indices_sorted_all(indices, array_len, check_bounds)?, + AllOr::All => valid_indices_sorted_all(indices, array_len, check_bounds), AllOr::None => true, AllOr::Some(validity) => { - valid_indices_sorted_masked(indices, validity.iter(), array_len, check_bounds)? + valid_indices_sorted_masked(indices, validity.iter(), array_len, check_bounds) } }; - Ok(ValidIndicesStats { count, sorted }) + ValidIndicesStats { count, sorted } } fn valid_indices_sorted_all>( indices: &[T], array_len: usize, check_bounds: bool, -) -> VortexResult { +) -> bool { // Seed the comparison with the first index; an empty or single-element slice is trivially // sorted, so the loop below starts from the second element. let Some((first, rest)) = indices.split_first() else { - return Ok(true); + return true; }; let mut previous_idx = first.as_(); if check_bounds { - check_index(previous_idx, array_len)?; + check_index(previous_idx, array_len); } let mut sorted = true; for idx in rest { let idx = idx.as_(); if check_bounds { - check_index(idx, array_len)?; + check_index(idx, array_len); } if previous_idx > idx { sorted = false; @@ -241,7 +244,7 @@ fn valid_indices_sorted_all>( previous_idx = idx; } - Ok(sorted) + sorted } fn valid_indices_sorted_masked>( @@ -249,7 +252,7 @@ fn valid_indices_sorted_masked>( is_valid: impl Iterator, array_len: usize, check_bounds: bool, -) -> VortexResult { +) -> bool { // Invalid positions are skipped without a bounds check, matching the take path that never // dereferences them. let mut valid = is_valid @@ -260,16 +263,16 @@ fn valid_indices_sorted_masked>( // Seed the comparison with the first valid index; zero or one valid index is trivially // sorted, so the loop below starts from the second valid index. let Some(mut previous_idx) = valid.next() else { - return Ok(true); + return true; }; if check_bounds { - check_index(previous_idx, array_len)?; + check_index(previous_idx, array_len); } let mut sorted = true; for idx in valid { if check_bounds { - check_index(idx, array_len)?; + check_index(idx, array_len); } if previous_idx > idx { sorted = false; @@ -280,7 +283,7 @@ fn valid_indices_sorted_masked>( previous_idx = idx; } - Ok(sorted) + sorted } fn prefer_linear_scan( @@ -291,11 +294,11 @@ fn prefer_linear_scan( ends_len <= valid_count.saturating_mul(runs_per_index_threshold) } -fn check_index(index: usize, array_len: usize) -> VortexResult<()> { - if index >= array_len { - vortex_bail!("index {} out of bounds from {} to {}", index, 0, array_len); - } - Ok(()) +fn check_index(index: usize, array_len: usize) { + assert!( + index < array_len, + "take index {index} out of bounds for length {array_len}" + ); } fn physical_indices_linear_sorted( @@ -542,7 +545,7 @@ mod tests { } #[test] - #[should_panic] + #[should_panic(expected = "take index 12 out of bounds for length 12")] fn ree_take_out_of_bounds() { let _array = ree_array() .take(buffer![12].into_array())