Skip to content

Commit bdf48cd

Browse files
committed
refactor(array): name the Widen kernel helpers for what they return
Rename the shared helpers and the arithmetic helpers so that each name says what it returns, move constant_fits_values next to the other shared helpers now that between also calls it, and document where the arithmetic kernel relies on min and max statistics. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
1 parent 6e27274 commit bdf48cd

7 files changed

Lines changed: 93 additions & 78 deletions

File tree

‎vortex-array/src/arrays/widen/compute/between.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ use crate::array::ArrayView;
88
use crate::arrays::Constant;
99
use crate::arrays::Widen;
1010
use crate::arrays::widen::WidenArraySlotsExt;
11-
use crate::arrays::widen::compute::compare::constant_fits_child;
12-
use crate::arrays::widen::compute::constant_values;
11+
use crate::arrays::widen::compute::constant_fits_values;
12+
use crate::arrays::widen::compute::constant_in_values_ptype;
1313
use crate::builtins::ArrayBuiltins;
1414
use crate::scalar_fn::fns::between::BetweenOptions;
1515
use crate::scalar_fn::fns::between::BetweenReduce;
@@ -25,11 +25,11 @@ impl BetweenReduce for Widen {
2525
let values = array.values();
2626

2727
if let (Some(lower), Some(upper)) = (lower.as_constant(), upper.as_constant())
28-
&& constant_fits_child(lower, values.dtype()) == Some(true)
29-
&& constant_fits_child(upper, values.dtype()) == Some(true)
28+
&& constant_fits_values(lower, values.dtype()) == Some(true)
29+
&& constant_fits_values(upper, values.dtype()) == Some(true)
3030
{
31-
let lower = constant_values(lower, values, array.len())?;
32-
let upper = constant_values(upper, values, array.len())?;
31+
let lower = constant_in_values_ptype(lower, values, array.len())?;
32+
let upper = constant_in_values_ptype(upper, values, array.len())?;
3333
return values
3434
.clone()
3535
.between(lower, upper, options.clone())

‎vortex-array/src/arrays/widen/compute/cast.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ impl CastReduce for Widen {
2222
return Ok(None);
2323
};
2424

25-
let cast_ptype = match cast_values_ptype(*values_ptype, *target_ptype) {
25+
let cast_ptype = match values_ptype_for_cast(*values_ptype, *target_ptype) {
2626
Some(cast_ptype) if target_ptype.byte_width() > cast_ptype.byte_width() => cast_ptype,
2727
// A float target, or an integer target no wider than `cast_ptype`, goes to the values,
2828
// whose cast checks every value and every null.
@@ -43,7 +43,7 @@ impl CastReduce for Widen {
4343
/// The result has the signedness of `target`. An unsigned type fits a signed type only one width
4444
/// up, so unsigned values with a signed target widen one step. Signed values with an unsigned
4545
/// target keep their width, and the cast of the values rejects a negative value.
46-
fn cast_values_ptype(values: PType, target: PType) -> Option<PType> {
46+
fn values_ptype_for_cast(values: PType, target: PType) -> Option<PType> {
4747
if target.is_float() {
4848
return None;
4949
}

‎vortex-array/src/arrays/widen/compute/compare.rs‎

Lines changed: 9 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,11 @@ use crate::arrays::BoolArray;
1313
use crate::arrays::ConstantArray;
1414
use crate::arrays::Widen;
1515
use crate::arrays::widen::WidenArraySlotsExt;
16-
use crate::arrays::widen::compute::cast_to_ptype;
17-
use crate::arrays::widen::compute::constant_values;
18-
use crate::arrays::widen::compute::wider_values_ptype;
16+
use crate::arrays::widen::compute::cast_keeping_nullability;
17+
use crate::arrays::widen::compute::constant_fits_values;
18+
use crate::arrays::widen::compute::constant_in_values_ptype;
19+
use crate::arrays::widen::compute::ptype_holding_both_values;
1920
use crate::builtins::ArrayBuiltins;
20-
use crate::dtype::DType;
21-
use crate::scalar::PValue;
2221
use crate::scalar::Scalar;
2322
use crate::scalar_fn::fns::binary::CompareKernel;
2423
use crate::scalar_fn::fns::operators::CompareOperator;
@@ -44,54 +43,31 @@ impl CompareKernel for Widen {
4443
return Ok(None);
4544
};
4645
let values = lhs.values();
47-
let Some(fits) = constant_fits_child(constant, values.dtype()) else {
46+
let Some(fits) = constant_fits_values(constant, values.dtype()) else {
4847
return Ok(None);
4948
};
5049
if !fits {
5150
return compare_out_of_range(lhs, constant, operator, ctx).map(Some);
5251
}
5352

54-
let constant = constant_values(constant, values, lhs.len())?;
53+
let constant = constant_in_values_ptype(constant, values, lhs.len())?;
5554

5655
values.binary(constant, Operator::from(operator)).map(Some)
5756
}
5857
}
5958

60-
/// Returns whether the range of the integer dtype `child` holds `constant`.
61-
///
62-
/// Returns `None` when `child` is not an integer dtype, when `constant` is null, and when
63-
/// `constant` is not an integer with the signedness of `child`.
64-
pub(super) fn constant_fits_child(constant: &Scalar, child: &DType) -> Option<bool> {
65-
let DType::Primitive(ptype, _) = child else {
66-
return None;
67-
};
68-
let value = constant.as_primitive_opt()?.pvalue()?;
69-
70-
// `PValue` compares an integer with another value through `i64` or `u64`, and it panics when
71-
// the other value does not convert. So only integers of the same signedness compare here.
72-
let value_ptype = value.ptype();
73-
if !ptype.is_int() || !value_ptype.is_int() {
74-
return None;
75-
}
76-
if ptype.is_signed_int() != value_ptype.is_signed_int() {
77-
return None;
78-
}
79-
80-
Some(PValue::min_value(*ptype) <= value && value <= PValue::max_value(*ptype))
81-
}
82-
8359
/// Compares the values of two `Widen` arrays at the wider of the two value types.
8460
fn compare_narrow_values(
8561
lhs: ArrayView<'_, Widen>,
8662
rhs: ArrayView<'_, Widen>,
8763
operator: CompareOperator,
8864
) -> VortexResult<Option<ArrayRef>> {
89-
let Some(ptype) = wider_values_ptype(lhs, rhs) else {
65+
let Some(ptype) = ptype_holding_both_values(lhs, rhs) else {
9066
return Ok(None);
9167
};
9268

93-
let lhs_values = cast_to_ptype(lhs.values(), ptype)?;
94-
let rhs_values = cast_to_ptype(rhs.values(), ptype)?;
69+
let lhs_values = cast_keeping_nullability(lhs.values(), ptype)?;
70+
let rhs_values = cast_keeping_nullability(rhs.values(), ptype)?;
9571

9672
lhs_values
9773
.binary(rhs_values, Operator::from(operator))

‎vortex-array/src/arrays/widen/compute/fill_null.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ use crate::ArrayRef;
77
use crate::array::ArrayView;
88
use crate::arrays::Widen;
99
use crate::arrays::widen::WidenArraySlotsExt;
10-
use crate::arrays::widen::compute::cast_to_ptype;
11-
use crate::arrays::widen::compute::values_ptype_holding;
10+
use crate::arrays::widen::compute::cast_keeping_nullability;
11+
use crate::arrays::widen::compute::ptype_holding_values_and_scalar;
1212
use crate::arrays::widen::compute::wrap_values;
1313
use crate::builtins::ArrayBuiltins;
1414
use crate::dtype::DType;
@@ -20,13 +20,13 @@ impl FillNullReduce for Widen {
2020
array: ArrayView<'_, Self>,
2121
fill_value: &Scalar,
2222
) -> VortexResult<Option<ArrayRef>> {
23-
let Some(ptype) = values_ptype_holding(array, fill_value)? else {
23+
let Some(ptype) = ptype_holding_values_and_scalar(array, fill_value)? else {
2424
return Ok(None);
2525
};
2626

2727
// The values widen to a type that holds the fill value. When only the logical type holds
2828
// it, the result is a plain array of the logical type.
29-
let widened = cast_to_ptype(array.values(), ptype)?;
29+
let widened = cast_keeping_nullability(array.values(), ptype)?;
3030
let fill_value =
3131
fill_value.cast(&DType::Primitive(ptype, fill_value.dtype().nullability()))?;
3232
let filled = widened.fill_null(fill_value)?;

‎vortex-array/src/arrays/widen/compute/mod.rs‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99
//! and extrema carry over from the values, and every value in a result widens exactly to the
1010
//! logical dtype.
1111
//!
12+
//! Most of these operations decide from dtypes and constants alone. The arithmetic kernel also
13+
//! needs the min and max of each operand, which come from cached statistics or from one scan of the
14+
//! operand. That scan is the one hidden cost in this module.
15+
//!
1216
//! [`AggregateFnSession::default`] registers the aggregate kernel, not
1317
//! [`initialize`](super::initialize), because the kernel session runs `initialize` on a temporary
1418
//! session and drops its aggregate registry.
@@ -42,6 +46,7 @@ use crate::arrays::widen::encode::smallest_integer_ptype;
4246
use crate::builtins::ArrayBuiltins;
4347
use crate::dtype::DType;
4448
use crate::dtype::PType;
49+
use crate::scalar::PValue;
4550
use crate::scalar::Scalar;
4651

4752
/// Returns `values` when it has `dtype`, and a `Widen` of `dtype` over `values` otherwise.
@@ -62,7 +67,7 @@ pub(super) fn wrap_values(values: ArrayRef, dtype: DType) -> VortexResult<ArrayR
6267
///
6368
/// A null `scalar` keeps the type of the values. Returns `None` when `scalar` does not have the
6469
/// logical dtype, or when the values or the logical dtype is not an integer (a decimal `Widen`).
65-
pub(super) fn values_ptype_holding(
70+
pub(super) fn ptype_holding_values_and_scalar(
6671
array: ArrayView<'_, Widen>,
6772
scalar: &Scalar,
6873
) -> VortexResult<Option<PType>> {
@@ -92,16 +97,39 @@ pub(super) fn values_ptype_holding(
9297
}
9398

9499
/// Casts the integer array `array` to `ptype`, keeping its nullability.
95-
pub(super) fn cast_to_ptype(array: &ArrayRef, ptype: PType) -> VortexResult<ArrayRef> {
100+
pub(super) fn cast_keeping_nullability(array: &ArrayRef, ptype: PType) -> VortexResult<ArrayRef> {
96101
array.cast(DType::Primitive(ptype, array.dtype().nullability()))
97102
}
98103

104+
/// Returns whether the range of the integer dtype `values` holds `constant`.
105+
///
106+
/// Returns `None` when `values` is not an integer dtype, when `constant` is null, and when
107+
/// `constant` is not an integer with the signedness of `values`.
108+
pub(super) fn constant_fits_values(constant: &Scalar, values: &DType) -> Option<bool> {
109+
let DType::Primitive(ptype, _) = values else {
110+
return None;
111+
};
112+
let value = constant.as_primitive_opt()?.pvalue()?;
113+
114+
// `PValue` compares an integer with another value through `i64` or `u64`, and it panics when
115+
// the other value does not convert. So only integers of the same signedness compare here.
116+
let value_ptype = value.ptype();
117+
if !ptype.is_int() || !value_ptype.is_int() {
118+
return None;
119+
}
120+
if ptype.is_signed_int() != value_ptype.is_signed_int() {
121+
return None;
122+
}
123+
124+
Some(PValue::min_value(*ptype) <= value && value <= PValue::max_value(*ptype))
125+
}
126+
99127
/// Returns the wider of the value types of two [`Widen`] arrays, or `None` when a value type is
100128
/// not primitive.
101129
///
102130
/// The logical dtypes **must** have the same signedness, which a debug build asserts. The value
103131
/// types then have that signedness too, so the wider one holds every value of the narrower one.
104-
pub(super) fn wider_values_ptype(
132+
pub(super) fn ptype_holding_both_values(
105133
lhs: ArrayView<'_, Widen>,
106134
rhs: ArrayView<'_, Widen>,
107135
) -> Option<PType> {
@@ -120,7 +148,7 @@ pub(super) fn wider_values_ptype(
120148

121149
/// Returns `constant`, which **must** fit the type of `values`, as a constant array of that type
122150
/// with the nullability of `constant`.
123-
pub(super) fn constant_values(
151+
pub(super) fn constant_in_values_ptype(
124152
constant: &Scalar,
125153
values: &ArrayRef,
126154
len: usize,

‎vortex-array/src/arrays/widen/compute/numeric.rs‎

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use crate::arrays::scalar_fn::ExactScalarFn;
2323
use crate::arrays::scalar_fn::ScalarFnArrayExt;
2424
use crate::arrays::scalar_fn::ScalarFnArrayView;
2525
use crate::arrays::widen::WidenArraySlotsExt;
26-
use crate::arrays::widen::compute::cast_to_ptype;
26+
use crate::arrays::widen::compute::cast_keeping_nullability;
2727
use crate::arrays::widen::compute::wrap_values;
2828
use crate::arrays::widen::encode::integer_value;
2929
use crate::arrays::widen::encode::smallest_integer_ptype;
@@ -39,11 +39,17 @@ use crate::scalar_fn::fns::operators::Operator;
3939
/// Runs checked add, subtract, multiply and divide of a [`Widen`] with another `Widen` or a
4040
/// constant at the smallest integer type that holds both operands and every result.
4141
///
42-
/// Narrow values read and write less memory. [`arithmetic_ptype`] picks the type from the extrema
43-
/// of the valid rows, so the type can be too wide but never too narrow. When no narrower type is
44-
/// known to hold the result, the operation runs at the logical type, where the checked kernel
45-
/// reports an overflow or a division by zero with the same error as canonical execution. Any other
46-
/// operand declines, so the default path widens it once.
42+
/// Narrow values read and write less memory. [`smallest_arithmetic_ptype`] picks the type from the
43+
/// extrema of the valid rows, so the type can be too wide but never too narrow. When no narrower
44+
/// type is known to hold the result, the operation runs at the logical type, where the checked
45+
/// kernel reports an overflow or a division by zero with the same error as canonical execution. Any
46+
/// other operand declines, so the default path widens it once.
47+
///
48+
/// **Cost:** the extrema come from statistics. [`valid_value_interval`] reads the exact cached
49+
/// `Min` and `Max` of each executed operand when they exist, and otherwise scans it once before the
50+
/// operation runs. Executing `Primitive` values returns the same array, so its cached statistics
51+
/// count and the result stays cached. Encoded values execute to a new array, so neither their
52+
/// cached statistics nor the new result carry over.
4753
#[derive(Debug)]
4854
pub(crate) struct WidenNumericKernel;
4955

@@ -77,7 +83,7 @@ impl ExecuteParentKernel<Widen> for WidenNumericKernel {
7783
1 => binary.get_child(0),
7884
_ => return Ok(None),
7985
};
80-
let other_stored = if let Some(other) = other.as_opt::<Widen>() {
86+
let other_values = if let Some(other) = other.as_opt::<Widen>() {
8187
other.values().clone()
8288
} else if other.is::<Constant>() {
8389
other.clone()
@@ -90,33 +96,35 @@ impl ExecuteParentKernel<Widen> for WidenNumericKernel {
9096
return Ok(Some(Canonical::empty(dtype).into_array()));
9197
}
9298

93-
// Execute each stored operand once. `Columnar` keeps a constant constant.
94-
let stored = array
99+
// Execute the values of each operand once. `Columnar` keeps a constant constant.
100+
let values = array
95101
.values()
96102
.clone()
97103
.execute::<Columnar>(ctx)?
98104
.into_array();
99-
let other_stored = other_stored.execute::<Columnar>(ctx)?.into_array();
105+
let other_values = other_values.execute::<Columnar>(ctx)?.into_array();
100106
let (lhs, rhs) = if child_idx == 0 {
101-
(stored, other_stored)
107+
(values, other_values)
102108
} else {
103-
(other_stored, stored)
109+
(other_values, values)
104110
};
105111

106112
// An operand without a valid row makes every row null.
107-
let (Some(lhs_bounds), Some(rhs_bounds)) =
108-
(valid_bounds(&lhs, ctx)?, valid_bounds(&rhs, ctx)?)
109-
else {
113+
let (Some(lhs_interval), Some(rhs_interval)) = (
114+
valid_value_interval(&lhs, ctx)?,
115+
valid_value_interval(&rhs, ctx)?,
116+
) else {
110117
let null = Scalar::null(dtype.clone());
111118
return Ok(Some(ConstantArray::new(null, parent.len()).into_array()));
112119
};
113120

114-
let ptype = arithmetic_ptype(&lhs_bounds, &rhs_bounds, operator, *logical_ptype);
115-
let lhs = cast_to_ptype(&lhs, ptype)?;
116-
let rhs = cast_to_ptype(&rhs, ptype)?;
117-
let values = execute_numeric(&lhs, &rhs, operator, ctx)?;
121+
let ptype =
122+
smallest_arithmetic_ptype(&lhs_interval, &rhs_interval, operator, *logical_ptype);
123+
let lhs = cast_keeping_nullability(&lhs, ptype)?;
124+
let rhs = cast_keeping_nullability(&rhs, ptype)?;
125+
let result = execute_numeric(&lhs, &rhs, operator, ctx)?;
118126

119-
wrap_values(values, dtype.clone()).map(Some)
127+
wrap_values(result, dtype.clone()).map(Some)
120128
}
121129
}
122130

@@ -126,7 +134,7 @@ impl ExecuteParentKernel<Widen> for WidenNumericKernel {
126134
///
127135
/// Null rows do not count. A cast to the narrower type can change their payloads, and the checked
128136
/// kernel skips them.
129-
fn arithmetic_ptype(
137+
fn smallest_arithmetic_ptype(
130138
lhs: &RangeInclusive<i128>,
131139
rhs: &RangeInclusive<i128>,
132140
operator: NumericOperator,
@@ -143,7 +151,10 @@ fn arithmetic_ptype(
143151
}
144152

145153
/// Returns the extrema of the valid values of an integer array, or `None` when no row is valid.
146-
fn valid_bounds(
154+
///
155+
/// [`min_max`] reads the exact cached `Min` and `Max` statistics of `array`. Without them it scans
156+
/// `array` once and caches the result on it.
157+
fn valid_value_interval(
147158
array: &ArrayRef,
148159
ctx: &mut ExecutionCtx,
149160
) -> VortexResult<Option<RangeInclusive<i128>>> {

‎vortex-array/src/arrays/widen/compute/zip.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@ use crate::arrays::scalar_fn::ExactScalarFn;
1313
use crate::arrays::scalar_fn::ScalarFnArrayExt;
1414
use crate::arrays::scalar_fn::ScalarFnArrayView;
1515
use crate::arrays::widen::WidenArraySlotsExt;
16-
use crate::arrays::widen::compute::cast_to_ptype;
17-
use crate::arrays::widen::compute::values_ptype_holding;
18-
use crate::arrays::widen::compute::wider_values_ptype;
16+
use crate::arrays::widen::compute::cast_keeping_nullability;
17+
use crate::arrays::widen::compute::ptype_holding_both_values;
18+
use crate::arrays::widen::compute::ptype_holding_values_and_scalar;
1919
use crate::arrays::widen::compute::wrap_values;
2020
use crate::builtins::ArrayBuiltins;
2121
use crate::kernel::ExecuteParentKernel;
@@ -79,21 +79,21 @@ fn zip_values(
7979
) -> VortexResult<Option<ArrayRef>> {
8080
let values = array.values();
8181
let (other_values, ptype) = if let Some(other) = other.as_opt::<Widen>() {
82-
let Some(ptype) = wider_values_ptype(array, other) else {
82+
let Some(ptype) = ptype_holding_both_values(array, other) else {
8383
return Ok(None);
8484
};
8585
(other.values().clone(), ptype)
8686
} else if let Some(constant) = other.as_constant() {
87-
let Some(ptype) = values_ptype_holding(array, constant)? else {
87+
let Some(ptype) = ptype_holding_values_and_scalar(array, constant)? else {
8888
return Ok(None);
8989
};
9090
(other.clone(), ptype)
9191
} else {
9292
return Ok(None);
9393
};
9494

95-
let values = cast_to_ptype(values, ptype)?;
96-
let other_values = cast_to_ptype(&other_values, ptype)?;
95+
let values = cast_keeping_nullability(values, ptype)?;
96+
let other_values = cast_keeping_nullability(&other_values, ptype)?;
9797

9898
let zipped = match branch {
9999
Branch::IfTrue => mask.zip(values, other_values)?,

0 commit comments

Comments
 (0)