From 5623a2e307d574513ce76248f0d95254173d36bc Mon Sep 17 00:00:00 2001 From: Abanoub Doss Date: Thu, 21 May 2026 08:46:09 -0500 Subject: [PATCH] =?UTF-8?q?test(vortex-array):=20repro=20for=20ABA-23=20?= =?UTF-8?q?=E2=80=94=20integer=20arithmetic=20panics=20on=20overflow=20/?= =?UTF-8?q?=20div-by-zero?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add 5 inline repro tests in `scalar_fn::fns::binary::numeric::tests`: - 4 `#[ignore]` + `#[should_panic]` tests locking the current wrong behaviour (i32 add overflow, i64 div-by-zero, u32 sub underflow, i32 mul overflow — all panic via `to_primitive()` -> `vortex_expect("to_canonical failed")`). - 1 always-green test confirming `to_canonical()` (the VortexResult path) returns `Err` gracefully rather than panicking. Root cause: `ToCanonical::to_primitive()` (canonical.rs:489-492) calls `to_canonical().vortex_expect(...)`, converting any Arrow arithmetic `Err` into a process-aborting panic that callers cannot catch as `VortexResult`. Fixes: https://linear.app/abanoubdoss/issue/ABA-23 Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: Abanoub Doss --- .../src/scalar_fn/fns/binary/numeric.rs | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/vortex-array/src/scalar_fn/fns/binary/numeric.rs b/vortex-array/src/scalar_fn/fns/binary/numeric.rs index 9429a7310d1..526581b29fe 100644 --- a/vortex-array/src/scalar_fn/fns/binary/numeric.rs +++ b/vortex-array/src/scalar_fn/fns/binary/numeric.rs @@ -72,6 +72,136 @@ fn constant_numeric( Ok(Some(ConstantArray::new(result, lhs.len()).into_array())) } +/// Repro tests for ABA-23: integer arithmetic panics on overflow / divide-by-zero. +/// +/// The bug: `to_primitive()` calls `to_canonical().vortex_expect("to_canonical failed")`, +/// which panics on any `Err` (canonical.rs:489-492). Arrow arithmetic kernels return `Err` +/// on integer overflow and divide-by-zero, but because evaluation is lazy the error only +/// surfaces inside `to_canonical`, where `vortex_expect` converts it to an unrecoverable +/// panic. Callers cannot catch this with normal `VortexResult` handling. +/// +/// Each test is marked `#[should_panic]` (current wrong behaviour = panics) AND +/// `#[ignore]` (so CI stays green until a fix lands). +/// When the fix ships, flip `#[should_panic]` to an assertion that `to_canonical` returns +/// `Err`, and remove `#[ignore]`. +/// +/// See: +#[cfg(test)] +#[allow(deprecated)] // intentionally exercising the deprecated to_primitive() panic site +mod tests { + use std::panic; + use std::panic::AssertUnwindSafe; + + use vortex_buffer::buffer; + + use crate::IntoArray; + use crate::ToCanonical; + use crate::builtins::ArrayBuiltins; + use crate::scalar_fn::fns::operators::Operator; + + /// Materializes a lazy binary array via `to_canonical()` (the `VortexResult` path), + /// NOT via `to_primitive()` (which panics). Returns `Ok(())` on success or graceful + /// `Err`; returns `Err(String)` if `to_canonical` itself returns `Err`. + /// + /// This helper is intentionally separate from `to_primitive()` so the tests can + /// document *where* the panic is rather than triggering it through the happy path. + fn eval_to_canonical_result( + lhs: crate::ArrayRef, + rhs: crate::ArrayRef, + op: Operator, + ) -> Result<(), String> { + let arr = lhs + .binary(rhs, op) + .map_err(|e| format!("binary() Err: {e}"))?; + arr.to_canonical() + .map(|_| ()) + .map_err(|e| format!("to_canonical Err: {e}")) + } + + #[test] + #[ignore = "demonstrates ABA-23; see https://linear.app/abanoubdoss/issue/ABA-23"] + #[should_panic(expected = "to_canonical failed")] + fn issue_aba23_i32_add_max_plus_one_must_not_panic() { + // i32::MAX + 1 overflows. Arrow returns Err; `to_primitive()` panics via + // `vortex_expect`. This test locks the wrong behaviour: flip when fixed. + let lhs = buffer![i32::MAX].into_array(); + let rhs = buffer![1i32].into_array(); + let arr = lhs + .binary(rhs, Operator::Add) + .expect("binary() should not fail"); + // to_primitive() is the panic site (canonical.rs:489-492). + drop(arr.to_primitive()); + } + + #[test] + #[ignore = "demonstrates ABA-23; see https://linear.app/abanoubdoss/issue/ABA-23"] + #[should_panic(expected = "to_canonical failed")] + fn issue_aba23_i64_div_by_zero_must_not_panic() { + // Integer divide-by-zero: Arrow returns Err; `to_primitive()` panics. + let lhs = buffer![10i64].into_array(); + let rhs = buffer![0i64].into_array(); + let arr = lhs + .binary(rhs, Operator::Div) + .expect("binary() should not fail"); + drop(arr.to_primitive()); + } + + #[test] + #[ignore = "demonstrates ABA-23; see https://linear.app/abanoubdoss/issue/ABA-23"] + #[should_panic(expected = "to_canonical failed")] + fn issue_aba23_u32_sub_underflow_must_not_panic() { + // 0u32 - 1 underflows (unsigned). Arrow returns Err; `to_primitive()` panics. + let lhs = buffer![0u32].into_array(); + let rhs = buffer![1u32].into_array(); + let arr = lhs + .binary(rhs, Operator::Sub) + .expect("binary() should not fail"); + drop(arr.to_primitive()); + } + + #[test] + #[ignore = "demonstrates ABA-23; see https://linear.app/abanoubdoss/issue/ABA-23"] + #[should_panic(expected = "to_canonical failed")] + fn issue_aba23_i32_mul_overflow_must_not_panic() { + // i32::MAX * 2 overflows. Arrow returns Err; `to_primitive()` panics. + let lhs = buffer![i32::MAX].into_array(); + let rhs = buffer![2i32].into_array(); + let arr = lhs + .binary(rhs, Operator::Mul) + .expect("binary() should not fail"); + drop(arr.to_primitive()); + } + + /// Verify the `to_canonical()` path (the `VortexResult` path) does surface the error + /// rather than panicking. This test documents the *desired* graceful behaviour and + /// should stay green both before and after the fix. + #[test] + fn issue_aba23_to_canonical_returns_err_not_panic_on_overflow() { + // Use catch_unwind to distinguish graceful Err from panic. + let prev_hook = panic::take_hook(); + panic::set_hook(Box::new(|_| {})); + let result = panic::catch_unwind(AssertUnwindSafe(|| { + eval_to_canonical_result( + buffer![i32::MAX].into_array(), + buffer![1i32].into_array(), + Operator::Add, + ) + })); + panic::set_hook(prev_hook); + + match result { + Ok(Err(_graceful_error)) => { /* correct: graceful VortexResult::Err */ } + Ok(Ok(())) => { + panic!("ABA-23: i32::MAX + 1 returned Ok — silent integer wrap is also wrong.") + } + Err(_panic) => panic!( + "ABA-23: i32::MAX + 1 panicked via to_canonical() — the VortexResult path \ + should not panic; only to_primitive() (which calls vortex_expect) should." + ), + } + } +} + #[cfg(test)] mod test { use vortex_buffer::buffer;