Skip to content

Commit a008ea6

Browse files
joseph-isaacsjoseph-isaacs
authored
fix[array]: invalid arbitrary decimal scalar value (#5143)
Signed-off-by: Joe Isaacs <[email protected]>
1 parent 9ba13d4 commit a008ea6

File tree

12 files changed

+110
-77
lines changed

12 files changed

+110
-77
lines changed

‎encodings/sparse/src/canonical.rs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use num_traits::NumCast;
88
use vortex_array::arrays::binary_view::BinaryView;
99
use vortex_array::arrays::{
1010
BoolArray, ConstantArray, FixedSizeListArray, ListViewArray, NullArray, PrimitiveArray,
11-
StructArray, VarBinViewArray, smallest_decimal_value_type,
11+
StructArray, VarBinViewArray,
1212
};
1313
use vortex_array::builders::{
1414
ArrayBuilder, DecimalBuilder, ListViewBuilder, builder_with_capacity,
@@ -19,8 +19,8 @@ use vortex_array::vtable::{CanonicalVTable, ValidityHelper};
1919
use vortex_array::{Array, Canonical, ToCanonical};
2020
use vortex_buffer::{BitBuffer, Buffer, BufferString, ByteBuffer, buffer, buffer_mut};
2121
use vortex_dtype::{
22-
DType, DecimalDType, IntegerPType, NativeDecimalType, NativePType, Nullability, StructFields,
23-
match_each_integer_ptype, match_each_native_ptype,
22+
DType, DecimalDType, DecimalType, IntegerPType, NativeDecimalType, NativePType, Nullability,
23+
StructFields, match_each_integer_ptype, match_each_native_ptype,
2424
};
2525
use vortex_error::{VortexError, VortexExpect, vortex_panic};
2626
use vortex_scalar::{
@@ -58,7 +58,8 @@ impl CanonicalVTable<SparseVTable> for SparseVTable {
5858
array.len(),
5959
),
6060
DType::Decimal(decimal_dtype, nullability) => {
61-
let canonical_decimal_value_type = smallest_decimal_value_type(decimal_dtype);
61+
let canonical_decimal_value_type =
62+
DecimalType::smallest_decimal_value_type(decimal_dtype);
6263
let fill_value = array.fill_scalar().as_decimal();
6364
match_each_decimal_value_type!(canonical_decimal_value_type, |D| {
6465
canonicalize_sparse_decimal::<D>(

‎vortex-array/src/arrays/arbitrary.rs‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use vortex_scalar::arbitrary::random_scalar;
1212
use vortex_scalar::{Scalar, match_each_decimal_value_type};
1313

1414
use super::{BoolArray, ChunkedArray, NullArray, PrimitiveArray, StructArray};
15-
use crate::arrays::{VarBinArray, VarBinViewArray, smallest_decimal_value_type};
15+
use crate::arrays::{VarBinArray, VarBinViewArray};
1616
use crate::builders::{ArrayBuilder, DecimalBuilder, FixedSizeListBuilder, ListViewBuilder};
1717
use crate::validity::Validity;
1818
use crate::{Array, ArrayRef, IntoArray, ToCanonical};
@@ -95,17 +95,20 @@ fn random_array_chunk(
9595
},
9696
DType::Decimal(decimal, n) => {
9797
let elem_len = chunk_len.unwrap_or(u.int_in_range(0..=20)?);
98-
match_each_decimal_value_type!(smallest_decimal_value_type(decimal), |DVT| {
99-
let mut builder =
100-
DecimalBuilder::new::<DVT>(decimal.precision(), decimal.scale(), *n);
101-
for _i in 0..elem_len {
102-
let random_decimal = random_scalar(u, &DType::Decimal(*decimal, *n))?;
103-
builder.append_scalar(&random_decimal).vortex_expect(
104-
"was somehow unable to append a decimal to a decimal builder",
105-
);
98+
match_each_decimal_value_type!(
99+
DecimalType::smallest_decimal_value_type(decimal),
100+
|DVT| {
101+
let mut builder =
102+
DecimalBuilder::new::<DVT>(decimal.precision(), decimal.scale(), *n);
103+
for _i in 0..elem_len {
104+
let random_decimal = random_scalar(u, &DType::Decimal(*decimal, *n))?;
105+
builder.append_scalar(&random_decimal).vortex_expect(
106+
"was somehow unable to append a decimal to a decimal builder",
107+
);
108+
}
109+
Ok(builder.finish())
106110
}
107-
Ok(builder.finish())
108-
})
111+
)
109112
}
110113
DType::Utf8(n) => random_string(u, *n, chunk_len),
111114
DType::Binary(n) => random_bytes(u, *n, chunk_len),

‎vortex-array/src/arrays/constant/vtable/canonical.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
use std::sync::Arc;
55

66
use vortex_buffer::{BitBuffer, Buffer, buffer};
7-
use vortex_dtype::{DType, Nullability, match_each_native_ptype};
7+
use vortex_dtype::{DType, DecimalType, Nullability, match_each_native_ptype};
88
use vortex_error::VortexExpect;
99
use vortex_scalar::{
1010
BinaryScalar, BoolScalar, DecimalValue, ExtScalar, ListScalar, Scalar, StructScalar,
@@ -16,7 +16,7 @@ use crate::arrays::constant::ConstantArray;
1616
use crate::arrays::primitive::PrimitiveArray;
1717
use crate::arrays::{
1818
BoolArray, ConstantVTable, DecimalArray, ExtensionArray, FixedSizeListArray, ListViewArray,
19-
NullArray, StructArray, VarBinViewArray, smallest_decimal_value_type,
19+
NullArray, StructArray, VarBinViewArray,
2020
};
2121
use crate::builders::builder_with_capacity;
2222
use crate::validity::Validity;
@@ -66,7 +66,7 @@ impl CanonicalVTable<ConstantVTable> for ConstantVTable {
6666
})
6767
}
6868
DType::Decimal(decimal_type, ..) => {
69-
let size = smallest_decimal_value_type(decimal_type);
69+
let size = DecimalType::smallest_decimal_value_type(decimal_type);
7070
let decimal = scalar.as_decimal();
7171
let Some(value) = decimal.decimal_value() else {
7272
let all_null = match_each_decimal_value_type!(size, |D| {

‎vortex-array/src/arrays/decimal/array.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ use vortex_error::{VortexExpect, VortexResult, vortex_ensure, vortex_panic};
1111
use vortex_scalar::match_each_decimal_value_type;
1212

1313
use crate::ToCanonical;
14-
use crate::arrays::is_compatible_decimal_value_type;
1514
use crate::patches::Patches;
1615
use crate::stats::ArrayStats;
1716
use crate::validity::Validity;
@@ -279,7 +278,7 @@ where
279278
PatchDVT: NativeDecimalType,
280279
ValuesDVT: NativeDecimalType,
281280
{
282-
if !is_compatible_decimal_value_type(ValuesDVT::DECIMAL_TYPE, decimal_dtype) {
281+
if !ValuesDVT::DECIMAL_TYPE.is_compatible_decimal_value_type(decimal_dtype) {
283282
vortex_panic!(
284283
"patch_typed: {:?} cannot represent every value in {}.",
285284
ValuesDVT::DECIMAL_TYPE,

‎vortex-array/src/arrays/decimal/compute/sum.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@
33

44
use arrow_schema::DECIMAL256_MAX_PRECISION;
55
use num_traits::AsPrimitive;
6-
use vortex_dtype::DecimalDType;
76
use vortex_dtype::Nullability::Nullable;
7+
use vortex_dtype::{DecimalDType, DecimalType};
88
use vortex_error::{VortexResult, vortex_bail};
99
use vortex_mask::Mask;
1010
use vortex_scalar::{DecimalValue, Scalar, match_each_decimal_value_type};
1111

12-
use crate::arrays::{DecimalArray, DecimalVTable, smallest_decimal_value_type};
12+
use crate::arrays::{DecimalArray, DecimalVTable};
1313
use crate::compute::{SumKernel, SumKernelAdapter};
1414
use crate::register_kernel;
1515

@@ -54,7 +54,7 @@ impl SumKernel for DecimalVTable {
5454
vortex_bail!("invalid state, all-null array should be checked by top-level sum fn")
5555
}
5656
Mask::AllTrue(_) => {
57-
let values_type = smallest_decimal_value_type(&return_dtype);
57+
let values_type = DecimalType::smallest_decimal_value_type(&return_dtype);
5858
match_each_decimal_value_type!(array.values_type(), |I| {
5959
match_each_decimal_value_type!(values_type, |O| {
6060
Ok(Scalar::decimal(
@@ -66,7 +66,7 @@ impl SumKernel for DecimalVTable {
6666
})
6767
}
6868
Mask::Values(mask_values) => {
69-
let values_type = smallest_decimal_value_type(&return_dtype);
69+
let values_type = DecimalType::smallest_decimal_value_type(&return_dtype);
7070
match_each_decimal_value_type!(array.values_type(), |I| {
7171
match_each_decimal_value_type!(values_type, |O| {
7272
Ok(Scalar::decimal(

‎vortex-array/src/arrays/decimal/utils.rs‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,32 +2,12 @@
22
// SPDX-FileCopyrightText: Copyright the Vortex contributors
33

44
use itertools::{Itertools, MinMaxResult};
5-
use vortex_dtype::DecimalDType;
65
use vortex_error::VortexExpect;
76
use vortex_scalar::{DecimalType, i256};
87

98
use crate::arrays::DecimalArray;
109
use crate::vtable::ValidityHelper;
1110

12-
/// Maps a decimal precision into the smallest type that can represent it.
13-
pub fn smallest_decimal_value_type(decimal_dtype: &DecimalDType) -> DecimalType {
14-
match decimal_dtype.precision() {
15-
1..=2 => DecimalType::I8,
16-
3..=4 => DecimalType::I16,
17-
5..=9 => DecimalType::I32,
18-
10..=18 => DecimalType::I64,
19-
19..=38 => DecimalType::I128,
20-
39..=76 => DecimalType::I256,
21-
0 => unreachable!("precision must be greater than 0"),
22-
p => unreachable!("precision larger than 76 is invalid found precision {p}"),
23-
}
24-
}
25-
26-
/// True if `value_type` can represent every value of the type `dtype`.
27-
pub fn is_compatible_decimal_value_type(value_type: DecimalType, dtype: DecimalDType) -> bool {
28-
value_type >= smallest_decimal_value_type(&dtype)
29-
}
30-
3111
macro_rules! try_downcast {
3212
($array:expr, from: $src:ty, to: $($dst:ty),*) => {{
3313
use vortex_dtype::BigCast;

‎vortex-array/src/builders/decimal.rs‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ use std::any::Any;
55

66
use vortex_buffer::BufferMut;
77
use vortex_dtype::{BigCast, DType, DecimalDType, NativeDecimalType, Nullability};
8-
use vortex_error::{VortexExpect, VortexResult, vortex_ensure, vortex_panic};
8+
use vortex_error::{
9+
VortexExpect, VortexResult, VortexUnwrap, vortex_ensure, vortex_err, vortex_panic,
10+
};
911
use vortex_mask::Mask;
1012
use vortex_scalar::{
1113
DecimalValue, Scalar, i256, match_each_decimal_value, match_each_decimal_value_type,
@@ -212,7 +214,18 @@ impl ArrayBuilder for DecimalBuilder {
212214
impl DecimalBuffer {
213215
fn push<V: NativeDecimalType>(&mut self, value: V) {
214216
delegate_fn!(self, |T, buffer| {
215-
buffer.push(<T as BigCast>::from(value).vortex_expect("decimal conversion failure"))
217+
buffer.push(
218+
<T as BigCast>::from(value)
219+
.ok_or_else(|| {
220+
vortex_err!(
221+
"decimal conversion failure {:?}, type: {:?} to {:?}",
222+
value,
223+
V::DECIMAL_TYPE,
224+
T::DECIMAL_TYPE,
225+
)
226+
})
227+
.vortex_unwrap(),
228+
)
216229
});
217230
}
218231

‎vortex-array/src/builders/mod.rs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ use vortex_error::{VortexResult, vortex_panic};
3535
use vortex_mask::Mask;
3636
use vortex_scalar::{Scalar, match_each_decimal_value_type};
3737

38-
use crate::arrays::smallest_decimal_value_type;
3938
use crate::canonical::Canonical;
4039
use crate::{Array, ArrayRef};
4140

@@ -246,13 +245,16 @@ pub fn builder_with_capacity(dtype: &DType, capacity: usize) -> Box<dyn ArrayBui
246245
})
247246
}
248247
DType::Decimal(decimal_type, n) => {
249-
match_each_decimal_value_type!(smallest_decimal_value_type(decimal_type), |D| {
250-
Box::new(DecimalBuilder::with_capacity::<D>(
251-
capacity,
252-
*decimal_type,
253-
*n,
254-
))
255-
})
248+
match_each_decimal_value_type!(
249+
DecimalType::smallest_decimal_value_type(decimal_type),
250+
|D| {
251+
Box::new(DecimalBuilder::with_capacity::<D>(
252+
capacity,
253+
*decimal_type,
254+
*n,
255+
))
256+
}
257+
)
256258
}
257259
DType::Utf8(n) => Box::new(VarBinViewBuilder::with_capacity(DType::Utf8(*n), capacity)),
258260
DType::Binary(n) => Box::new(VarBinViewBuilder::with_capacity(

‎vortex-dtype/src/decimal/precision.rs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,3 +122,18 @@ impl<D: NativeDecimalType> TryFrom<&DecimalDType> for PrecisionScale<D> {
122122
PrecisionScale::try_new(value.precision, value.scale)
123123
}
124124
}
125+
126+
#[cfg(test)]
127+
mod tests {
128+
use crate::PrecisionScale;
129+
130+
#[test]
131+
fn max_precision() {
132+
let prec = PrecisionScale::<i8>::new(2, 1);
133+
assert!(prec.is_valid(8));
134+
assert!(prec.is_valid(99));
135+
assert!(prec.is_valid(-9));
136+
assert!(prec.is_valid(0));
137+
assert!(prec.is_valid(-99))
138+
}
139+
}

‎vortex-dtype/src/decimal/types.rs‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use paste::paste;
1010
use crate::decimal::max_precision::{
1111
MAX_DECIMAL256_FOR_EACH_PRECISION, MIN_DECIMAL256_FOR_EACH_PRECISION,
1212
};
13-
use crate::{BigCast, i256};
13+
use crate::{BigCast, DecimalDType, i256};
1414

1515
/// Type of the decimal values.
1616
///
@@ -35,6 +35,27 @@ pub enum DecimalType {
3535
I256 = 5,
3636
}
3737

38+
impl DecimalType {
39+
/// Maps a `DecimalDType` (precision) into the smallest `DecimalType` that can represent it.
40+
pub fn smallest_decimal_value_type(decimal_dtype: &DecimalDType) -> DecimalType {
41+
match decimal_dtype.precision() {
42+
1..=2 => DecimalType::I8,
43+
3..=4 => DecimalType::I16,
44+
5..=9 => DecimalType::I32,
45+
10..=18 => DecimalType::I64,
46+
19..=38 => DecimalType::I128,
47+
39..=76 => DecimalType::I256,
48+
0 => unreachable!("precision must be greater than 0"),
49+
p => unreachable!("precision larger than 76 is invalid found precision {p}"),
50+
}
51+
}
52+
53+
/// True if `Self` can represent every value of the type `DecimalDType`.
54+
pub fn is_compatible_decimal_value_type(self, dtype: DecimalDType) -> bool {
55+
self >= Self::smallest_decimal_value_type(&dtype)
56+
}
57+
}
58+
3859
/// Type of decimal scalar values.
3960
///
4061
/// This trait is implemented by native integer types that can be used to store decimal values.
@@ -61,8 +82,11 @@ pub trait NativeDecimalType:
6182
const MAX_SCALE: i8;
6283

6384
/// The minimum value for each precision supported by this decimal type.
85+
/// This is an array of length `MAX_PRECISION + 1` where the `i`th element is the minimum value
86+
/// for a precision of `i` (including precision 0).
6487
const MIN_BY_PRECISION: &'static [Self];
6588
/// The maximum value for each precision supported by this decimal type.
89+
/// similar to `MIN_BY_PRECISION`.
6690
const MAX_BY_PRECISION: &'static [Self];
6791

6892
/// Downcast the provided object to a type-specific instance.
@@ -127,24 +151,24 @@ macro_rules! impl_decimal {
127151
const MAX_SCALE: i8 = Self::MAX_PRECISION as i8;
128152

129153
const MIN_BY_PRECISION: &'static [Self] = &{
130-
let mut mins = [$T::ZERO; Self::MAX_PRECISION as usize];
154+
let mut mins = [$T::ZERO; Self::MAX_PRECISION as usize + 1];
131155
let mut p = $T::ONE;
132156
let mut i = 0;
133157
while i < Self::MAX_PRECISION as usize {
134158
p = p * 10;
135-
mins[i] = -(p - 1);
159+
mins[i + 1] = -(p - 1);
136160
i += 1;
137161
}
138162
mins
139163
};
140164

141165
const MAX_BY_PRECISION: &'static [Self] = &{
142-
let mut maxs = [$T::ZERO; Self::MAX_PRECISION as usize];
166+
let mut maxs = [$T::ZERO; Self::MAX_PRECISION as usize + 1];
143167
let mut p = $T::ONE;
144168
let mut i = 0;
145169
while i < Self::MAX_PRECISION as usize {
146170
p = p * 10;
147-
maxs[i] = p - 1;
171+
maxs[i + 1] = p - 1;
148172
i += 1;
149173
}
150174
maxs

0 commit comments

Comments
 (0)