From c58976862cc5a1d6b3a8bbb3f6672729fb4f5e62 Mon Sep 17 00:00:00 2001 From: Peter Bower <37089506+pbower@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:58:38 +0100 Subject: [PATCH] Numerical and mask correctness audit + fixes Fixes integer division contract (as it was thresholding instead of returning floats for Scalar) + Small mask correctness bugfixes + improvements : 1. Correct Value::len to report the window for an array view 2. Add get_unchecked and Copy to BitmaskV 3. Make guarantee_f64 return windowed null mask 4. Document and pin integer floor division contract 5. Ensure integer power validates its exponent and wraps on overflow 6. Ensure Integer division rounds towards negative infinity 7. Ensure scalar integer division is true division --- src/enums/operators.rs | 27 +++- src/enums/value/mod.rs | 60 +++++++- src/kernels/arithmetic/mod.rs | 75 ++++++++++ src/kernels/arithmetic/simd.rs | 130 +++++++++++------- src/kernels/arithmetic/std.rs | 88 +++++++----- src/kernels/routing/arithmetic.rs | 18 +-- src/structs/views/bitmask_view.rs | 17 ++- .../views/collections/numeric_array_view.rs | 66 +++++++-- 8 files changed, 375 insertions(+), 106 deletions(-) diff --git a/src/enums/operators.rs b/src/enums/operators.rs index cffb126..177310b 100644 --- a/src/enums/operators.rs +++ b/src/enums/operators.rs @@ -25,18 +25,33 @@ pub enum ArithmeticOperator { Multiply, /// Division (`lhs / rhs`) /// - /// For integers, division by zero panics in unmasked arrays and nullifies in masked arrays. - /// For floating-point, follows IEEE 754 (yields ±Inf or NaN). + /// Division is true division: `7 / 2` is `3.5`. The `Scalar` arms + /// return a `Float64` scalar for integer operands, and callers dividing + /// integer arrays cast their operands to `f64` before dispatch so the + /// float kernels produce the float result. + /// + /// The integer slice kernels themselves serve `FloorDiv`, so `Divide` + /// handed raw integer slices at the kernel level behaves as floor + /// division. Cast to float first for a true-division result. + /// + /// Division by zero on floats follows IEEE 754: a nonzero value over + /// zero yields Inf with the operands' sign, and zero over zero yields + /// NaN. On raw integer slices it panics in unmasked arrays and + /// nullifies in masked arrays. Divide, /// Modulus/remainder operation (`lhs % rhs`) /// - /// Behaviour matches Rust's `%` operator. Division by zero handling follows same - /// rules as `Divide` operation. + /// Behaviour matches Rust's `%` operator: the result keeps the + /// dividend's sign, so `-7 % 2` is `-1`. Division by zero handling + /// follows same rules as `Divide` operation. Remainder, /// Exponentiation (`lhs ^ rhs`) /// - /// For integers, uses repeated multiplication. For floating-point, uses `pow()` function. - /// Negative exponents on integers may yield zero due to truncation. + /// For integers, exponentiation by squaring with wrapping + /// multiplication, so overflow wraps like the other integer arms. The + /// exponent must convert to `u32`: a negative or larger exponent + /// returns an error advising a cast to float. For floating-point, uses + /// logarithmic computation. Power, /// Floor division (`lhs // rhs`) /// diff --git a/src/enums/value/mod.rs b/src/enums/value/mod.rs index 5f42024..530365d 100644 --- a/src/enums/value/mod.rs +++ b/src/enums/value/mod.rs @@ -160,7 +160,7 @@ impl Value { Value::Array(a) => a.len(), #[cfg(feature = "views")] - Value::ArrayView(av) => av.array.len(), + Value::ArrayView(av) => av.len(), Value::FieldArray(fa) => fa.array.len(), @@ -376,3 +376,61 @@ impl Value { } // Also see typed accessors in ./conversions.rs and trait impls in ./impls.rs + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use super::*; + use crate::{ + Array, ArrayV, ArrowType, Field, FieldArray, IntegerArray, MaskedArray, NumericArray, Table, + }; + + fn seq_array(n: usize) -> Array { + let mut arr = IntegerArray::::default(); + for i in 0..n { + arr.push(i as i64); + } + Array::NumericArray(NumericArray::Int64(Arc::new(arr))) + } + + /// `len` counts the rows a view covers, so callers can pass the count + /// straight to `slice`. + #[cfg(feature = "views")] + #[test] + fn len_of_an_array_view_counts_the_window() { + let view = ArrayV::new(seq_array(100), 30, 10); + let value = Value::ArrayView(Arc::new(view)); + + assert_eq!(value.len(), 10, "the window, not the backing array"); + } + + /// The two view variants agree, so an operator sizing its work from + /// `len` behaves the same whichever shape it was handed. + #[cfg(feature = "views")] + #[test] + fn len_agrees_across_the_view_variants() { + let array = Value::ArrayView(Arc::new(ArrayV::new(seq_array(100), 30, 10))); + + let mut table = Table::new("t".to_string(), None); + table.add_col(FieldArray::new( + Field::new("v", ArrowType::Int64, false, None), + seq_array(100), + )); + let table = Value::Table(Arc::new(table)).slice(30, 10); + + assert_eq!(array.len(), table.len()); + } + + /// `len` bounds a `slice` on the same value. The two are read together + /// wherever work is split into row ranges, so a `len` drawn from the + /// backing array would run a window past its own end. + #[cfg(feature = "views")] + #[test] + fn len_bounds_a_slice_of_the_same_value() { + let value = Value::ArrayView(Arc::new(ArrayV::new(seq_array(100), 30, 10))); + + let whole = value.slice(0, value.len()); + assert_eq!(whole.len(), 10); + } +} diff --git a/src/kernels/arithmetic/mod.rs b/src/kernels/arithmetic/mod.rs index 62c3116..5a362fe 100644 --- a/src/kernels/arithmetic/mod.rs +++ b/src/kernels/arithmetic/mod.rs @@ -290,6 +290,81 @@ mod tests { apply_int_u64 ); + // At the slice-kernel level, integer division rounds towards negative + // infinity. Rust's `/` truncates towards zero, so the two differ on + // every negative inexact quotient: -7 / 2 floors to -4 where truncation + // gives -3. Divide and FloorDiv therefore agree on integer slices. + // Remainder keeps the dividend's sign per Rust's `%`. + + #[test] + fn int_divide_floors_towards_negative_infinity_dense() { + let lhs = vec64![-7i64, 7, -7, 8, -8]; + let rhs = vec64![2i64, 2, -2, 2, 2]; + + let out = apply_int_i64(&lhs, &rhs, ArithmeticOperator::Divide, None).unwrap(); + assert_int(&out, &[-4, 3, 3, 4, -4], None); + + let floor = apply_int_i64(&lhs, &rhs, ArithmeticOperator::FloorDiv, None).unwrap(); + assert_eq!( + out.data.as_slice(), + floor.data.as_slice(), + "Divide and FloorDiv agree on integers" + ); + } + + #[test] + fn int_divide_floors_towards_negative_infinity_masked() { + let lhs = vec64![-7i64, 8, 9]; + let rhs = vec64![2i64, 0, 3]; + let mask = bitmask(&[true, true, true]); + + let out = apply_int_i64(&lhs, &rhs, ArithmeticOperator::Divide, Some(&mask)).unwrap(); + assert_int(&out, &[-4, 0, 3], Some(&[true, false, true])); + } + + #[test] + fn int_divide_unsigned_never_rounds() { + // Unsigned quotients never round below zero, so the floor rule + // leaves them identical to truncation. + let lhs = vec64![7u64, 9]; + let rhs = vec64![2u64, 4]; + + let out = apply_int_u64(&lhs, &rhs, ArithmeticOperator::Divide, None).unwrap(); + assert_int(&out, &[3, 2], None); + } + + #[test] + fn int_remainder_keeps_dividend_sign() { + let lhs = vec64![-7i64, 7, -7]; + let rhs = vec64![2i64, -2, -2]; + + let out = apply_int_i64(&lhs, &rhs, ArithmeticOperator::Remainder, None).unwrap(); + assert_int(&out, &[-1, 1, -1], None); + } + + #[cfg(feature = "scalar_type")] + #[test] + fn scalar_divide_is_true_division() { + use crate::Scalar; + use crate::kernels::routing::arithmetic::scalar_arithmetic; + + let out = scalar_arithmetic( + Scalar::Int64(-7), + Scalar::Int64(2), + ArithmeticOperator::Divide, + ) + .unwrap(); + assert_eq!(out, Scalar::Float64(-3.5)); + + let out = scalar_arithmetic( + Scalar::UInt32(7), + Scalar::UInt32(2), + ArithmeticOperator::Divide, + ) + .unwrap(); + assert_eq!(out, Scalar::Float64(3.5)); + } + macro_rules! float_kernel_suite { ($test_fn:ident, $ty:ty, $apply_fn:ident, $eps:expr) => { #[test] diff --git a/src/kernels/arithmetic/simd.rs b/src/kernels/arithmetic/simd.rs index 81983c2..a2152f8 100644 --- a/src/kernels/arithmetic/simd.rs +++ b/src/kernels/arithmetic/simd.rs @@ -44,12 +44,14 @@ use crate::Bitmask; use num_traits::{One, PrimInt, ToPrimitive, WrappingAdd, WrappingMul, WrappingSub, Zero}; use crate::enums::operators::ArithmeticOperator; +use crate::kernels::arithmetic::std::wrapping_pow_u32; use crate::kernels::bitmask::simd::all_true_mask_simd; use crate::utils::{simd_mask, write_simd_mask_bits}; /// SIMD integer arithmetic kernel for dense arrays (no nulls). /// Vectorised operations with scalar fallback for power operations and array tails. -/// Panics on division/remainder by zero (consistent with scalar behaviour). +/// Panics on division/remainder by zero, or on a `Power` exponent outside `u32` +/// range (consistent with scalar behaviour). #[inline(always)] pub fn int_dense_body_simd( op: ArithmeticOperator, @@ -57,7 +59,15 @@ pub fn int_dense_body_simd( rhs: &[T], out: &mut [T], ) where - T: Copy + One + PrimInt + ToPrimitive + Zero + SimdElement + WrappingMul, + T: Copy + + One + + PrimInt + + ToPrimitive + + Zero + + SimdElement + + WrappingAdd + + WrappingMul + + WrappingSub, Simd: Add> + Sub> + Mul> @@ -65,8 +75,12 @@ pub fn int_dense_body_simd( + Rem>, { let n = lhs.len(); + // The scalar tail takes its own closure because SIMD integer lanes wrap + // by definition while the raw scalar operators panic on overflow in a + // debug build, so the tail wraps through the same wrapping calls as the + // scalar kernels to keep one overflow policy across the whole input. macro_rules! run { - ($vec_op:tt) => {{ + ($vec_op:tt, $scalar:expr) => {{ let vectorisable = n / LANES * LANES; let mut i = 0; while i < vectorisable { @@ -77,31 +91,32 @@ pub fn int_dense_body_simd( } // Scalar tail for idx in vectorisable..n { - out[idx] = lhs[idx] $vec_op rhs[idx]; + out[idx] = $scalar(lhs[idx], rhs[idx]); } }}; } match op { - ArithmeticOperator::Add => run!(+), - ArithmeticOperator::Subtract => run!(-), - ArithmeticOperator::Multiply => run!(*), - ArithmeticOperator::Divide => run!(/), // Panics if divisor is zero - ArithmeticOperator::Remainder => run!(%), // Panics if divisor is zero - // Power and floor division run per element on the whole input. + ArithmeticOperator::Add => run!(+, |x: T, y: T| x.wrapping_add(&y)), + ArithmeticOperator::Subtract => run!(-, |x: T, y: T| x.wrapping_sub(&y)), + ArithmeticOperator::Multiply => run!(*, |x: T, y: T| x.wrapping_mul(&y)), + ArithmeticOperator::Remainder => run!(%, |x: T, y: T| x % y), // Panics if divisor is zero + // Power and division run per element on the whole input. There is no + // hardware SIMD integer divide, so the per-element loop costs nothing + // over the lane form. Integer division rounds towards negative + // infinity, so Divide and FloorDiv share one arm. Panics if the + // divisor is zero. ArithmeticOperator::Power => { for idx in 0..n { - let mut acc = T::one(); - let exp = rhs[idx].to_u32().unwrap_or(0); - for _ in 0..exp { - acc = acc.wrapping_mul(&lhs[idx]); - } - out[idx] = acc; + out[idx] = match rhs[idx].to_u32() { + Some(e) => wrapping_pow_u32(lhs[idx], e), + None => panic!("Power exponent out of u32 range"), + }; } } - ArithmeticOperator::FloorDiv => { + ArithmeticOperator::Divide | ArithmeticOperator::FloorDiv => { for idx in 0..n { out[idx] = if rhs[idx] == T::zero() { - panic!("Floor division by zero") + panic!("Division by zero") } else { let d = lhs[idx] / rhs[idx]; let r = lhs[idx] % rhs[idx]; @@ -117,7 +132,8 @@ pub fn int_dense_body_simd( } /// SIMD integer arithmetic kernel with null mask support. -/// Division/remainder by zero produces null results (mask=false) rather than panicking. +/// Division/remainder by zero, and a `Power` exponent outside `u32` range, +/// produce null results (mask=false) rather than panicking. #[inline(always)] pub fn int_masked_body_simd( op: ArithmeticOperator, @@ -192,27 +208,34 @@ pub fn int_masked_body_simd( ArithmeticOperator::Add => run!(+), ArithmeticOperator::Subtract => run!(-), ArithmeticOperator::Multiply => run!(*), - ArithmeticOperator::Divide => run_div!(/), ArithmeticOperator::Remainder => run_div!(%), + // An exponent outside u32 range nulls its lane. ArithmeticOperator::Power => { let mut i = 0; while i < vectorisable { let a = Simd::::from_slice(&lhs[i..i + LANES]); let b = Simd::::from_slice(&rhs[i..i + LANES]); let mut tmp = [T::zero(); LANES]; + let mut exp_ok = [false; LANES]; for l in 0..LANES { - tmp[l] = a[l].pow(b[l].to_u32().unwrap_or(0)); + if let Some(e) = b[l].to_u32() { + tmp[l] = wrapping_pow_u32(a[l], e); + exp_ok[l] = true; + } } Simd::::from_array(tmp).copy_to_slice(&mut out[i..i + LANES]); write_simd_mask_bits( out_mask, i, - Mask::<::Mask, LANES>::splat(true), + Mask::<::Mask, LANES>::from_array(exp_ok), ); i += LANES; } } - ArithmeticOperator::FloorDiv => { + // Integer `/` truncates toward zero, so this arm corrects the + // result to floor towards negative infinity, giving Divide and + // FloorDiv the same result. A zero divisor nulls its lane. + ArithmeticOperator::Divide | ArithmeticOperator::FloorDiv => { let mut i = 0; while i < vectorisable { let a = Simd::::from_slice(&lhs[i..i + LANES]); @@ -259,29 +282,35 @@ pub fn int_masked_body_simd( } } ArithmeticOperator::Power => { - out[idx] = lhs[idx].pow(rhs[idx].to_u32().unwrap_or(0)); - unsafe { - out_mask.set_unchecked(idx, true); + if let Some(e) = rhs[idx].to_u32() { + out[idx] = wrapping_pow_u32(lhs[idx], e); + unsafe { + out_mask.set_unchecked(idx, true); + } + } else { + out[idx] = T::zero(); + unsafe { + out_mask.set_unchecked(idx, false); + } } } - ArithmeticOperator::Divide | ArithmeticOperator::Remainder => { + ArithmeticOperator::Remainder => { if rhs[idx] == T::zero() { out[idx] = T::zero(); unsafe { out_mask.set_unchecked(idx, false); } } else { - out[idx] = match op { - ArithmeticOperator::Divide => lhs[idx] / rhs[idx], - ArithmeticOperator::Remainder => lhs[idx] % rhs[idx], - _ => unreachable!(), - }; + out[idx] = lhs[idx] % rhs[idx]; unsafe { out_mask.set_unchecked(idx, true); } } } - ArithmeticOperator::FloorDiv => { + // Integer `/` truncates toward zero, so this arm corrects + // the result to floor towards negative infinity, giving + // Divide and FloorDiv the same result. + ArithmeticOperator::Divide | ArithmeticOperator::FloorDiv => { if rhs[idx] == T::zero() { out[idx] = T::zero(); unsafe { @@ -339,8 +368,8 @@ pub fn int_masked_body_simd( ArithmeticOperator::Add => run!(+), ArithmeticOperator::Subtract => run!(-), ArithmeticOperator::Multiply => run!(*), - ArithmeticOperator::Divide => run_div!(/), ArithmeticOperator::Remainder => run_div!(%), + // An exponent outside u32 range nulls its lane. ArithmeticOperator::Power => { while i + LANES <= n { let a = Simd::::from_slice(&lhs[i..i + LANES]); @@ -348,17 +377,25 @@ pub fn int_masked_body_simd( let m_src: Mask<::Mask, LANES> = simd_mask(mask, i, n); // scalar per-lane power let mut tmp = [T::zero(); LANES]; + let mut exp_ok = [false; LANES]; for l in 0..LANES { - tmp[l] = a[l].pow(b[l].to_u32().unwrap_or(0)); + if let Some(e) = b[l].to_u32() { + tmp[l] = wrapping_pow_u32(a[l], e); + exp_ok[l] = true; + } } + let exp_ok = Mask::<::Mask, LANES>::from_array(exp_ok); let selected = m_src.select(Simd::::from_array(tmp), Simd::splat(T::zero())); selected.copy_to_slice(&mut out[i..i + LANES]); - write_simd_mask_bits(out_mask, i, m_src); + write_simd_mask_bits(out_mask, i, m_src & exp_ok); i += LANES; } } - ArithmeticOperator::FloorDiv => { + // Integer `/` truncates toward zero, so this arm corrects the + // result to floor towards negative infinity, giving Divide and + // FloorDiv the same result. A zero divisor nulls its lane. + ArithmeticOperator::Divide | ArithmeticOperator::FloorDiv => { while i + LANES <= n { let a = Simd::::from_slice(&lhs[i..i + LANES]); let b = Simd::::from_slice(&rhs[i..i + LANES]); @@ -394,13 +431,6 @@ pub fn int_masked_body_simd( ArithmeticOperator::Add => (lhs[j].wrapping_add(&rhs[j]), true), ArithmeticOperator::Subtract => (lhs[j].wrapping_sub(&rhs[j]), true), ArithmeticOperator::Multiply => (lhs[j].wrapping_mul(&rhs[j]), true), - ArithmeticOperator::Divide => { - if rhs[j] == T::zero() { - (T::zero(), false) // division by zero -> invalid - } else { - (lhs[j] / rhs[j], true) - } - } ArithmeticOperator::Remainder => { if rhs[j] == T::zero() { (T::zero(), false) // remainder by zero -> invalid @@ -408,10 +438,16 @@ pub fn int_masked_body_simd( (lhs[j] % rhs[j], true) } } - ArithmeticOperator::Power => (lhs[j].pow(rhs[j].to_u32().unwrap_or(0)), true), - ArithmeticOperator::FloorDiv => { + ArithmeticOperator::Power => match rhs[j].to_u32() { + Some(e) => (wrapping_pow_u32(lhs[j], e), true), + None => (T::zero(), false), // exponent out of u32 range -> null + }, + // Integer `/` truncates toward zero, so this arm corrects + // the result to floor towards negative infinity, giving + // Divide and FloorDiv the same result. + ArithmeticOperator::Divide | ArithmeticOperator::FloorDiv => { if rhs[j] == T::zero() { - (T::zero(), false) + (T::zero(), false) // division by zero -> invalid } else { let d = lhs[j] / rhs[j]; let r = lhs[j] % rhs[j]; diff --git a/src/kernels/arithmetic/std.rs b/src/kernels/arithmetic/std.rs index 287ee9d..09b0e7c 100644 --- a/src/kernels/arithmetic/std.rs +++ b/src/kernels/arithmetic/std.rs @@ -30,15 +30,38 @@ //! branch-free loop per operation //! - Intentionally avoids parallelisation to allow higher-level chunking strategies //! - Wrapping arithmetic for integers to prevent overflow panics +//! - `Divide` and `FloorDiv` share one arm: integer `/` truncates toward zero, +//! so the result is corrected to floor towards negative infinity //! - Division by zero handling: panics for integers, produces Inf/NaN for floats use crate::Bitmask; use crate::enums::operators::ArithmeticOperator; use num_traits::{Float, PrimInt, ToPrimitive, WrappingAdd, WrappingMul, WrappingSub}; +/// Wrapping integer exponentiation by squaring. +/// +/// Overflow wraps modulo the lane width, matching the wrapping +/// add/subtract/multiply arms, so a large power never panics. Wrapping +/// multiplication is associative modulo `2^n`, so squaring produces the same +/// result as repeated multiplication. +#[inline(always)] +pub fn wrapping_pow_u32(base: T, exp: u32) -> T { + let mut acc = T::one(); + let mut base = base; + let mut exp = exp; + while exp > 0 { + if exp & 1 == 1 { + acc = acc.wrapping_mul(&base); + } + base = base.wrapping_mul(&base); + exp >>= 1; + } + acc +} + /// Scalar integer arithmetic kernel for dense arrays (no nulls). /// Performs element-wise operations using wrapping arithmetic to prevent overflow panics. -/// Panics on division/remainder by zero. +/// Panics on division/remainder by zero, or on a `Power` exponent outside `u32` range. #[inline(always)] pub fn int_dense_body_std( op: ArithmeticOperator, @@ -60,24 +83,12 @@ pub fn int_dense_body_std run!(|x, y| x.wrapping_add(&y)), ArithmeticOperator::Subtract => run!(|x, y| x.wrapping_sub(&y)), ArithmeticOperator::Multiply => run!(|x, y| x.wrapping_mul(&y)), - ArithmeticOperator::Divide => run!(|x, y| { + // Integer `/` truncates toward zero, so this arm corrects the result + // to floor towards negative infinity, giving Divide and FloorDiv the + // same result and letting them share one arm. + ArithmeticOperator::Divide | ArithmeticOperator::FloorDiv => run!(|x, y| { if y == T::zero() { panic!("Division by zero") - } else { - x / y - } - }), - ArithmeticOperator::Remainder => run!(|x, y| { - if y == T::zero() { - panic!("Remainder by zero") - } else { - x % y - } - }), - ArithmeticOperator::Power => run!(|x, y| x.pow(y.to_u32().unwrap_or(0))), - ArithmeticOperator::FloorDiv => run!(|x, y| { - if y == T::zero() { - panic!("Floor division by zero") } else { let d = x / y; let r = x % y; @@ -89,12 +100,24 @@ pub fn int_dense_body_std run!(|x, y| { + if y == T::zero() { + panic!("Remainder by zero") + } else { + x % y + } + }), + ArithmeticOperator::Power => run!(|x, y| match y.to_u32() { + Some(e) => wrapping_pow_u32(x, e), + None => panic!("Power exponent out of u32 range"), + }), } } /// Scalar integer arithmetic kernel with null mask support. /// Handles division by zero gracefully by marking results as null instead of panicking. -/// Invalid inputs (mask=false) and zero division produce null outputs. +/// A cleared input bit, a zero divisor, and a `Power` exponent outside `u32` +/// range all produce a null output rather than panicking. #[inline(always)] pub fn int_masked_body_std( op: ArithmeticOperator, @@ -130,11 +153,20 @@ pub fn int_masked_body_std run!(|x, y| (x.wrapping_add(&y), true)), ArithmeticOperator::Subtract => run!(|x, y| (x.wrapping_sub(&y), true)), ArithmeticOperator::Multiply => run!(|x, y| (x.wrapping_mul(&y), true)), - ArithmeticOperator::Divide => run!(|x, y| { + // Integer `/` truncates toward zero, so this arm corrects the result + // to floor towards negative infinity, giving Divide and FloorDiv the + // same result and letting them share one arm. + ArithmeticOperator::Divide | ArithmeticOperator::FloorDiv => run!(|x, y| { if y == T::zero() { (T::zero(), false) // division by zero -> invalid } else { - (x / y, true) + let d = x / y; + let r = x % y; + if r != T::zero() && (x ^ y) < T::zero() { + (d - T::one(), true) + } else { + (d, true) + } } }), ArithmeticOperator::Remainder => run!(|x, y| { @@ -144,19 +176,9 @@ pub fn int_masked_body_std run!(|x, y| (x.pow(y.to_u32().unwrap_or(0)), true)), - ArithmeticOperator::FloorDiv => run!(|x, y| { - if y == T::zero() { - (T::zero(), false) - } else { - let d = x / y; - let r = x % y; - if r != T::zero() && (x ^ y) < T::zero() { - (d - T::one(), true) - } else { - (d, true) - } - } + ArithmeticOperator::Power => run!(|x, y| match y.to_u32() { + Some(e) => (wrapping_pow_u32(x, e), true), + None => (T::zero(), false), // exponent out of u32 range -> null }), } } diff --git a/src/kernels/routing/arithmetic.rs b/src/kernels/routing/arithmetic.rs index b80aaac..68662e9 100644 --- a/src/kernels/routing/arithmetic.rs +++ b/src/kernels/routing/arithmetic.rs @@ -44,13 +44,15 @@ pub fn scalar_arithmetic( (Scalar::Int32(l), Scalar::Int32(r), Add) => Scalar::Int32(l + r), (Scalar::Int32(l), Scalar::Int32(r), Subtract) => Scalar::Int32(l - r), (Scalar::Int32(l), Scalar::Int32(r), Multiply) => Scalar::Int32(l * r), - (Scalar::Int32(l), Scalar::Int32(r), Divide) => Scalar::Int32(l / r), + // Integer scalar division is true division and returns a Float64 + // scalar, so 7 / 2 is 3.5 and division by zero follows IEEE 754. + (Scalar::Int32(l), Scalar::Int32(r), Divide) => Scalar::Float64(l as f64 / r as f64), // Int64 operations (Scalar::Int64(l), Scalar::Int64(r), Add) => Scalar::Int64(l + r), (Scalar::Int64(l), Scalar::Int64(r), Subtract) => Scalar::Int64(l - r), (Scalar::Int64(l), Scalar::Int64(r), Multiply) => Scalar::Int64(l * r), - (Scalar::Int64(l), Scalar::Int64(r), Divide) => Scalar::Int64(l / r), + (Scalar::Int64(l), Scalar::Int64(r), Divide) => Scalar::Float64(l as f64 / r as f64), // Float32 operations (Scalar::Float32(l), Scalar::Float32(r), Add) => Scalar::Float32(l + r), @@ -86,7 +88,7 @@ pub fn scalar_arithmetic( #[cfg(feature = "extended_numeric_types")] (Scalar::Int8(l), Scalar::Int8(r), Multiply) => Scalar::Int8(l * r), #[cfg(feature = "extended_numeric_types")] - (Scalar::Int8(l), Scalar::Int8(r), Divide) => Scalar::Int8(l / r), + (Scalar::Int8(l), Scalar::Int8(r), Divide) => Scalar::Float64(l as f64 / r as f64), // Int16 #[cfg(feature = "extended_numeric_types")] @@ -96,7 +98,7 @@ pub fn scalar_arithmetic( #[cfg(feature = "extended_numeric_types")] (Scalar::Int16(l), Scalar::Int16(r), Multiply) => Scalar::Int16(l * r), #[cfg(feature = "extended_numeric_types")] - (Scalar::Int16(l), Scalar::Int16(r), Divide) => Scalar::Int16(l / r), + (Scalar::Int16(l), Scalar::Int16(r), Divide) => Scalar::Float64(l as f64 / r as f64), // UInt8 #[cfg(feature = "extended_numeric_types")] @@ -106,7 +108,7 @@ pub fn scalar_arithmetic( #[cfg(feature = "extended_numeric_types")] (Scalar::UInt8(l), Scalar::UInt8(r), Multiply) => Scalar::UInt8(l * r), #[cfg(feature = "extended_numeric_types")] - (Scalar::UInt8(l), Scalar::UInt8(r), Divide) => Scalar::UInt8(l / r), + (Scalar::UInt8(l), Scalar::UInt8(r), Divide) => Scalar::Float64(l as f64 / r as f64), // UInt16 #[cfg(feature = "extended_numeric_types")] @@ -116,19 +118,19 @@ pub fn scalar_arithmetic( #[cfg(feature = "extended_numeric_types")] (Scalar::UInt16(l), Scalar::UInt16(r), Multiply) => Scalar::UInt16(l * r), #[cfg(feature = "extended_numeric_types")] - (Scalar::UInt16(l), Scalar::UInt16(r), Divide) => Scalar::UInt16(l / r), + (Scalar::UInt16(l), Scalar::UInt16(r), Divide) => Scalar::Float64(l as f64 / r as f64), // UInt32 (Scalar::UInt32(l), Scalar::UInt32(r), Add) => Scalar::UInt32(l + r), (Scalar::UInt32(l), Scalar::UInt32(r), Subtract) => Scalar::UInt32(l - r), (Scalar::UInt32(l), Scalar::UInt32(r), Multiply) => Scalar::UInt32(l * r), - (Scalar::UInt32(l), Scalar::UInt32(r), Divide) => Scalar::UInt32(l / r), + (Scalar::UInt32(l), Scalar::UInt32(r), Divide) => Scalar::Float64(l as f64 / r as f64), // UInt64 (Scalar::UInt64(l), Scalar::UInt64(r), Add) => Scalar::UInt64(l + r), (Scalar::UInt64(l), Scalar::UInt64(r), Subtract) => Scalar::UInt64(l - r), (Scalar::UInt64(l), Scalar::UInt64(r), Multiply) => Scalar::UInt64(l * r), - (Scalar::UInt64(l), Scalar::UInt64(r), Divide) => Scalar::UInt64(l / r), + (Scalar::UInt64(l), Scalar::UInt64(r), Divide) => Scalar::Float64(l as f64 / r as f64), // String concatenation (Scalar::String32(l), Scalar::String32(r), Add) => Scalar::String32(format!("{}{}", l, r)), #[cfg(feature = "large_string")] diff --git a/src/structs/views/bitmask_view.rs b/src/structs/views/bitmask_view.rs index a1086a7..5403e93 100644 --- a/src/structs/views/bitmask_view.rs +++ b/src/structs/views/bitmask_view.rs @@ -82,7 +82,7 @@ use crate::{Array, Bitmask, BitmaskVT, BooleanArray}; /// assert!(view.get(1)); /// assert!(view.get(2)); /// ``` -#[derive(Clone, PartialEq)] +#[derive(Clone, Copy, PartialEq)] pub struct BitmaskV<'a> { /// The **outer bitmask** that this view is derived from - we retain a reference to it. /// Importantly, this is the ***full bitmask*** - not the *view*, and thus should not be @@ -133,6 +133,21 @@ impl<'a> BitmaskV<'a> { self.bitmask.get(self.offset + i) } + /// Returns the value at logical index `i` within the view, skipping the + /// bounds check. + /// + /// Element-wise kernels read a flagged null_mask (valid) bit per row, so the checked + /// [`get`](Self::get) costs a comparison in the innermost loop. This is + /// the unchecked counterpart for loops that have already established + /// `i < len`. + /// + /// # Safety + /// `i` must be less than the view's length. + #[inline] + pub unsafe fn get_unchecked(&self, i: usize) -> bool { + unsafe { self.bitmask.get_unchecked(self.offset + i) } + } + /// Returns a slice of the bitmask’s bytes /// Due to the booleans being bitpacked in a u8, /// the slice retains:\ diff --git a/src/structs/views/collections/numeric_array_view.rs b/src/structs/views/collections/numeric_array_view.rs index 4beea63..8032f58 100644 --- a/src/structs/views/collections/numeric_array_view.rs +++ b/src/structs/views/collections/numeric_array_view.rs @@ -52,7 +52,7 @@ use crate::structs::views::bitmask_view::BitmaskV; use crate::traits::concatenate::Concatenate; use crate::traits::print::MAX_PREVIEW; use crate::traits::shape::Shape; -use crate::{Array, ArrayV, Bitmask, FieldArray, MaskedArray, NumericArray}; +use crate::{Array, ArrayV, FieldArray, MaskedArray, NumericArray}; /// # NumericArrayView /// @@ -301,23 +301,24 @@ impl NumericArrayV { /// that still reference the original will cast independently when they /// reach this call, so it generally is best avoided in such contexts as it would /// clone for every independent window view. - pub fn guarantee_f64(&mut self) -> (&[f64], Option<&Bitmask>, Option) { + pub fn guarantee_f64(&mut self) -> (&[f64], Option>, Option) { if !matches!(&self.array, NumericArray::Float64(_)) { // Take the old array out, leaving Null as placeholder let old = std::mem::take(&mut self.array); self.array = old.cow_into_f64(); } - // Safe: the branch above guarantees Float64 at this point - let NumericArray::Float64(arr) = &self.array else { - unreachable!() - }; - let slice = &arr.data.as_slice()[self.offset..self.offset + self.len]; - let mask = arr.null_mask.as_ref(); - let nc = if mask.is_some() { + let nc = if self.array.null_mask().is_some() { Some(self.null_count()) } else { None }; + let (offset, len) = (self.offset, self.len); + // Safe: the branch above guarantees Float64 at this point + let NumericArray::Float64(arr) = &self.array else { + unreachable!() + }; + let slice = &arr.data.as_slice()[offset..offset + len]; + let mask = arr.null_mask.as_ref().map(|m| m.view(offset, len)); (slice, mask, nc) } } @@ -470,7 +471,52 @@ mod tests { use std::sync::Arc; use super::*; - use crate::{Array, Bitmask, IntegerArray, NumericArray, vec64}; + use crate::{Array, Bitmask, FloatArray, IntegerArray, NumericArray, vec64}; + + #[test] + fn guarantee_f64_windows_the_null_mask_with_the_slice() { + let mut arr = FloatArray::::default(); + for v in [10.0, 20.0, 30.0, 40.0, 50.0] { + arr.push(v); + } + let mut mask = Bitmask::new_set_all(5, true); + mask.set(0, false); + mask.set(1, false); + arr.null_mask = Some(mask); + + let mut view = NumericArrayV::new(NumericArray::Float64(Arc::new(arr)), 2, 3); + let (slice, mask, nc) = view.guarantee_f64(); + + assert_eq!(slice, &[30.0, 40.0, 50.0]); + let mask = mask.expect("the backing array carries a null mask"); + assert_eq!(mask.len(), 3); + // Reading the parent's bits from zero would report the first two + // window rows as null. + assert!((0..3).all(|i| mask.get(i)), "every window row is valid"); + assert_eq!(nc, Some(0)); + } + + #[test] + fn guarantee_f64_windows_the_null_mask_after_casting() { + let mut arr = IntegerArray::::default(); + for v in [1, 2, 3, 4, 5] { + arr.push(v); + } + let mut mask = Bitmask::new_set_all(5, true); + mask.set(1, false); + mask.set(3, false); + arr.null_mask = Some(mask); + + let mut view = NumericArrayV::new(NumericArray::Int32(Arc::new(arr)), 2, 3); + let (slice, mask, _) = view.guarantee_f64(); + + assert_eq!(slice, &[3.0, 4.0, 5.0]); + let mask = mask.expect("the backing array carries a null mask"); + // Window row 1 is parent row 3, the null one. + assert!(mask.get(0)); + assert!(!mask.get(1)); + assert!(mask.get(2)); + } #[test] fn test_numeric_array_view_basic_indexing_and_slice() {