diff --git a/vortex-array/src/scalar_fn/unstable/row/execute/mod.rs b/vortex-array/src/scalar_fn/unstable/row/execute/mod.rs index 73722549f41..b2f74a515aa 100644 --- a/vortex-array/src/scalar_fn/unstable/row/execute/mod.rs +++ b/vortex-array/src/scalar_fn/unstable/row/execute/mod.rs @@ -4,16 +4,12 @@ //! Row-loop execution for owned outputs and output sinks. //! //! [`owned`] stores one independent value per row and reduces compact failure evidence. [`sink`] -//! drives output builders whose row handles may share batch state. Both return [`RowExecution`], -//! which distinguishes a completed array from a deferred error that batch validity may suppress. +//! drives output builders whose row handles may share batch state. mod owned; pub(super) use owned::execute_owned; pub(super) use owned::execute_owned_infallible; -mod outcome; -pub use outcome::RowExecution; - mod sink; pub(super) use sink::execute_sink; pub(super) use sink::execute_sink_valid_rows; diff --git a/vortex-array/src/scalar_fn/unstable/row/execute/outcome.rs b/vortex-array/src/scalar_fn/unstable/row/execute/outcome.rs deleted file mode 100644 index fc013e7a317..00000000000 --- a/vortex-array/src/scalar_fn/unstable/row/execute/outcome.rs +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: Copyright the Vortex contributors - -//! The result of a completed row loop before batch-level null handling. -//! -//! [`RowExecution`] preserves deferred failure evidence until batch execution can determine whether -//! the failing payload belonged to a valid row. - -use vortex_error::VortexError; -use vortex_error::VortexResult; - -use crate::ArrayRef; - -/// The outcome of a row loop before batch execution decides whether an error is observable. -/// -/// Together with the surrounding [`VortexResult`], this represents three outcomes: -/// -/// - `Err(error)` is a non-retryable execution or immediate row error. -/// - [`Output`](Self::Output) is a successful row loop. -/// - [`DeferredError`](Self::DeferredError) is failure evidence from a completed row loop. -/// -/// A dense loop can evaluate null payloads, so its deferred error is not always observable. Batch -/// execution can retry only valid rows to discard errors caused by null payloads. A plain -/// `VortexResult` cannot distinguish these errors from failures that a retry cannot fix. -/// -/// Once execution is known to contain only valid rows, converting this outcome into a -/// `VortexResult` turns [`DeferredError`](Self::DeferredError) into an ordinary error. -pub enum RowExecution { - /// The successfully built, full-length output column. - Output(ArrayRef), - - /// An error constructed from failure evidence reduced across a completed row loop. - DeferredError(VortexError), -} - -impl From for VortexResult { - fn from(execution: RowExecution) -> Self { - match execution { - RowExecution::Output(output) => Ok(output), - RowExecution::DeferredError(error) => Err(error), - } - } -} diff --git a/vortex-array/src/scalar_fn/unstable/row/execute/owned.rs b/vortex-array/src/scalar_fn/unstable/row/execute/owned.rs index 513178cd97c..cf459f14148 100644 --- a/vortex-array/src/scalar_fn/unstable/row/execute/owned.rs +++ b/vortex-array/src/scalar_fn/unstable/row/execute/owned.rs @@ -13,13 +13,12 @@ use vortex_compute::lane_kernels::IndexedSourceExt; use vortex_error::VortexResult; use vortex_error::vortex_ensure; -use super::RowExecution; +use crate::ArrayRef; use crate::ExecutionCtx; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::unstable::row::FailureEvidence; use crate::scalar_fn::unstable::row::IndexedElementTuple; use crate::scalar_fn::unstable::row::OutputElement; -use crate::scalar_fn::unstable::row::ViewLen; use crate::scalar_fn::unstable::row::visitor::assert_owned_output_needs_no_drop; /// Zero-sized failure accumulator for infallible owned visits. @@ -36,7 +35,7 @@ pub(crate) fn execute_owned_infallible( ctx: &mut ExecutionCtx, prepare: impl FnOnce(Args::ConstElems<'_>) -> Prepared, apply: impl Fn(&Prepared, Args::Elems<'_>) -> Out, -) -> VortexResult +) -> VortexResult where Args: IndexedElementTuple, Out: OutputElement, @@ -57,7 +56,7 @@ pub(crate) fn execute_owned( prepare: impl FnOnce(Args::ConstElems<'_>) -> Prepared, apply: impl Fn(&Prepared, Args::Elems<'_>) -> (Out, Fail), finish_failure: impl FnOnce(Fail) -> VortexResult<()>, -) -> VortexResult +) -> VortexResult where Args: IndexedElementTuple, Out: OutputElement, @@ -78,12 +77,12 @@ where let failure = if let Some(views) = Args::views_if_no_consts(&columns) { // Keep this validation beside the views so LLVM sees their common length here. vortex_ensure!( - Args::ARITY == 0 || views.len() == row_count, + Args::view_lens_match(&views, row_count), "a decoded row input does not address exactly {row_count} rows", ); - // SAFETY: the tuple length check proved every non-nullary view addresses exactly - // `row_count` rows immediately above. Nullary tuples do not access an input view. + // SAFETY: `view_lens_match` checked that these exact retained views address `row_count` + // rows. let source = unsafe { Args::indexed_source(views, row_count) }; source.map_checked_into(output, |elements| apply(&prepared, elements)) @@ -115,9 +114,8 @@ where // once, and `values` was allocated with at least `row_count` capacity. unsafe { values.set_len(row_count) }; - // Defer failures so batch execution can retry with only valid rows. - match finish_failure(failure) { - Ok(()) => Ok(RowExecution::Output(Out::build(values))), - Err(error) => Ok(RowExecution::DeferredError(error)), - } + // Defer rich error construction until after the row loop. + finish_failure(failure)?; + + Ok(Out::build(values)) } diff --git a/vortex-array/src/scalar_fn/unstable/row/execute/sink.rs b/vortex-array/src/scalar_fn/unstable/row/execute/sink.rs index 9b809179cae..9b9131db008 100644 --- a/vortex-array/src/scalar_fn/unstable/row/execute/sink.rs +++ b/vortex-array/src/scalar_fn/unstable/row/execute/sink.rs @@ -14,7 +14,7 @@ use vortex_error::vortex_ensure_eq; use vortex_mask::AllOr; use vortex_mask::Mask; -use super::RowExecution; +use crate::ArrayRef; use crate::ExecutionCtx; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::unstable::row::ElementTuple; @@ -32,7 +32,7 @@ pub(crate) fn execute_sink( ctx: &mut ExecutionCtx, prepare: impl FnOnce(Args::ConstElems<'_>) -> Prepared, apply: impl Fn(&Prepared, Args::Elems<'_>, >::Row<'_>) -> ApplyResult, -) -> VortexResult +) -> VortexResult where Args: ElementTuple, Sink: OutputSink, @@ -60,13 +60,13 @@ where let views = Args::views_if_no_consts(&columns); if let Some(views) = views { - if Args::ARITY != 0 && views.len() != row_count { + if !Args::view_lens_match(&views, row_count) { decoded_length_error(row_count)?; } for index in 0..row_count { - // SAFETY: the tuple length check proved every non-nullary view has `row_count` - // rows before the loop. Nullary tuples do not access an input view. + // SAFETY: `view_lens_match` checked that these exact retained views address + // `row_count` rows before the loop. let elements = unsafe { Args::get_from_views_unchecked(&views, index) }; // SAFETY: the sink row-count check above proved every loop index is in bounds. let output = @@ -92,21 +92,21 @@ where } // SAFETY: every row callback completed successfully, so each returned the required write token. - unsafe { >::finish(sink) }.map(RowExecution::Output) + unsafe { >::finish(sink) } } /// Write only the rows set in `valid`, or decline when the inputs or sink cannot support /// skip-invalid execution. /// -/// `Ok(None)` signals batch execution to filter every input to the valid rows, run the dense -/// kernel, and scatter the results back into a null-padded array. +/// `Ok(None)` signals that direct skip-invalid execution is unavailable. Batch execution decides +/// how to handle the decline. pub(crate) fn execute_sink_valid_rows( args: &dyn ExecutionArgs, valid: &Mask, ctx: &mut ExecutionCtx, prepare: impl FnOnce(Args::ConstElems<'_>) -> Prepared, apply: impl Fn(&Prepared, Args::Elems<'_>, >::Row<'_>) -> ApplyResult, -) -> VortexResult> +) -> VortexResult> where Args: ElementTuple, Sink: OutputSink, @@ -144,7 +144,7 @@ where ); if let Some(views) = views { - if Args::ARITY != 0 && views.len() != row_count { + if !Args::view_lens_match(&views, row_count) { decoded_length_error(row_count)?; } @@ -154,9 +154,8 @@ where let output = unsafe { >::row_unchecked(&mut rows, index) }; - // SAFETY: the tuple length check proved every non-nullary view has `row_count` - // rows, and mask indices are below `row_count`. Nullary tuples do not access an - // input view. + // SAFETY: `view_lens_match` checked that these exact retained views address + // `row_count` rows, and mask indices are below `row_count`. let elements = unsafe { Args::get_from_views_unchecked(&views, index) }; apply(&prepared, elements, output).into_result() @@ -179,9 +178,7 @@ where // SAFETY: the initializer completed before traversal, and every visited callback completed // successfully and returned the required write token. - unsafe { >::finish(sink) } - .map(RowExecution::Output) - .map(Some) + unsafe { >::finish(sink) }.map(Some) } /// Construct a decoded-length error outside the traversal branches. @@ -267,21 +264,17 @@ mod tests { use vortex_error::vortex_err; use vortex_mask::Mask; - use super::RowExecution; use super::execute_sink_valid_rows; use crate::ArrayRef; use crate::IntoArray; use crate::VortexSessionExecute; use crate::array_session; use crate::arrays::PrimitiveArray; - use crate::assert_arrays_eq; use crate::dtype::DType; use crate::dtype::NativePType; use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::VecExecutionArgs; - use crate::scalar_fn::unstable::row::InitializedElement; use crate::scalar_fn::unstable::row::OutputSink; - use crate::scalar_fn::unstable::row::UninitElementSink; use crate::validity::Validity; struct NonSkippingSink; @@ -370,39 +363,6 @@ mod tests { Ok(()) } - #[test] - fn test_skip_invalid_sink_initializes_and_writes_addressed_rows() -> VortexResult<()> { - let input = PrimitiveArray::from_iter([10_i64, 20, 30]).into_array(); - let args = VecExecutionArgs::new(vec![input], 3); - let valid = Mask::from_iter([true, false, true]); - let mut ctx = array_session().create_execution_ctx(); - - let execution = execute_sink_valid_rows::< - (i64,), - (), - UninitElementSink, - InitializedElement, - EmptyOptions, - >( - &args, - &valid, - &mut ctx, - |_| (), - |_, (value,), output| { - // SAFETY: `output` is the row supplied to this callback. - unsafe { InitializedElement::write(output, value * 2) } - }, - )?; - let Some(RowExecution::Output(actual)) = execution else { - vortex_bail!("the skip-invalid sink must produce an output"); - }; - let expected = PrimitiveArray::from_iter([20_i64, 0, 60]); - - assert_arrays_eq!(&actual, expected.as_ref(), &mut ctx); - - Ok(()) - } - #[test] fn test_skip_invalid_sink_rechecks_rows_after_initialization() -> VortexResult<()> { let input = PrimitiveArray::from_iter([10_i64, 20]).into_array(); diff --git a/vortex-array/src/scalar_fn/unstable/row/mod.rs b/vortex-array/src/scalar_fn/unstable/row/mod.rs index 12ce0db87c2..b863b8d24de 100644 --- a/vortex-array/src/scalar_fn/unstable/row/mod.rs +++ b/vortex-array/src/scalar_fn/unstable/row/mod.rs @@ -14,13 +14,11 @@ //! Unlike a general strict function, a [`RowFn`] cannot produce null from valid inputs. //! //! Prepared visits move work derived from constant operands outside the hot loop. Deferred visits -//! reduce compact failure evidence in that loop and retry only valid rows when null payloads may -//! have caused the failure. +//! reduce compact failure evidence without constructing errors in that loop. // TODO(connor)[RowFn]: Remove this expectation when #9450 connects the batch executor. #[expect(dead_code)] mod execute; -pub use execute::RowExecution; mod row_fn; pub use row_fn::RowFn; diff --git a/vortex-array/src/scalar_fn/unstable/row/row_fn.rs b/vortex-array/src/scalar_fn/unstable/row/row_fn.rs index 831cc25f251..dd183d13e58 100644 --- a/vortex-array/src/scalar_fn/unstable/row/row_fn.rs +++ b/vortex-array/src/scalar_fn/unstable/row/row_fn.rs @@ -40,13 +40,13 @@ pub trait RowFn: 'static + Sized + Clone + Send + Sync { /// The arguments in display order. Its length is the function's exact arity. const ARG_NAMES: &'static [&'static str]; - /// Whether any dispatch can raise a semantic error. + /// Whether every dispatch is infallible. /// /// See [`ScalarFnVTable::is_fallible`](crate::scalar_fn::ScalarFnVTable::is_fallible) for a /// more detailed explanation of semantic errors. /// - /// The framework checks dispatched element and result types. A conservative `true` is allowed. - const FALLIBLE: bool; + /// The framework checks dispatched element and result types. A conservative `false` is allowed. + const INFALLIBLE: bool; /// Returns the ID of the scalar function. fn id(&self) -> ScalarFnId; diff --git a/vortex-array/src/scalar_fn/unstable/row/types/element/input.rs b/vortex-array/src/scalar_fn/unstable/row/types/element/input.rs index bc047721310..0b7a8078cfe 100644 --- a/vortex-array/src/scalar_fn/unstable/row/types/element/input.rs +++ b/vortex-array/src/scalar_fn/unstable/row/types/element/input.rs @@ -54,8 +54,8 @@ pub unsafe trait InputElement: 'static { /// Decode `array` into its column representation. /// - /// Called once per row-kernel invocation, including deferred-error retries. Hoist dtype checks, - /// downcasts, and other invocation-invariant work into this method. + /// Called once per row-kernel invocation. Hoist dtype checks, downcasts, and other + /// invocation-invariant work into this method. fn decode(array: ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult; /// Whether [`decode_null_tolerant`](Self::decode_null_tolerant) can decode this array. diff --git a/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/element_tuple.rs b/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/element_tuple.rs index af42ff56868..b251ede1575 100644 --- a/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/element_tuple.rs +++ b/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/element_tuple.rs @@ -175,8 +175,6 @@ pub trait ElementTuple: 'static + private::Sealed { fn validate(dtypes: &[DType]) -> VortexResult<()>; /// Decode every input column once for one row-kernel invocation. - /// - /// A dense deferred-error retry starts another invocation over filtered valid rows. fn decode(args: &dyn ExecutionArgs, ctx: &mut ExecutionCtx) -> VortexResult; /// Whether every input can be decoded without assuming that all rows are valid. @@ -206,15 +204,19 @@ pub trait ElementTuple: 'static + private::Sealed { /// returned tuple removes batch-constant checks from the row loop. fn views_if_no_consts(columns: &Self::Columns) -> Option>; - /// Whether every argument decoded at full batch length contains exactly `row_count` rows. + /// Whether every borrowed view contains exactly `row_count` rows. /// - /// NB: `Columns` cannot implement [`ViewLen`] because a batch-constant [`ArgColumn`] physically - /// contains one decoded row while logically addressing every row in the batch. By contrast, - /// [`views_if_no_consts`](Self::views_if_no_consts) constructs `Views` only when every argument - /// is non-constant, so those views have one common physical length. + /// Executors must validate each retained view before unchecked traversal. [`ViewLen::len`] on + /// a tuple asserts that component lengths are equal and panics on a mismatch. Rebuilding views + /// from [`Columns`](Self::Columns) does not prove the lengths of the views passed to + /// [`get_from_views_unchecked`](Self::get_from_views_unchecked). + fn view_lens_match(views: &Self::Views<'_>, row_count: usize) -> bool; + + /// Whether every argument decoded at full batch length contains exactly `row_count` rows. /// - /// This check runs once before the hot loop. A batch constant is exempt because its - /// [`ArgColumn`] constructor already validated the one-row representation produced by decoding. + /// `Columns` cannot implement [`ViewLen`] because a batch-constant `ArgColumn` stores one + /// decoded row while logically addressing the full batch. The batch-constant constructor + /// validates that one-row representation, so this method checks only non-constant columns. fn decoded_lens_match(columns: &Self::Columns, row_count: usize) -> bool; /// Read one row from borrowed views. @@ -281,6 +283,10 @@ impl ElementTuple for () { Some(()) } + fn view_lens_match(_views: &Self::Views<'_>, _row_count: usize) -> bool { + true + } + fn decoded_lens_match(_columns: &Self::Columns, _row_count: usize) -> bool { true } @@ -361,6 +367,10 @@ macro_rules! element_tuple { Some(($($t::view(columns.$idx.as_column()?),)+)) } + fn view_lens_match(views: &Self::Views<'_>, row_count: usize) -> bool { + $(views.$idx.len() == row_count &&)+ true + } + fn decoded_lens_match(columns: &Self::Columns, row_count: usize) -> bool { $(columns.$idx.addresses_rows(row_count) &&)+ true } diff --git a/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/tests.rs b/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/tests.rs index 4f8ea67348c..55d6e267329 100644 --- a/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/tests.rs +++ b/vortex-array/src/scalar_fn/unstable/row/types/element/tuple/tests.rs @@ -90,6 +90,14 @@ i64_test_element!( ); i64_test_element!(DenseFallible, false); +#[test] +fn test_view_lens_match_checks_each_view() { + let first: &[i64] = &[1, 2]; + let second: &[i64] = &[3]; + + assert!(!<(i64, i64)>::view_lens_match(&(first, second), 2)); +} + #[test] fn test_null_tolerant_decline_precedes_decoding() -> VortexResult<()> { DECODE_CALLS.store(0, Ordering::Relaxed); diff --git a/vortex-array/src/scalar_fn/unstable/row/types/result.rs b/vortex-array/src/scalar_fn/unstable/row/types/result.rs index 55c34da0a49..42914150691 100644 --- a/vortex-array/src/scalar_fn/unstable/row/types/result.rs +++ b/vortex-array/src/scalar_fn/unstable/row/types/result.rs @@ -28,8 +28,8 @@ pub trait SinkResult: 'static + private::Sealed { /// The [`OutputSink::WriteToken`](super::OutputSink::WriteToken) carried by a success. type WriteToken: 'static; - /// Whether this return type can carry an error. - const FALLIBLE: bool; + /// Whether this return type is infallible. + const INFALLIBLE: bool; /// Convert this row's outcome into immediate success or failure. fn into_result(self) -> VortexResult<()>; @@ -39,7 +39,7 @@ impl private::Sealed for () {} impl SinkResult for () { type WriteToken = (); - const FALLIBLE: bool = false; + const INFALLIBLE: bool = true; fn into_result(self) -> VortexResult<()> { Ok(()) @@ -50,7 +50,7 @@ impl private::Sealed for InitializedElement {} impl SinkResult for InitializedElement { type WriteToken = InitializedElement; - const FALLIBLE: bool = false; + const INFALLIBLE: bool = true; fn into_result(self) -> VortexResult<()> { Ok(()) @@ -61,7 +61,7 @@ impl private::Sealed for VortexResult<()> {} impl SinkResult for VortexResult<()> { type WriteToken = (); - const FALLIBLE: bool = true; + const INFALLIBLE: bool = false; fn into_result(self) -> VortexResult<()> { self @@ -72,7 +72,7 @@ impl private::Sealed for VortexResult {} impl SinkResult for VortexResult { type WriteToken = InitializedElement; - const FALLIBLE: bool = true; + const INFALLIBLE: bool = false; fn into_result(self) -> VortexResult<()> { self.map(|_| ()) diff --git a/vortex-array/src/scalar_fn/unstable/row/types/sink.rs b/vortex-array/src/scalar_fn/unstable/row/types/sink.rs index e95431bf9e6..747d68d47f4 100644 --- a/vortex-array/src/scalar_fn/unstable/row/types/sink.rs +++ b/vortex-array/src/scalar_fn/unstable/row/types/sink.rs @@ -28,7 +28,7 @@ use crate::scalar_fn::unstable::row::ViewLen; /// /// Lifecycle methods report only incidental failures such as allocation. A semantic error that /// depends on input values **must** come from the row callback through a fallible [`SinkResult`], -/// or [`RowFn::FALLIBLE`] cannot protect optimizations such as dictionary push-down. +/// or [`RowFn::INFALLIBLE`] cannot protect optimizations such as dictionary push-down. /// /// # Safety /// @@ -54,7 +54,7 @@ use crate::scalar_fn::unstable::row::ViewLen; /// [`Rows`]: Self::Rows /// [`WriteToken`]: Self::WriteToken /// [`finish`]: Self::finish -/// [`RowFn::FALLIBLE`]: crate::scalar_fn::unstable::row::RowFn::FALLIBLE +/// [`RowFn::INFALLIBLE`]: crate::scalar_fn::unstable::row::RowFn::INFALLIBLE /// [`SinkResult`]: crate::scalar_fn::unstable::row::SinkResult /// [`skipped_rows_initializer`]: Self::skipped_rows_initializer pub unsafe trait OutputSink: 'static + Sized { diff --git a/vortex-array/src/scalar_fn/unstable/row/types/view.rs b/vortex-array/src/scalar_fn/unstable/row/types/view.rs index 18085a12d14..6bac339d822 100644 --- a/vortex-array/src/scalar_fn/unstable/row/types/view.rs +++ b/vortex-array/src/scalar_fn/unstable/row/types/view.rs @@ -85,14 +85,6 @@ impl_tuple_view_len!(A; B: 1, C: 2, D: 3, E: 4, F: 5, G: 6, H: 7, I: 8, J: 9, K: mod tests { use super::ViewLen; - #[test] - fn tuple_len_returns_common_len() { - let first: &[i64] = &[1, 2]; - let second: &[i64] = &[3, 4]; - - assert_eq!((first, second).len(), 2); - } - #[test] #[should_panic(expected = "tuple views must have equal lengths")] fn tuple_len_rejects_mismatch() { diff --git a/vortex-array/src/scalar_fn/unstable/row/visitor/check.rs b/vortex-array/src/scalar_fn/unstable/row/visitor/check.rs index 7606f82f0b5..5611e779b44 100644 --- a/vortex-array/src/scalar_fn/unstable/row/visitor/check.rs +++ b/vortex-array/src/scalar_fn/unstable/row/visitor/check.rs @@ -36,8 +36,8 @@ const fn assert_input_visit_contract() { // Dictionary push-down can evaluate values that no input row references. Every dispatch must // therefore match the function-wide fallibility declaration. assert!( - Args::DECODE_INFALLIBLE || F::FALLIBLE, - "RowFn::FALLIBLE must be true when input decoding can fail", + Args::DECODE_INFALLIBLE || !F::INFALLIBLE, + "RowFn::INFALLIBLE must be false when input decoding can fail", ); } @@ -59,8 +59,8 @@ where { assert_input_visit_contract::(); assert!( - !ApplyResult::FALLIBLE || Function::FALLIBLE, - "RowFn::FALLIBLE must be true when a row result can fail", + ApplyResult::INFALLIBLE || !Function::INFALLIBLE, + "RowFn::INFALLIBLE must be false when a row result can fail", ); } @@ -73,8 +73,8 @@ where { assert_owned_visit_contract::(); assert!( - Function::FALLIBLE, - "RowFn::FALLIBLE must be true when a row result defers failure evidence", + !Function::INFALLIBLE, + "RowFn::INFALLIBLE must be false when a row result defers failure evidence", ); assert!( size_of::() <= size_of::(), diff --git a/vortex-array/src/scalar_fn/unstable/row/visitor/execute.rs b/vortex-array/src/scalar_fn/unstable/row/visitor/execute.rs index 610568343f3..93fd640b82e 100644 --- a/vortex-array/src/scalar_fn/unstable/row/visitor/execute.rs +++ b/vortex-array/src/scalar_fn/unstable/row/visitor/execute.rs @@ -3,9 +3,9 @@ //! Visitors that execute dense and skip-invalid row loops. //! -//! Each visit revalidates its concrete signature and checks that its output dtype and null policy -//! match the plan before entering a row loop. [`ExecuteValidRows`] can decline, so the batch layer -//! filters the inputs and retries with [`ExecuteRows`]. +//! Each visit revalidates its concrete signature and checks that its output dtype and execution +//! policy match the plan before entering a row loop. [`ExecuteValidRows`] can decline when the +//! signature cannot execute over the original inputs. use vortex_error::VortexResult; use vortex_error::vortex_ensure_eq; @@ -19,6 +19,7 @@ use super::check::assert_sink_visit_contract; use super::check::validate_owned_visit; use super::check::validate_sink_visit; use super::row_visitor::private; +use crate::ArrayRef; use crate::ExecutionCtx; use crate::dtype::DType; use crate::scalar_fn::ExecutionArgs; @@ -29,7 +30,6 @@ use crate::scalar_fn::unstable::row::OutputElement; use crate::scalar_fn::unstable::row::OutputSink; use crate::scalar_fn::unstable::row::RowFn; use crate::scalar_fn::unstable::row::SinkResult; -use crate::scalar_fn::unstable::row::execute::RowExecution; use crate::scalar_fn::unstable::row::execute::execute_owned; use crate::scalar_fn::unstable::row::execute::execute_owned_infallible; use crate::scalar_fn::unstable::row::execute::execute_sink; @@ -79,7 +79,7 @@ impl<'args, 'ctx, F: RowFn> ExecuteRows<'args, 'ctx, F> { impl private::Sealed for ExecuteRows<'_, '_, F> {} impl RowVisitor for ExecuteRows<'_, '_, F> { - type VisitResult = RowExecution; + type VisitResult = ArrayRef; fn visit_prepared( self, @@ -159,8 +159,8 @@ impl RowVisitor for ExecuteRows<'_, '_, F> { /// The runtime visit that executes valid rows over the original input columns. /// -/// Only output sinks have a contract for skipped output positions. Owned visits therefore decline -/// so batch execution can filter the valid inputs and scatter the output back. +/// Only output sinks have a contract for skipped output positions. Owned visits therefore decline, +/// and batch execution decides how to handle that unsupported signature. pub(crate) struct ExecuteValidRows<'args, 'ctx, F: RowFn> { /// The original inputs for this kernel invocation. args: &'args dyn ExecutionArgs, @@ -209,7 +209,7 @@ impl<'args, 'ctx, F: RowFn> ExecuteValidRows<'args, 'ctx, F> { impl private::Sealed for ExecuteValidRows<'_, '_, F> {} impl RowVisitor for ExecuteValidRows<'_, '_, F> { - type VisitResult = Option; + type VisitResult = Option; fn visit_prepared( self, @@ -228,8 +228,6 @@ impl RowVisitor for ExecuteValidRows<'_, '_, F> { RowPolicy::for_owned_output::(), )?; - // Owned execution has no sink that can initialize skipped output positions. Decline so - // batch execution filters the inputs and retries with the dense visitor. Ok(None) } @@ -279,7 +277,6 @@ impl RowVisitor for ExecuteValidRows<'_, '_, F> { RowPolicy::for_deferred_output::(), )?; - // Deferred owned execution has the same skipped-output limitation as `visit_prepared`. Ok(None) } } diff --git a/vortex-array/src/scalar_fn/unstable/row/visitor/plan.rs b/vortex-array/src/scalar_fn/unstable/row/visitor/plan.rs index fe3c5e32bb3..a742045749d 100644 --- a/vortex-array/src/scalar_fn/unstable/row/visitor/plan.rs +++ b/vortex-array/src/scalar_fn/unstable/row/visitor/plan.rs @@ -135,10 +135,7 @@ pub(crate) enum RowPolicy { /// Evaluate all rows and mask the result. Dense, - /// Evaluate all rows, retrying only valid rows if a deferred error is raised. - DenseWithRetry, - - /// Execute only valid rows over the original inputs before filtering. + /// Execute only valid rows over the original inputs. ValidOnly, } @@ -154,16 +151,13 @@ impl RowPolicy { /// The policy for an owned output carrying batch-deferred failure evidence. pub(crate) const fn for_deferred_output() -> Self { - if Args::DENSE_SAFE && Args::DECODE_INFALLIBLE { - Self::DenseWithRetry - } else { - Self::ValidOnly - } + let _ = PhantomData::; + Self::ValidOnly } /// The policy for a sink-writing output. pub(crate) const fn for_sink() -> Self { - if Args::DENSE_SAFE && Args::DECODE_INFALLIBLE && !ApplyResult::FALLIBLE { + if Args::DENSE_SAFE && Args::DECODE_INFALLIBLE && ApplyResult::INFALLIBLE { Self::Dense } else { Self::ValidOnly diff --git a/vortex-array/src/scalar_fn/unstable/row/visitor/row_visitor.rs b/vortex-array/src/scalar_fn/unstable/row/visitor/row_visitor.rs index ce46e53210b..3add5e918d1 100644 --- a/vortex-array/src/scalar_fn/unstable/row/visitor/row_visitor.rs +++ b/vortex-array/src/scalar_fn/unstable/row/visitor/row_visitor.rs @@ -21,10 +21,10 @@ use crate::scalar_fn::unstable::row::SinkResult; /// /// Only the framework implements this trait. The `visit_prepared*` methods derive shared state /// from constant arguments before visiting any rows. Every visit verifies that the argument tuple -/// matches [`RowFn::ARG_NAMES`] and that fallible decoding agrees with [`RowFn::FALLIBLE`]. +/// matches [`RowFn::ARG_NAMES`] and that fallible decoding agrees with [`RowFn::INFALLIBLE`]. /// /// [`RowFn::ARG_NAMES`]: crate::scalar_fn::unstable::row::RowFn::ARG_NAMES -/// [`RowFn::FALLIBLE`]: crate::scalar_fn::unstable::row::RowFn::FALLIBLE +/// [`RowFn::INFALLIBLE`]: crate::scalar_fn::unstable::row::RowFn::INFALLIBLE pub trait RowVisitor: private::Sealed + Sized { /// The framework result of visiting one concrete row signature. /// @@ -109,7 +109,7 @@ pub trait RowVisitor: private::Sealed + Sized { /// another row, sink, or local cell can violate the safety contract of [`OutputSink::finish`]. /// /// A fallible `ApplyResult` requires - /// [`RowFn::FALLIBLE`](crate::scalar_fn::unstable::row::RowFn::FALLIBLE) to be `true`. + /// [`RowFn::INFALLIBLE`](crate::scalar_fn::unstable::row::RowFn::INFALLIBLE) to be `false`. /// /// # Examples /// @@ -199,7 +199,7 @@ pub trait RowVisitor: private::Sealed + Sized { /// The executor OR-reduces [`FailureEvidence`] across rows and passes the result to /// `finish_failure`. /// - /// [`RowFn::FALLIBLE`](crate::scalar_fn::unstable::row::RowFn::FALLIBLE) **must** be `true`. + /// [`RowFn::INFALLIBLE`](crate::scalar_fn::unstable::row::RowFn::INFALLIBLE) **must** be `false`. /// `Out` must not require drop glue. `Fail` must be no wider than `Out`, or failure tracking /// reduces the vector width. The framework checks these requirements. /// diff --git a/vortex-array/src/scalar_fn/unstable/row/vtable.rs b/vortex-array/src/scalar_fn/unstable/row/vtable.rs index b6c14585c48..317fae1ed17 100644 --- a/vortex-array/src/scalar_fn/unstable/row/vtable.rs +++ b/vortex-array/src/scalar_fn/unstable/row/vtable.rs @@ -74,7 +74,7 @@ impl ScalarFnVTable for F { } fn is_fallible(&self, _options: &Self::Options) -> bool { - F::FALLIBLE + !F::INFALLIBLE } } @@ -149,7 +149,7 @@ mod tests { const ARG_NAMES: &'static [&'static str] = &["value"]; - const FALLIBLE: bool = false; + const INFALLIBLE: bool = true; fn id(&self) -> ScalarFnId { static ID: CachedId = CachedId::new("test.indexing_row_fn");