From 9363149111ec21ef5bbaa5fbc14feb12dcf3a424 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 22:12:45 +0000 Subject: [PATCH] Rename the `precondition` helpers to `short_circuit` These helpers do not check a precondition. They compute the answer for the inputs that need no encoding-specific work and return it, so a caller treats `Some` as done. Every one of their doc comments already explained the name by reaching for a different word, which is the name they should have had. Renames all five, in `slice`, `dict::take`, `filter`, `fill_null`, and `between`, so one grep still finds the family. Rewords the shared doc template, which would otherwise read as "the short circuit short-circuits", and gives the `slice` helper the doc comment the other four already had. Updates the messages that name the mechanism, and leaves the unrelated uses of "precondition" that describe a real caller contract. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- .../fastlanes/src/bitpacking/compute/between.rs | 4 ++-- vortex-array/src/arrays/dict/take.rs | 12 ++++++------ .../src/arrays/filter/execute/take/fixed_width.rs | 2 +- vortex-array/src/arrays/filter/execute/take/rank.rs | 2 +- vortex-array/src/arrays/filter/kernel.rs | 12 ++++++------ vortex-array/src/arrays/slice/mod.rs | 10 +++++++--- vortex-array/src/scalar_fn/fns/between/kernel.rs | 6 +++--- vortex-array/src/scalar_fn/fns/between/mod.rs | 13 ++++++------- vortex-array/src/scalar_fn/fns/fill_null/kernel.rs | 12 ++++++------ vortex-array/src/scalar_fn/fns/fill_null/mod.rs | 4 ++-- 10 files changed, 40 insertions(+), 37 deletions(-) diff --git a/encodings/fastlanes/src/bitpacking/compute/between.rs b/encodings/fastlanes/src/bitpacking/compute/between.rs index 1dd3b61dbf4..b08686ea8c8 100644 --- a/encodings/fastlanes/src/bitpacking/compute/between.rs +++ b/encodings/fastlanes/src/bitpacking/compute/between.rs @@ -55,10 +55,10 @@ impl BetweenKernel for BitPacked { let result = match_each_integer_ptype!(arr_ptype, |T| { let lo: T = lower_prim .typed_value::() - .vortex_expect("between precondition strips null lower"); + .vortex_expect("between short circuit strips null lower"); let up: T = upper_prim .typed_value::() - .vortex_expect("between precondition strips null upper"); + .vortex_expect("between short circuit strips null upper"); between_constant_typed::(array, lo, up, options, nullability, ctx)? }); Ok(Some(result)) diff --git a/vortex-array/src/arrays/dict/take.rs b/vortex-array/src/arrays/dict/take.rs index b77bef19b39..d557d34b72f 100644 --- a/vortex-array/src/arrays/dict/take.rs +++ b/vortex-array/src/arrays/dict/take.rs @@ -53,11 +53,11 @@ pub trait TakeExecute: VTable { ) -> VortexResult>; } -/// Common preconditions for take operations that apply to all arrays. +/// Short-circuits take for the inputs that need no encoding-specific work. /// -/// Returns `Some(result)` if the precondition short-circuits the take operation, -/// or `None` if the take should proceed normally. -fn precondition(array: ArrayView<'_, V>, indices: &ArrayRef) -> Option { +/// Returns `Some(result)` when the answer is already known, or `None` when take must proceed +/// normally. +fn short_circuit(array: ArrayView<'_, V>, indices: &ArrayRef) -> Option { // Fast-path for empty indices. if indices.is_empty() { let result_dtype = array @@ -97,7 +97,7 @@ where if child_idx != 1 { return Ok(None); } - if let Some(result) = precondition::(array, parent.codes()) { + if let Some(result) = short_circuit::(array, parent.codes()) { return Ok(Some(result)); } let result = ::take(array, parent.codes())?; @@ -128,7 +128,7 @@ where if child_idx != 1 { return Ok(None); } - if let Some(result) = precondition::(array, parent.codes()) { + if let Some(result) = short_circuit::(array, parent.codes()) { return Ok(Some(result)); } let result = ::take(array, parent.codes(), ctx)?; diff --git a/vortex-array/src/arrays/filter/execute/take/fixed_width.rs b/vortex-array/src/arrays/filter/execute/take/fixed_width.rs index 790497988c4..6518293db41 100644 --- a/vortex-array/src/arrays/filter/execute/take/fixed_width.rs +++ b/vortex-array/src/arrays/filter/execute/take/fixed_width.rs @@ -175,7 +175,7 @@ where take_values_by_rank(values, ranks, filtered_len, |idx| idx) } } - AllOr::None => unreachable!("empty filters are handled by take preconditions"), + AllOr::None => unreachable!("empty filters are handled by the filter short circuit"), AllOr::Some(indices) => { if let Some(indices_validity) = indices_validity { take_values_by_rank_nullable( diff --git a/vortex-array/src/arrays/filter/execute/take/rank.rs b/vortex-array/src/arrays/filter/execute/take/rank.rs index efbfa768df3..58d385d0b47 100644 --- a/vortex-array/src/arrays/filter/execute/take/rank.rs +++ b/vortex-array/src/arrays/filter/execute/take/rank.rs @@ -82,7 +82,7 @@ pub(in crate::arrays::filter) fn translate_ranks( match filter.indices() { AllOr::All => translate_ranks_with(ranks, ranks_validity, filtered_len, |rank| rank), - AllOr::None => unreachable!("empty filters are handled by take preconditions"), + AllOr::None => unreachable!("empty filters are handled by the filter short circuit"), AllOr::Some(filter_indices) => { translate_ranks_with(ranks, ranks_validity, filtered_len, |rank| unsafe { *filter_indices.get_unchecked(rank) diff --git a/vortex-array/src/arrays/filter/kernel.rs b/vortex-array/src/arrays/filter/kernel.rs index dbbe283138c..a62dc7910a0 100644 --- a/vortex-array/src/arrays/filter/kernel.rs +++ b/vortex-array/src/arrays/filter/kernel.rs @@ -67,11 +67,11 @@ pub trait FilterKernel: VTable { ) -> VortexResult>; } -/// Common preconditions for filter operations that apply to all arrays. +/// Short-circuits filter for the inputs that need no encoding-specific work. /// -/// Returns `Some(result)` if the precondition short-circuits the filter operation, -/// or `None` if the filter should proceed normally. -fn precondition(array: ArrayView<'_, V>, mask: &Mask) -> Option { +/// Returns `Some(result)` when the answer is already known, or `None` when the filter must proceed +/// normally. +fn short_circuit(array: ArrayView<'_, V>, mask: &Mask) -> Option { let true_count = mask.true_count(); // Fast-path for empty mask (all false). @@ -104,7 +104,7 @@ where child_idx: usize, ) -> VortexResult> { assert_eq!(child_idx, 0); - if let Some(result) = precondition::(array, parent.filter_mask()) { + if let Some(result) = short_circuit::(array, parent.filter_mask()) { return Ok(Some(result)); } ::filter(array, parent.filter_mask()) @@ -129,7 +129,7 @@ where ctx: &mut ExecutionCtx, ) -> VortexResult> { assert_eq!(child_idx, 0); - if let Some(result) = precondition::(array, parent.filter_mask()) { + if let Some(result) = short_circuit::(array, parent.filter_mask()) { return Ok(Some(result)); } ::filter(array, parent.filter_mask(), ctx) diff --git a/vortex-array/src/arrays/slice/mod.rs b/vortex-array/src/arrays/slice/mod.rs index 3f0307773b7..f4d34f47a02 100644 --- a/vortex-array/src/arrays/slice/mod.rs +++ b/vortex-array/src/arrays/slice/mod.rs @@ -66,7 +66,11 @@ pub trait SliceKernel: VTable { ) -> VortexResult>; } -fn precondition(array: ArrayView<'_, V>, range: &Range) -> Option { +/// Short-circuits slice for the ranges that need no encoding-specific work. +/// +/// Returns `Some(result)` when the answer is already known, or `None` when the slice must proceed +/// normally. +fn short_circuit(array: ArrayView<'_, V>, range: &Range) -> Option { if range.start == 0 && range.end == array.len() { return Some(array.array().clone()); }; @@ -93,7 +97,7 @@ where child_idx: usize, ) -> VortexResult> { assert_eq!(child_idx, 0); - if let Some(result) = precondition::(array, &parent.range) { + if let Some(result) = short_circuit::(array, &parent.range) { return Ok(Some(result)); } ::slice(array, parent.range.clone()) @@ -118,7 +122,7 @@ where ctx: &mut ExecutionCtx, ) -> VortexResult> { assert_eq!(child_idx, 0); - if let Some(result) = precondition::(array, &parent.range) { + if let Some(result) = short_circuit::(array, &parent.range) { return Ok(Some(result)); } ::slice(array, parent.range.clone(), ctx) diff --git a/vortex-array/src/scalar_fn/fns/between/kernel.rs b/vortex-array/src/scalar_fn/fns/between/kernel.rs index a4e03af623f..f7a42252164 100644 --- a/vortex-array/src/scalar_fn/fns/between/kernel.rs +++ b/vortex-array/src/scalar_fn/fns/between/kernel.rs @@ -6,7 +6,7 @@ use vortex_error::VortexResult; use super::Between; use super::BetweenOptions; -use super::precondition; +use super::short_circuit; use crate::ArrayRef; use crate::ExecutionCtx; use crate::array::ArrayView; @@ -70,7 +70,7 @@ where let lower = &children[1]; let upper = &children[2]; let arr = array.array().clone(); - if let Some(result) = precondition(&arr, lower, upper, parent.options)? { + if let Some(result) = short_circuit(&arr, lower, upper, parent.options)? { return Ok(Some(result)); } ::between(array, lower, upper, parent.options) @@ -105,7 +105,7 @@ where let lower = &children[1]; let upper = &children[2]; let arr = array.array().clone(); - if let Some(result) = precondition(&arr, lower, upper, parent.options)? { + if let Some(result) = short_circuit(&arr, lower, upper, parent.options)? { // TODO(joe): return the lazy array directly, blocked on the same executor support as // the fallback in `between_canonical`. The reduce adaptor above already passes it // through unexecuted, since a reduce rule can return a lazy array. diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index a8df0214ae0..5e0a6ec83c6 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -78,18 +78,17 @@ impl StrictComparison { } } -/// Common preconditions for between operations that apply to all arrays. +/// Short-circuits between for the inputs that need no encoding-specific work. /// -/// Returns `Some(result)` if the precondition short-circuits the between operation -/// (empty array, null bounds), or `None` if between must proceed with the -/// encoding-specific implementation. Kernels can therefore rely on both bounds being -/// non-null. +/// Returns `Some(result)` when the answer is already known (empty array, null bounds), or `None` +/// when between must proceed with the encoding-specific implementation. Kernels can therefore rely +/// on both bounds being non-null. /// /// The result can be a lazy [`ScalarFn`] array, so a caller that needs a computed array /// **must** execute it. /// /// [`ScalarFn`]: crate::arrays::ScalarFn -pub(super) fn precondition( +pub(super) fn short_circuit( arr: &ArrayRef, lower: &ArrayRef, upper: &ArrayRef, @@ -147,7 +146,7 @@ fn between_canonical( options: &BetweenOptions, ctx: &mut ExecutionCtx, ) -> VortexResult { - if let Some(result) = precondition(arr, lower, upper, options)? { + if let Some(result) = short_circuit(arr, lower, upper, options)? { // TODO(joe): return the lazy array directly, blocked on the same executor support as the // fallback below. Only the single-null-bound case is lazy, so this forces it for now. return result.execute::(ctx); diff --git a/vortex-array/src/scalar_fn/fns/fill_null/kernel.rs b/vortex-array/src/scalar_fn/fns/fill_null/kernel.rs index 147658ddfb2..389f95462d0 100644 --- a/vortex-array/src/scalar_fn/fns/fill_null/kernel.rs +++ b/vortex-array/src/scalar_fn/fns/fill_null/kernel.rs @@ -55,11 +55,11 @@ pub trait FillNullKernel: VTable { ) -> VortexResult>; } -/// Common preconditions for fill_null operations that apply to all arrays. +/// Short-circuits fill_null for the inputs that need no encoding-specific work. /// -/// Returns `Some(result)` if the precondition short-circuits the fill_null operation, -/// or `None` if fill_null should proceed with the encoding-specific implementation. -pub(super) fn precondition( +/// Returns `Some(result)` when the answer is already known, or `None` when fill_null must proceed +/// with the encoding-specific implementation. Kernels can therefore rely on a non-null fill value. +pub(super) fn short_circuit( array: &ArrayRef, fill_value: &Scalar, ) -> VortexResult> { @@ -130,7 +130,7 @@ where .as_constant() .vortex_expect("fill_null fill_value must be constant"); let arr = array.array().clone(); - if let Some(result) = precondition(&arr, &fill_value)? { + if let Some(result) = short_circuit(&arr, &fill_value)? { return Ok(Some(result)); } ::fill_null(array, &fill_value) @@ -166,7 +166,7 @@ where .as_constant() .vortex_expect("fill_null fill_value must be constant"); let arr = array.array().clone(); - if let Some(result) = precondition(&arr, &fill_value)? { + if let Some(result) = short_circuit(&arr, &fill_value)? { return Ok(Some(result)); } ::fill_null(array, &fill_value, ctx) diff --git a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs index 34504e7a519..51c50275f97 100644 --- a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs +++ b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs @@ -151,8 +151,8 @@ fn fill_null_canonical( ctx: &mut ExecutionCtx, ) -> VortexResult { let arr = canonical.to_array_ref(); - if let Some(result) = precondition(&arr, fill_value)? { - // The result of precondition may return another ScalarFn, in which case we should + if let Some(result) = short_circuit(&arr, fill_value)? { + // The result of short_circuit may return another ScalarFn, in which case we should // apply it immediately. // TODO(aduffy): Remove this once we have better driver check. We're also implicitly // relying on the fact that Cast execution will do an optimize on its result.