Skip to content

Commit 6fdcd8e

Browse files
committed
Simplify the RowFn public API
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
1 parent 1139f60 commit 6fdcd8e

18 files changed

Lines changed: 766 additions & 806 deletions

File tree

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,6 @@ use crate::scalar_fn::fns::ext_storage::ExtStorage;
1919
use crate::scalar_fn::fns::get_item::GetItem;
2020
use crate::scalar_fn::fns::literal::Literal;
2121

22-
/// Reexports used by [`impl_row_fn_vtable!`] without downstream transitive dependencies.
23-
#[doc(hidden)]
24-
pub mod row_fn_macro_support {
25-
pub use vortex_error::VortexResult;
26-
pub use vortex_session::VortexSession;
27-
}
28-
2922
mod vtable;
3023
pub use vtable::*;
3124

‎vortex-array/src/scalar_fn/row/batch/execution.rs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use crate::ArrayRef;
1919
use crate::ExecutionCtx;
2020
use crate::IntoArray;
2121
use crate::arrays::BoolArray;
22+
use crate::arrays::Constant;
2223
use crate::arrays::ConstantArray;
2324
use crate::arrays::MaskedArray;
2425
use crate::arrays::PrimitiveArray;
@@ -141,10 +142,11 @@ impl Batch {
141142
// Strictness: an all-null batch has no observable row work. Keep the literal-constant
142143
// check explicit alongside the conjoined validity invariant.
143144
if matches!(self.validity, Validity::AllInvalid)
144-
|| self
145-
.inputs
146-
.iter()
147-
.any(|input| input.as_constant().is_some_and(|scalar| scalar.is_null()))
145+
|| self.inputs.iter().any(|input| {
146+
input
147+
.as_opt::<Constant>()
148+
.is_some_and(|constant| constant.scalar().is_null())
149+
})
148150
{
149151
return Ok(self.all_null());
150152
}

‎vortex-array/src/scalar_fn/row/batch/policy.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,11 @@ mod tests {
8585

8686
struct SparseFallibleElement;
8787

88-
// SAFETY: the varying view reports length zero, so no index satisfies the unchecked-read
88+
// SAFETY: the per-row view reports length zero, so no index satisfies the unchecked-read
8989
// precondition.
9090
unsafe impl InputElement for SparseFallibleElement {
9191
type Column = ();
92-
type Varying<'a> = ();
92+
type View<'a> = ();
9393
type Elem<'a> = ();
9494

9595
const DENSE_SAFE: bool = false;
@@ -105,13 +105,13 @@ mod tests {
105105

106106
fn get(_column: &Self::Column, _index: usize) -> Self::Elem<'_> {}
107107

108-
fn varying(_column: &Self::Column) -> Self::Varying<'_> {}
108+
fn view(_column: &Self::Column) -> Self::View<'_> {}
109109

110-
fn varying_len(_column: &Self::Varying<'_>) -> usize {
110+
fn view_len(_view: &Self::View<'_>) -> usize {
111111
0
112112
}
113113

114-
fn get_varying<'a>(_column: &Self::Varying<'a>, _index: usize) -> Self::Elem<'a> {}
114+
fn get_from_view<'a>(_view: &Self::View<'a>, _index: usize) -> Self::Elem<'a> {}
115115
}
116116

117117
#[test]

‎vortex-array/src/scalar_fn/row/batch/tests.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -690,11 +690,11 @@ fn assert_invalid_kernel_output(input: ArrayRef) -> VortexResult<()> {
690690

691691
#[rstest]
692692
#[case::owned_constant(PreparedVisit::Owned, true)]
693-
#[case::owned_varying(PreparedVisit::Owned, false)]
693+
#[case::owned_per_row(PreparedVisit::Owned, false)]
694694
#[case::sink_constant(PreparedVisit::Sink, true)]
695-
#[case::sink_varying(PreparedVisit::Sink, false)]
695+
#[case::sink_per_row(PreparedVisit::Sink, false)]
696696
#[case::deferred_constant(PreparedVisit::Deferred, true)]
697-
#[case::deferred_varying(PreparedVisit::Deferred, false)]
697+
#[case::deferred_per_row(PreparedVisit::Deferred, false)]
698698
fn test_prepared_visits(
699699
#[case] visit: PreparedVisit,
700700
#[case] constant_rhs: bool,
@@ -771,7 +771,7 @@ fn test_nullary_row_function_broadcasts() -> VortexResult<()> {
771771
}
772772

773773
#[test]
774-
fn test_owned_execution_traverses_three_varying_inputs() -> VortexResult<()> {
774+
fn test_owned_execution_traverses_three_per_row_inputs() -> VortexResult<()> {
775775
let args = VecExecutionArgs::new(
776776
vec![
777777
PrimitiveArray::from_iter([1_i64, 2, 3]).into_array(),

‎vortex-array/src/scalar_fn/row/execute/mod.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,11 @@ impl From<RowExecution> for VortexResult<ArrayRef> {
5656
/// Ensure that every decoded input addresses the complete row loop.
5757
pub(super) fn ensure_decoded_lengths<Args: ElementTuple>(
5858
columns: &Args::Columns,
59-
varying: Option<&Args::VaryingColumns<'_>>,
59+
views: Option<&Args::Views<'_>>,
6060
row_count: usize,
6161
) -> VortexResult<()> {
62-
let lengths_match = match varying {
63-
Some(varying) => Args::varying_len_matches(varying, row_count),
62+
let lengths_match = match views {
63+
Some(views) => Args::view_lens_match(views, row_count),
6464
None => Args::decoded_lens_match(columns, row_count),
6565
};
6666
vortex_ensure!(

‎vortex-array/src/scalar_fn/row/execute/owned.rs‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -71,25 +71,26 @@ where
7171
{
7272
let output = &mut values.spare_capacity_mut()[..row_count];
7373

74-
// When every input varies, the indexed source removes argument-shape dispatch from the hot
75-
// loop and lets the lane kernel optimize the traversal as one operation. Keep the varying
76-
// view and its length proof in this branch: hoisting them through the shared validation
77-
// helper changed mixed-constant add, subtract, and multiply from 9.219, 9.229, and 18.94 us
78-
// to 30.46, 31.11, and 37.73 us on a Ryzen 9 7950X with rustc 1.91.0 and LLVM 21.1.2.
74+
// When every input stores one value per row, the indexed source removes argument-shape
75+
// dispatch from the hot loop and lets the lane kernel optimize the traversal as one
76+
// operation. Keep view construction and its length proof in this branch. Hoisting them
77+
// through the shared validation helper changed mixed-constant add, subtract, and multiply
78+
// from 9.219, 9.229, and 18.94 us to 30.46, 31.11, and 37.73 us on a Ryzen 9 7950X with
79+
// rustc 1.91.0 and LLVM 21.1.2.
7980
// Restoring this placement recovered the fast code under the 16-CGU, no-LTO bench profile.
80-
if let Some(varying) = Args::varying(&columns) {
81+
if let Some(views) = Args::per_row_views(&columns) {
8182
vortex_ensure!(
82-
Args::varying_len_matches(&varying, row_count),
83+
Args::view_lens_match(&views, row_count),
8384
"a decoded row input does not address exactly {row_count} rows",
8485
);
8586

86-
// SAFETY: `varying_len_matches` proved every column addresses exactly `row_count`
87-
// rows immediately above.
88-
failure = unsafe { Args::indexed_source(varying, row_count) }
87+
// SAFETY: `view_lens_match` proved every view addresses exactly `row_count` rows
88+
// immediately above.
89+
failure = unsafe { Args::indexed_source(views, row_count) }
8990
.map_checked_into(output, |elements| apply(&prepared, elements));
9091
} else {
9192
// A batch-constant input was collapsed to one row during decoding. This path reads that
92-
// row repeatedly while indexing only the inputs that vary.
93+
// row repeatedly while indexing only the per-row inputs.
9394
vortex_ensure!(
9495
Args::decoded_lens_match(&columns, row_count),
9596
"a decoded row input does not address exactly {row_count} rows",

‎vortex-array/src/scalar_fn/row/execute/sink.rs‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ where
3838
let mut sink = <Sink as OutputSink<Options>>::with_capacity(row_count, sink_dtype)?;
3939
let columns = Args::decode(args, ctx)?;
4040
let prepared = prepare(Args::constants(&columns));
41-
let varying = Args::varying(&columns);
42-
ensure_decoded_lengths::<Args>(&columns, varying.as_ref(), row_count)?;
41+
let views = Args::per_row_views(&columns);
42+
ensure_decoded_lengths::<Args>(&columns, views.as_ref(), row_count)?;
4343

4444
{
4545
// Borrow the sink once so its shape and buffer descriptor remain loop invariants. This
@@ -50,13 +50,13 @@ where
5050
"the output sink does not address exactly {row_count} rows",
5151
);
5252

53-
// The all-varying representation removes argument-shape dispatch from the hot loop. The
53+
// The all-per-row representation removes argument-shape dispatch from the hot loop. The
5454
// mixed path instead reads collapsed batch constants at row zero.
55-
if let Some(varying) = varying {
55+
if let Some(views) = views {
5656
for index in 0..row_count {
57-
// SAFETY: `ensure_decoded_lengths` proved every varying column has `row_count`
58-
// rows before the loop.
59-
let elements = unsafe { Args::get_varying_unchecked(&varying, index) };
57+
// SAFETY: `ensure_decoded_lengths` proved every view has `row_count` rows before
58+
// the loop.
59+
let elements = unsafe { Args::get_from_views_unchecked(&views, index) };
6060
apply(
6161
&prepared,
6262
elements,
@@ -95,7 +95,7 @@ where
9595
{
9696
// Decline before input decoding or sink allocation when this sink cannot initialize rows that
9797
// the mask skips. The capability and the operation are the same function pointer.
98-
let Some(initialize_skipped_rows) = <Sink as OutputSink<Options>>::SKIPPED_ROWS_INITIALIZER
98+
let Some(initialize_skipped_rows) = <Sink as OutputSink<Options>>::skipped_rows_initializer()
9999
else {
100100
return Ok(None);
101101
};
@@ -125,8 +125,8 @@ where
125125
"the output sink does not address exactly {row_count} rows",
126126
);
127127

128-
let varying = Args::varying(&columns);
129-
ensure_decoded_lengths::<Args>(&columns, varying.as_ref(), row_count)?;
128+
let views = Args::per_row_views(&columns);
129+
ensure_decoded_lengths::<Args>(&columns, views.as_ref(), row_count)?;
130130

131131
// The loop writes only valid indices, but the sink still finishes a full-length output.
132132
// Initialize placeholders now; batch execution masks them before the result escapes.
@@ -140,12 +140,12 @@ where
140140
return;
141141
}
142142

143-
let result = match &varying {
144-
Some(varying) => apply(
143+
let result = match &views {
144+
Some(views) => apply(
145145
&prepared,
146-
// SAFETY: `ensure_decoded_lengths` proved every varying column has
147-
// `row_count` rows, and mask indices are below `row_count`.
148-
unsafe { Args::get_varying_unchecked(varying, index) },
146+
// SAFETY: `ensure_decoded_lengths` proved every view has `row_count` rows, and
147+
// mask indices are below `row_count`.
148+
unsafe { Args::get_from_views_unchecked(views, index) },
149149
<Sink as OutputSink<Options>>::row(&mut rows, index),
150150
),
151151
None => apply(

‎vortex-array/src/scalar_fn/row/row_fn.rs‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,10 @@ use crate::scalar_fn::ScalarFnId;
2323
/// Declare the argument names and use [`dispatch`](Self::dispatch) to choose concrete element and
2424
/// sink types for each accepted dtype combination.
2525
///
26-
/// A `RowFn` does not automatically implement
27-
/// [`ScalarFnVTable`](crate::scalar_fn::ScalarFnVTable). Invoke
28-
/// [`impl_row_fn_vtable!`](crate::impl_row_fn_vtable) to adopt its standard scalar-function
29-
/// behavior. A function that needs custom coercion, simplification, reduction, formatting, or
30-
/// validity hooks implements `ScalarFnVTable` itself and can delegate its `return_dtype` and
31-
/// `execute` methods to [`row_fn_return_dtype`](crate::scalar_fn::row_fn_return_dtype) and
26+
/// Every `RowFn` receives the standard [`ScalarFnVTable`](crate::scalar_fn::ScalarFnVTable)
27+
/// implementation. A function that needs custom scalar-function hooks instead implements
28+
/// `ScalarFnVTable` on its public type and delegates row execution to a private `RowFn` kernel with
29+
/// [`row_fn_return_dtype`](crate::scalar_fn::row_fn_return_dtype) and
3230
/// [`execute_rows`](crate::scalar_fn::execute_rows). Implement only `ScalarFnVTable` when the
3331
/// natural kernel is columnar rather than row-oriented.
3432
pub trait RowFn: 'static + Sized + Clone + Send + Sync {

‎vortex-array/src/scalar_fn/row/types/element/bool.rs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ use crate::scalar_fn::InputElement;
1515
use crate::scalar_fn::OutputElement;
1616
use crate::validity::Validity;
1717

18-
// SAFETY: the varying view is a bit buffer, and its reported length is the buffer length.
18+
// SAFETY: the per-row view is a bit buffer, and its reported length is the buffer length.
1919
unsafe impl InputElement for bool {
2020
type Column = BitBuffer;
21-
type Varying<'a> = &'a BitBuffer;
21+
type View<'a> = &'a BitBuffer;
2222
type Elem<'a> = bool;
2323

2424
// Every bit of the buffer is readable, valid or not.
@@ -41,27 +41,27 @@ unsafe impl InputElement for bool {
4141
column.value(index)
4242
}
4343

44-
fn varying(column: &Self::Column) -> Self::Varying<'_> {
44+
fn view(column: &Self::Column) -> Self::View<'_> {
4545
column
4646
}
4747

48-
fn varying_len(column: &Self::Varying<'_>) -> usize {
49-
column.len()
48+
fn view_len(view: &Self::View<'_>) -> usize {
49+
view.len()
5050
}
5151

52-
fn get_varying<'a>(column: &Self::Varying<'a>, index: usize) -> bool
52+
fn get_from_view<'a>(view: &Self::View<'a>, index: usize) -> bool
5353
where
5454
Self: 'a,
5555
{
56-
column.value(index)
56+
view.value(index)
5757
}
5858

59-
unsafe fn get_varying_unchecked<'a>(column: &Self::Varying<'a>, index: usize) -> bool
59+
unsafe fn get_from_view_unchecked<'a>(view: &Self::View<'a>, index: usize) -> bool
6060
where
6161
Self: 'a,
6262
{
6363
// SAFETY: forwarded from this method's contract.
64-
unsafe { column.value_unchecked(index) }
64+
unsafe { view.value_unchecked(index) }
6565
}
6666
}
6767

‎vortex-array/src/scalar_fn/row/types/element/mod.rs‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,20 +26,20 @@ pub use tuple::batch_constant;
2626
///
2727
/// # Safety
2828
///
29-
/// For every view returned by [`varying`](Self::varying), every index below
30-
/// [`varying_len`](Self::varying_len) **must** satisfy the safety contract of
31-
/// [`get_varying_unchecked`](Self::get_varying_unchecked). Shared execution relies on this proof to
32-
/// perform unchecked reads after one pre-loop length check.
29+
/// For every view returned by [`view`](Self::view), every index below
30+
/// [`view_len`](Self::view_len) **must** satisfy the safety contract of
31+
/// [`get_from_view_unchecked`](Self::get_from_view_unchecked). Shared execution relies on this
32+
/// proof to perform unchecked reads after one pre-loop length check.
3333
pub unsafe trait InputElement: 'static {
3434
/// The decoded column representation supporting `O(1)` row access.
3535
type Column;
3636

37-
/// The view of a varying decoded column read by the hot row loop.
37+
/// The view of a per-row decoded column read by the hot row loop.
3838
///
3939
/// This may borrow a cheaper representation than [`Column`](Self::Column). Primitive elements,
4040
/// for example, expose a slice so its pointer and length are loop invariants rather than
4141
/// re-reading a [`Buffer`](vortex_buffer::Buffer) descriptor for every row.
42-
type Varying<'a>;
42+
type View<'a>;
4343

4444
/// The borrowed element value handed to a row closure.
4545
type Elem<'a>;
@@ -48,8 +48,8 @@ pub unsafe trait InputElement: 'static {
4848
///
4949
/// Arrays only guarantee payloads for valid rows. This is `false` when a null row's stored
5050
/// offset or pointer may not address anything, and `true` only when [`decode`](Self::decode),
51-
/// [`get`](Self::get), [`varying`](Self::varying), [`varying_len`](Self::varying_len), and
52-
/// [`get_varying`](Self::get_varying) remain safe and correct for null rows.
51+
/// [`get`](Self::get), [`view`](Self::view), [`view_len`](Self::view_len), and
52+
/// [`get_from_view`](Self::get_from_view) remain safe and correct for null rows.
5353
///
5454
/// Dense execution requires this of every argument; otherwise the row layer executes only
5555
/// valid rows.
@@ -105,29 +105,29 @@ pub unsafe trait InputElement: 'static {
105105
///
106106
/// Called once before the hot loop. Constants do not use this view because the tuple adapter
107107
/// keeps their one-row decoded representation separate.
108-
fn varying(column: &Self::Column) -> Self::Varying<'_>;
108+
fn view(column: &Self::Column) -> Self::View<'_>;
109109

110-
/// Number of rows addressable through a [`Varying`](Self::Varying) view.
110+
/// Number of rows addressable through a [`View`](Self::View).
111111
///
112112
/// Every index below this length must be valid for
113-
/// [`get_varying_unchecked`](Self::get_varying_unchecked).
114-
fn varying_len(column: &Self::Varying<'_>) -> usize;
113+
/// [`get_from_view_unchecked`](Self::get_from_view_unchecked).
114+
fn view_len(view: &Self::View<'_>) -> usize;
115115

116-
/// Read one row from a [`Varying`](Self::Varying) view.
117-
fn get_varying<'a>(column: &Self::Varying<'a>, index: usize) -> Self::Elem<'a>
116+
/// Read one row from a [`View`](Self::View).
117+
fn get_from_view<'a>(view: &Self::View<'a>, index: usize) -> Self::Elem<'a>
118118
where
119119
Self: 'a;
120120

121121
/// Read one row without checking that `index` is in bounds.
122122
///
123123
/// # Safety
124124
///
125-
/// `index` must be less than [`varying_len`](Self::varying_len) for `column`.
126-
unsafe fn get_varying_unchecked<'a>(column: &Self::Varying<'a>, index: usize) -> Self::Elem<'a>
125+
/// `index` must be less than [`view_len`](Self::view_len) for `view`.
126+
unsafe fn get_from_view_unchecked<'a>(view: &Self::View<'a>, index: usize) -> Self::Elem<'a>
127127
where
128128
Self: 'a,
129129
{
130-
Self::get_varying(column, index)
130+
Self::get_from_view(view, index)
131131
}
132132
}
133133

0 commit comments

Comments
 (0)