From 9ca225017ac2593c4186293146b40599877b3a5b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Fri, 15 May 2026 22:49:05 +0000 Subject: [PATCH] fix(decimal): fall back to Arrow comparison when bound storage type differs from array storage type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `between_unpack` could not cast a bound scalar's backing integer value to the array's storage type `T` (e.g. `DecimalValue::I32(82246)` → `i16`), it previously returned `Err(vortex_bail!(...))`. The fuzzer found a case where a `DecimalArray` with `values_type = I16` received a bound stored as `DecimalValue::I32`, causing the error to propagate and the fuzz harness to panic via `vortex_expect`. The fix: return `Ok(None)` instead of `Err(...)` in that case, signalling that this kernel cannot handle the input. `between_canonical` then falls through to the generic Arrow-based comparison path, which compares decimal values semantically by their shared precision and scale and correctly handles mixed storage widths. A regression test exercises the exact scenario from the crash report: an `i16`-backed decimal array compared against bounds backed by `i32`, where the upper bound value (82246) exceeds `i16::MAX`. Fixes #7955 Co-authored-by: Nicholas Gates Signed-off-by: "Claude" --- .../src/arrays/decimal/compute/between.rs | 15 ++-- vortex-array/src/scalar_fn/fns/between/mod.rs | 68 +++++++++++++++++++ 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/vortex-array/src/arrays/decimal/compute/between.rs b/vortex-array/src/arrays/decimal/compute/between.rs index 3594e5a268c..cfae1ba8836 100644 --- a/vortex-array/src/arrays/decimal/compute/between.rs +++ b/vortex-array/src/arrays/decimal/compute/between.rs @@ -4,7 +4,6 @@ use vortex_buffer::BitBuffer; use vortex_error::VortexExpect; use vortex_error::VortexResult; -use vortex_error::vortex_bail; use crate::ArrayRef; use crate::ExecutionCtx; @@ -57,20 +56,18 @@ fn between_unpack( .decimal_value() .and_then(|v| v.cast::()) else { - vortex_bail!( - "invalid lower bound Scalar: {lower}, expected {:?}", - T::DECIMAL_TYPE - ) + // The bound's backing integer value doesn't fit in T (the array's storage type). This + // can happen when the bound scalar uses a wider decimal storage type than the array, + // even though both share the same precision and scale. Signal to the caller to fall + // back to the generic Arrow-based comparison, which handles mixed storage types. + return Ok(None); }; let Some(upper_value) = upper .as_decimal() .decimal_value() .and_then(|v| v.cast::()) else { - vortex_bail!( - "invalid upper bound Scalar: {upper}, expected {:?}", - T::DECIMAL_TYPE - ) + return Ok(None); }; let lower_op = match options.lower_strict { diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index bb44d71c846..3616f8d695c 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -339,6 +339,8 @@ mod tests { use rstest::rstest; use vortex_buffer::buffer; + use vortex_error::VortexResult; + use super::*; use crate::IntoArray; use crate::VortexSessionExecute; @@ -545,4 +547,70 @@ mod tests { BoolArray::from_iter([true, true, true, false]) ); } + + /// Regression test: between on a decimal array whose storage type is narrower than the + /// bound scalars' backing integer type must not crash. + /// + /// The fuzzer discovered that a DecimalArray with `values_type = I16` (i16 backing) can + /// receive bound scalars stored as `DecimalValue::I32` when both share the same precision + /// and scale. The old code called `vortex_bail!` when the cast from i32 to i16 failed, + /// propagating an error that the fuzz harness turned into a panic. + #[test] + fn test_between_decimal_mismatched_storage_types() -> VortexResult<()> { + // Array backed by i16, with precision=5 and a negative scale. + // DecimalArray allows narrower-than-canonical backing when all values fit. + let values = buffer![100i16, 200i16, 300i16, 400i16]; + let decimal_type = DecimalDType::new(5, -3); + let array = DecimalArray::new(values, decimal_type, Validity::NonNullable).into_array(); + + // Lower bound stored as I32(100) – fits in i16, but the backing type differs. + let lower = ConstantArray::new( + Scalar::decimal(DecimalValue::I32(100i32), decimal_type, Nullability::NonNullable), + array.len(), + ) + .into_array(); + + // Upper bound stored as I32(82246) – does NOT fit in i16 (> i16::MAX = 32767). + // All array values are <= 32767 < 82246, so every element satisfies the upper bound. + let upper = ConstantArray::new( + Scalar::decimal(DecimalValue::I32(82246i32), decimal_type, Nullability::NonNullable), + array.len(), + ) + .into_array(); + + let result = between_canonical( + &array, + &lower, + &upper, + &BetweenOptions { + lower_strict: StrictComparison::NonStrict, + upper_strict: StrictComparison::NonStrict, + }, + &mut SESSION.create_execution_ctx(), + )?; + // 100 <= [100,200,300,400] <= 82246 → all true + assert_arrays_eq!(result, BoolArray::from_iter([true, true, true, true])); + + // Verify that values outside the lower bound are correctly excluded. + let lower_high = ConstantArray::new( + Scalar::decimal(DecimalValue::I32(250i32), decimal_type, Nullability::NonNullable), + array.len(), + ) + .into_array(); + + let result2 = between_canonical( + &array, + &lower_high, + &upper, + &BetweenOptions { + lower_strict: StrictComparison::NonStrict, + upper_strict: StrictComparison::NonStrict, + }, + &mut SESSION.create_execution_ctx(), + )?; + // 250 <= [100,200,300,400] <= 82246 → [false, false, true, true] + assert_arrays_eq!(result2, BoolArray::from_iter([false, false, true, true])); + + Ok(()) + } }