Merge branch 'develop' into ji/decimal-builder-decimal-dtype

joseph-isaacs · joseph-isaacs · commit ab9d135f8845 · 2025-11-03T14:03:34.000Z
# Conflicts:
#	vortex-array/src/arrays/arbitrary.rs
diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml
@@ -8,10 +8,10 @@ on:
 jobs:
   io_fuzz:
     name: "IO Fuzz"
-    timeout-minutes: 180
+    timeout-minutes: 230  # almost 4 hours
     runs-on:
       - runs-on=${{ github.run_id }}
-      - family=m8g.2xlarge
+      - family=m8g.large
       - image=ubuntu24-full-arm64
       - disk=large
       - extras=s3-cache
@@ -44,7 +44,7 @@ jobs:
           AWS_ENDPOINT_URL: "https://01e9655179bbec953276890b183039bc.r2.cloudflarestorage.com"
       - name: Run fuzzing target
         id: fuzz
-        run: RUST_BACKTRACE=1 cargo +nightly fuzz run file_io -j "$(nproc)" -- -max_total_time=7200
+        run: RUST_BACKTRACE=1 cargo +nightly fuzz run --release --debug-assertions file_io -- -max_total_time=7200
         continue-on-error: true
       - name: Archive crash artifacts
         uses: actions/upload-artifact@v4
@@ -67,10 +67,10 @@ jobs:
 
   ops_fuzz:
     name: "Array Operations Fuzz"
-    timeout-minutes: 180
+    timeout-minutes: 230  # almost 4 hours
     runs-on:
       - runs-on=${{ github.run_id }}
-      - family=m8g.2xlarge
+      - family=m8g.large
       - image=ubuntu24-full-arm64
       - disk=large
       - extras=s3-cache
@@ -103,7 +103,7 @@ jobs:
           AWS_ENDPOINT_URL: "https://01e9655179bbec953276890b183039bc.r2.cloudflarestorage.com"
       - name: Run fuzzing target
         id: fuzz
-        run: RUST_BACKTRACE=1 cargo +nightly fuzz run array_ops -j "$(nproc)" -- -max_total_time=7200
+        run: RUST_BACKTRACE=1 cargo +nightly fuzz run --release --debug-assertions array_ops -- -max_total_time=7200
         continue-on-error: true
       - name: Archive crash artifacts
         uses: actions/upload-artifact@v4
diff --git a/encodings/sparse/src/canonical.rs b/encodings/sparse/src/canonical.rs
@@ -8,7 +8,7 @@ use num_traits::NumCast;
 use vortex_array::arrays::binary_view::BinaryView;
 use vortex_array::arrays::{
     BoolArray, ConstantArray, FixedSizeListArray, ListViewArray, NullArray, PrimitiveArray,
-    StructArray, VarBinViewArray, smallest_decimal_value_type,
+    StructArray, VarBinViewArray,
 };
 use vortex_array::builders::{
     ArrayBuilder, DecimalBuilder, ListViewBuilder, builder_with_capacity,
@@ -19,8 +19,8 @@ use vortex_array::vtable::{CanonicalVTable, ValidityHelper};
 use vortex_array::{Array, Canonical, ToCanonical};
 use vortex_buffer::{BitBuffer, Buffer, BufferString, ByteBuffer, buffer, buffer_mut};
 use vortex_dtype::{
-    DType, DecimalDType, IntegerPType, NativeDecimalType, NativePType, Nullability, StructFields,
-    match_each_integer_ptype, match_each_native_ptype,
+    DType, DecimalDType, DecimalType, IntegerPType, NativeDecimalType, NativePType, Nullability,
+    StructFields, match_each_integer_ptype, match_each_native_ptype,
 };
 use vortex_error::{VortexError, VortexExpect, vortex_panic};
 use vortex_scalar::{
@@ -58,7 +58,8 @@ impl CanonicalVTable<SparseVTable> for SparseVTable {
                 array.len(),
             ),
             DType::Decimal(decimal_dtype, nullability) => {
-                let canonical_decimal_value_type = smallest_decimal_value_type(decimal_dtype);
+                let canonical_decimal_value_type =
+                    DecimalType::smallest_decimal_value_type(decimal_dtype);
                 let fill_value = array.fill_scalar().as_decimal();
                 match_each_decimal_value_type!(canonical_decimal_value_type, |D| {
                     canonicalize_sparse_decimal::<D>(
diff --git a/fuzz/src/array/mod.rs b/fuzz/src/array/mod.rs
@@ -381,8 +381,8 @@ fn actions_for_dtype(dtype: &DType) -> HashSet<ActionType> {
             [Compress, Slice, Take, Filter, MinMax, Mask, ScalarAt].into()
         }
         DType::Utf8(_) | DType::Binary(_) => {
-            // Utf8/Binary supports everything except Sum
-            // Actions: Compress, Slice, Take, SearchSorted, Filter, Compare, Cast, MinMax, FillNull, Mask, ScalarAt
+            // Utf8/Binary supports everything except Sum and FillNull
+            // Actions: Compress, Slice, Take, SearchSorted, Filter, Compare, Cast, MinMax, Mask, ScalarAt
             [
                 Compress,
                 Slice,
@@ -392,7 +392,6 @@ fn actions_for_dtype(dtype: &DType) -> HashSet<ActionType> {
                 Compare,
                 Cast,
                 MinMax,
-                FillNull,
                 Mask,
                 ScalarAt,
             ]
diff --git a/fuzz/src/array/scalar_at.rs b/fuzz/src/array/scalar_at.rs
@@ -13,6 +13,9 @@ use vortex_scalar::{DecimalValue, Scalar, match_each_decimal_value_type};
 /// This implementation manually extracts the scalar value from each canonical type
 /// without using the scalar_at method, to serve as an independent baseline for testing.
 pub fn scalar_at_canonical_array(canonical: Canonical, index: usize) -> VortexResult<Scalar> {
+    if canonical.as_ref().is_invalid(index) {
+        return Ok(Scalar::null(canonical.as_ref().dtype().clone()));
+    }
     Ok(match canonical {
         Canonical::Null(_array) => Scalar::null(DType::Null),
         Canonical::Bool(array) => {
diff --git a/vortex-array/src/arrays/arbitrary.rs b/vortex-array/src/arrays/arbitrary.rs
@@ -12,7 +12,7 @@ use vortex_scalar::arbitrary::random_scalar;
 use vortex_scalar::{Scalar, match_each_decimal_value_type};
 
 use super::{BoolArray, ChunkedArray, NullArray, PrimitiveArray, StructArray};
-use crate::arrays::{VarBinArray, VarBinViewArray, smallest_decimal_value_type};
+use crate::arrays::{VarBinArray, VarBinViewArray};
 use crate::builders::{ArrayBuilder, DecimalBuilder, FixedSizeListBuilder, ListViewBuilder};
 use crate::validity::Validity;
 use crate::{Array, ArrayRef, IntoArray, ToCanonical};
@@ -95,16 +95,20 @@ fn random_array_chunk(
         },
         DType::Decimal(decimal, n) => {
             let elem_len = chunk_len.unwrap_or(u.int_in_range(0..=20)?);
-            match_each_decimal_value_type!(smallest_decimal_value_type(decimal), |DVT| {
-                let mut builder = DecimalBuilder::new::<DVT>(*decimal, *n);
-                for _i in 0..elem_len {
-                    let random_decimal = random_scalar(u, &DType::Decimal(*decimal, *n))?;
-                    builder.append_scalar(&random_decimal).vortex_expect(
-                        "was somehow unable to append a decimal to a decimal builder",
-                    );
+            match_each_decimal_value_type!(
+                DecimalType::smallest_decimal_value_type(decimal),
+                |DVT| {
+                    let mut builder =
+                        DecimalBuilder::new::<DVT>(decimal.precision(), decimal.scale(), *n);
+                    for _i in 0..elem_len {
+                        let random_decimal = random_scalar(u, &DType::Decimal(*decimal, *n))?;
+                        builder.append_scalar(&random_decimal).vortex_expect(
+                            "was somehow unable to append a decimal to a decimal builder",
+                        );
+                    }
+                    Ok(builder.finish())
                 }
-                Ok(builder.finish())
-            })
+            )
         }
         DType::Utf8(n) => random_string(u, *n, chunk_len),
         DType::Binary(n) => random_bytes(u, *n, chunk_len),
diff --git a/vortex-array/src/arrays/constant/vtable/canonical.rs b/vortex-array/src/arrays/constant/vtable/canonical.rs
@@ -4,7 +4,7 @@
 use std::sync::Arc;
 
 use vortex_buffer::{BitBuffer, Buffer, buffer};
-use vortex_dtype::{DType, Nullability, match_each_native_ptype};
+use vortex_dtype::{DType, DecimalType, Nullability, match_each_native_ptype};
 use vortex_error::VortexExpect;
 use vortex_scalar::{
     BinaryScalar, BoolScalar, DecimalValue, ExtScalar, ListScalar, Scalar, StructScalar,
@@ -16,7 +16,7 @@ use crate::arrays::constant::ConstantArray;
 use crate::arrays::primitive::PrimitiveArray;
 use crate::arrays::{
     BoolArray, ConstantVTable, DecimalArray, ExtensionArray, FixedSizeListArray, ListViewArray,
-    NullArray, StructArray, VarBinViewArray, smallest_decimal_value_type,
+    NullArray, StructArray, VarBinViewArray,
 };
 use crate::builders::builder_with_capacity;
 use crate::validity::Validity;
@@ -66,7 +66,7 @@ impl CanonicalVTable<ConstantVTable> for ConstantVTable {
                 })
             }
             DType::Decimal(decimal_type, ..) => {
-                let size = smallest_decimal_value_type(decimal_type);
+                let size = DecimalType::smallest_decimal_value_type(decimal_type);
                 let decimal = scalar.as_decimal();
                 let Some(value) = decimal.decimal_value() else {
                     let all_null = match_each_decimal_value_type!(size, |D| {
diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs
@@ -11,7 +11,6 @@ use vortex_error::{VortexExpect, VortexResult, vortex_ensure, vortex_panic};
 use vortex_scalar::match_each_decimal_value_type;
 
 use crate::ToCanonical;
-use crate::arrays::is_compatible_decimal_value_type;
 use crate::patches::Patches;
 use crate::stats::ArrayStats;
 use crate::validity::Validity;
@@ -279,7 +278,7 @@ where
     PatchDVT: NativeDecimalType,
     ValuesDVT: NativeDecimalType,
 {
-    if !is_compatible_decimal_value_type(ValuesDVT::DECIMAL_TYPE, decimal_dtype) {
+    if !ValuesDVT::DECIMAL_TYPE.is_compatible_decimal_value_type(decimal_dtype) {
         vortex_panic!(
             "patch_typed: {:?} cannot represent every value in {}.",
             ValuesDVT::DECIMAL_TYPE,
diff --git a/vortex-array/src/arrays/decimal/compute/sum.rs b/vortex-array/src/arrays/decimal/compute/sum.rs
@@ -3,13 +3,13 @@
 
 use arrow_schema::DECIMAL256_MAX_PRECISION;
 use num_traits::AsPrimitive;
-use vortex_dtype::DecimalDType;
 use vortex_dtype::Nullability::Nullable;
+use vortex_dtype::{DecimalDType, DecimalType};
 use vortex_error::{VortexResult, vortex_bail};
 use vortex_mask::Mask;
 use vortex_scalar::{DecimalValue, Scalar, match_each_decimal_value_type};
 
-use crate::arrays::{DecimalArray, DecimalVTable, smallest_decimal_value_type};
+use crate::arrays::{DecimalArray, DecimalVTable};
 use crate::compute::{SumKernel, SumKernelAdapter};
 use crate::register_kernel;
 
@@ -54,7 +54,7 @@ impl SumKernel for DecimalVTable {
                 vortex_bail!("invalid state, all-null array should be checked by top-level sum fn")
             }
             Mask::AllTrue(_) => {
-                let values_type = smallest_decimal_value_type(&return_dtype);
+                let values_type = DecimalType::smallest_decimal_value_type(&return_dtype);
                 match_each_decimal_value_type!(array.values_type(), |I| {
                     match_each_decimal_value_type!(values_type, |O| {
                         Ok(Scalar::decimal(
@@ -66,7 +66,7 @@ impl SumKernel for DecimalVTable {
                 })
             }
             Mask::Values(mask_values) => {
-                let values_type = smallest_decimal_value_type(&return_dtype);
+                let values_type = DecimalType::smallest_decimal_value_type(&return_dtype);
                 match_each_decimal_value_type!(array.values_type(), |I| {
                     match_each_decimal_value_type!(values_type, |O| {
                         Ok(Scalar::decimal(
diff --git a/vortex-array/src/arrays/decimal/utils.rs b/vortex-array/src/arrays/decimal/utils.rs
@@ -2,32 +2,12 @@
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
 use itertools::{Itertools, MinMaxResult};
-use vortex_dtype::DecimalDType;
 use vortex_error::VortexExpect;
 use vortex_scalar::{DecimalType, i256};
 
 use crate::arrays::DecimalArray;
 use crate::vtable::ValidityHelper;
 
-/// Maps a decimal precision into the smallest type that can represent it.
-pub fn smallest_decimal_value_type(decimal_dtype: &DecimalDType) -> DecimalType {
-    match decimal_dtype.precision() {
-        1..=2 => DecimalType::I8,
-        3..=4 => DecimalType::I16,
-        5..=9 => DecimalType::I32,
-        10..=18 => DecimalType::I64,
-        19..=38 => DecimalType::I128,
-        39..=76 => DecimalType::I256,
-        0 => unreachable!("precision must be greater than 0"),
-        p => unreachable!("precision larger than 76 is invalid found precision {p}"),
-    }
-}
-
-/// True if `value_type` can represent every value of the type `dtype`.
-pub fn is_compatible_decimal_value_type(value_type: DecimalType, dtype: DecimalDType) -> bool {
-    value_type >= smallest_decimal_value_type(&dtype)
-}
-
 macro_rules! try_downcast {
     ($array:expr, from: $src:ty, to: $($dst:ty),*) => {{
         use vortex_dtype::BigCast;
diff --git a/vortex-array/src/builders/decimal.rs b/vortex-array/src/builders/decimal.rs
@@ -5,7 +5,9 @@ use std::any::Any;
 
 use vortex_buffer::BufferMut;
 use vortex_dtype::{BigCast, DType, DecimalDType, NativeDecimalType, Nullability};
-use vortex_error::{VortexExpect, VortexResult, vortex_ensure, vortex_panic};
+use vortex_error::{
+    VortexExpect, VortexResult, VortexUnwrap, vortex_ensure, vortex_err, vortex_panic,
+};
 use vortex_mask::Mask;
 use vortex_scalar::{
     DecimalValue, Scalar, i256, match_each_decimal_value, match_each_decimal_value_type,
@@ -208,7 +210,18 @@ impl ArrayBuilder for DecimalBuilder {
 impl DecimalBuffer {
     fn push<V: NativeDecimalType>(&mut self, value: V) {
         delegate_fn!(self, |T, buffer| {
-            buffer.push(<T as BigCast>::from(value).vortex_expect("decimal conversion failure"))
+            buffer.push(
+                <T as BigCast>::from(value)
+                    .ok_or_else(|| {
+                        vortex_err!(
+                            "decimal conversion failure {:?}, type: {:?} to {:?}",
+                            value,
+                            V::DECIMAL_TYPE,
+                            T::DECIMAL_TYPE,
+                        )
+                    })
+                    .vortex_unwrap(),
+            )
         });
     }
 
diff --git a/vortex-array/src/builders/mod.rs b/vortex-array/src/builders/mod.rs
@@ -35,7 +35,6 @@ use vortex_error::{VortexResult, vortex_panic};
 use vortex_mask::Mask;
 use vortex_scalar::{Scalar, match_each_decimal_value_type};
 
-use crate::arrays::smallest_decimal_value_type;
 use crate::canonical::Canonical;
 use crate::{Array, ArrayRef};
 
@@ -246,13 +245,16 @@ pub fn builder_with_capacity(dtype: &DType, capacity: usize) -> Box<dyn ArrayBui
             })
         }
         DType::Decimal(decimal_type, n) => {
-            match_each_decimal_value_type!(smallest_decimal_value_type(decimal_type), |D| {
-                Box::new(DecimalBuilder::with_capacity::<D>(
-                    capacity,
-                    *decimal_type,
-                    *n,
-                ))
-            })
+            match_each_decimal_value_type!(
+                DecimalType::smallest_decimal_value_type(decimal_type),
+                |D| {
+                    Box::new(DecimalBuilder::with_capacity::<D>(
+                        capacity,
+                        *decimal_type,
+                        *n,
+                    ))
+                }
+            )
         }
         DType::Utf8(n) => Box::new(VarBinViewBuilder::with_capacity(DType::Utf8(*n), capacity)),
         DType::Binary(n) => Box::new(VarBinViewBuilder::with_capacity(
diff --git a/vortex-array/src/compute/fill_null.rs b/vortex-array/src/compute/fill_null.rs
@@ -108,6 +108,7 @@ impl ComputeFnVTable for FillNull {
             return Ok(fill_null(canonical_arr.as_ref(), fill_value)?.into());
         }
 
+        // TODO(joe): update fuzzer when fixed
         vortex_bail!("fill null not implemented for DType {}", array.dtype())
     }
 
diff --git a/vortex-dtype/src/decimal/mod.rs b/vortex-dtype/src/decimal/mod.rs
@@ -6,11 +6,14 @@ mod precision;
 mod types;
 
 use std::fmt::{Display, Formatter};
+use std::num::NonZero;
 
 use num_traits::ToPrimitive;
 pub use precision::*;
 pub use types::*;
-use vortex_error::{VortexError, VortexExpect, VortexResult, vortex_bail, vortex_panic};
+use vortex_error::{
+    VortexError, VortexExpect, VortexResult, vortex_bail, vortex_err, vortex_panic,
+};
 
 use crate::{DType, i256};
 
@@ -20,10 +23,11 @@ const MAX_SCALE: i8 = <i256 as NativeDecimalType>::MAX_SCALE;
 /// Parameters that define the precision and scale of a decimal type.
 ///
 /// Decimal types allow real numbers with a similar precision and scale to be represented exactly.
+/// Precision must be non-zero.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct DecimalDType {
-    precision: u8,
+    precision: NonZero<u8>,
     scale: i8,
 }
 
@@ -34,13 +38,14 @@ impl DecimalDType {
     ///
     /// Returns an error if precision exceeds MAX_PRECISION or scale is outside [MIN_SCALE, MAX_SCALE].
     pub fn try_new(precision: u8, scale: i8) -> VortexResult<Self> {
-        if precision == 0 {
-            vortex_bail!(
+        let precision = NonZero::new(precision).ok_or_else(|| {
+            vortex_err!(
                 "decimal precision must be between 1 and {} (inclusive)",
                 MAX_PRECISION
-            );
-        }
-        if precision > MAX_PRECISION {
+            )
+        })?;
+
+        if precision.get() > MAX_PRECISION {
             vortex_bail!(
                 "decimal precision {} exceeds MAX_PRECISION {}",
                 precision,
@@ -52,7 +57,7 @@ impl DecimalDType {
             vortex_bail!("decimal scale {} exceeds MAX_SCALE {}", scale, MAX_SCALE);
         }
 
-        if scale > 0 && scale as u8 > precision {
+        if scale > 0 && scale as u8 > precision.get() {
             vortex_bail!(
                 "decimal scale {} is greater than precision {}",
                 scale,
@@ -76,7 +81,7 @@ impl DecimalDType {
 
     /// The precision is the number of significant figures that the decimal tracks.
     pub fn precision(&self) -> u8 {
-        self.precision
+        self.precision.get()
     }
 
     /// The scale is the maximum number of digits relative to the decimal point.
@@ -89,7 +94,7 @@ impl DecimalDType {
 
     /// Return the max number of bits required to fit a decimal with `precision` in.
     pub fn required_bit_width(&self) -> usize {
-        (self.precision as f32 * 10.0f32.log(2.0))
+        (self.precision.get() as f32 * 10.0f32.log(2.0))
             .ceil()
             .to_usize()
             .vortex_expect("too many bits required")
diff --git a/vortex-dtype/src/decimal/precision.rs b/vortex-dtype/src/decimal/precision.rs
diff --git a/vortex-dtype/src/decimal/types.rs b/vortex-dtype/src/decimal/types.rs
diff --git a/vortex-scalar/src/arbitrary.rs b/vortex-scalar/src/arbitrary.rs
diff --git a/vortex-scalar/src/decimal/macros.rs b/vortex-scalar/src/decimal/macros.rs

Original file line number	Diff line number	Diff line change
`@@ -381,8 +381,8 @@ fn actions_for_dtype(dtype: &DType) -> HashSet<ActionType> {`
`381`	`381`	`[Compress, Slice, Take, Filter, MinMax, Mask, ScalarAt].into()`
`382`	`382`	`}`
`383`	`383`	`DType::Utf8(_) \| DType::Binary(_) => {`
`384`		`- // Utf8/Binary supports everything except Sum`
`385`		`- // Actions: Compress, Slice, Take, SearchSorted, Filter, Compare, Cast, MinMax, FillNull, Mask, ScalarAt`
	`384`	`+ // Utf8/Binary supports everything except Sum and FillNull`
	`385`	`+ // Actions: Compress, Slice, Take, SearchSorted, Filter, Compare, Cast, MinMax, Mask, ScalarAt`
`386`	`386`	`[`
`387`	`387`	`Compress,`
`388`	`388`	`Slice,`
`@@ -392,7 +392,6 @@ fn actions_for_dtype(dtype: &DType) -> HashSet<ActionType> {`
`392`	`392`	`Compare,`
`393`	`393`	`Cast,`
`394`	`394`	`MinMax,`
`395`		`- FillNull,`
`396`	`395`	`Mask,`
`397`	`396`	`ScalarAt,`
`398`	`397`	`]`