Skip to content

Commit ca7d423

Browse files
committed
Gate PiecewiseSequence take paths on unit multipliers
Signed-off-by: Daniel King <dan@spiraldb.com>
1 parent da98e61 commit ca7d423

5 files changed

Lines changed: 52 additions & 26 deletions

File tree

vortex-array/benches/take_fsl.rs

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ use vortex_array::VortexSessionExecute;
2626
use vortex_array::array_session;
2727
use vortex_array::arrays::ConstantArray;
2828
use vortex_array::arrays::FixedSizeListArray;
29-
use vortex_array::arrays::PiecewiseSequentialArray;
29+
use vortex_array::arrays::PiecewiseSequenceArray;
3030
use vortex_array::arrays::PrimitiveArray;
3131
use vortex_array::arrays::fixed_size_list::FixedSizeListArrayExt;
3232
use vortex_array::dtype::IntegerPType;
@@ -143,7 +143,7 @@ fn take_fsl_f16_force_per_index<const LIST_SIZE: usize>(bencher: Bencher, num_in
143143
}
144144

145145
#[divan::bench(args = NUM_INDICES, consts = F16_STRATEGY_LIST_SIZES)]
146-
fn take_fsl_f16_force_piecewise_sequential<const LIST_SIZE: usize>(
146+
fn take_fsl_f16_force_piecewise_sequence<const LIST_SIZE: usize>(
147147
bencher: Bencher,
148148
num_indices: usize,
149149
) {
@@ -154,7 +154,7 @@ fn take_fsl_f16_force_piecewise_sequential<const LIST_SIZE: usize>(
154154
.counter(BytesCount::of_many::<f16>(num_indices * LIST_SIZE))
155155
.with_inputs(|| (&fsl, &indices, SESSION.create_execution_ctx()))
156156
.bench_refs(|(array, indices, execution_ctx)| {
157-
take_fsl_f16_piecewise_sequential_strategy::<LIST_SIZE>(array, indices)
157+
take_fsl_f16_piecewise_sequence_strategy::<LIST_SIZE>(array, indices)
158158
.into_array()
159159
.execute::<RecursiveCanonical>(execution_ctx)
160160
.unwrap()
@@ -211,7 +211,7 @@ fn take_fsl_f16_per_index_strategy<const LIST_SIZE: usize, E: IntegerPType>(
211211
}
212212
}
213213

214-
fn take_fsl_f16_piecewise_sequential_strategy<const LIST_SIZE: usize>(
214+
fn take_fsl_f16_piecewise_sequence_strategy<const LIST_SIZE: usize>(
215215
array: &FixedSizeListArray,
216216
indices: &Buffer<u64>,
217217
) -> FixedSizeListArray {
@@ -223,11 +223,17 @@ fn take_fsl_f16_piecewise_sequential_strategy<const LIST_SIZE: usize>(
223223
let run_count = starts.len();
224224
let starts = PrimitiveArray::from_iter(starts).into_array();
225225
let lengths = ConstantArray::new(LIST_SIZE as u64, run_count).into_array();
226+
let multipliers = ConstantArray::new(1u64, run_count).into_array();
226227

227-
// SAFETY: benchmark indices are generated in-bounds, lengths is a non-nullable unsigned
228-
// constant, and output length is exactly `indices.len() * LIST_SIZE`.
228+
// SAFETY: benchmark indices are generated in-bounds; lengths and multiplier 1 are
229+
// non-nullable unsigned constants; output length is exactly `indices.len() * LIST_SIZE`.
229230
let element_indices = unsafe {
230-
PiecewiseSequentialArray::new_unchecked(starts, lengths, indices.len() * LIST_SIZE)
231+
PiecewiseSequenceArray::new_unchecked(
232+
starts,
233+
lengths,
234+
multipliers,
235+
indices.len() * LIST_SIZE,
236+
)
231237
}
232238
.into_array();
233239
let elements = array.elements().take(element_indices).unwrap();

vortex-array/src/arrays/decimal/compute/take.rs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,12 +75,10 @@ fn take_piecewise_sequence(
7575
let validity = array.validity()?.take(indices_ref)?;
7676

7777
// SAFETY: contiguous gather preserves the decimal dtype and value representation.
78-
Ok(
79-
Some(unsafe {
80-
DecimalArray::new_unchecked(values, array.decimal_dtype(), validity)
81-
}
82-
.into_array()),
83-
)
78+
Ok(Some(
79+
unsafe { DecimalArray::new_unchecked(values, array.decimal_dtype(), validity) }
80+
.into_array(),
81+
))
8482
})
8583
})
8684
})

vortex-array/src/arrays/piecewise_sequence/mod.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -163,16 +163,16 @@ where
163163
for (&start, &length) in starts.iter().zip_eq(lengths) {
164164
let start: usize = start.as_();
165165
let length: usize = length.as_();
166-
let end = start.checked_add(length).ok_or_else(|| {
167-
vortex_err!("PiecewiseSequenceArray range overflows usize")
168-
})?;
166+
let end = start
167+
.checked_add(length)
168+
.ok_or_else(|| vortex_err!("PiecewiseSequenceArray range overflows usize"))?;
169169
vortex_ensure!(
170170
end <= source_len,
171171
"PiecewiseSequenceArray range {start}..{end} exceeds source length {source_len}"
172172
);
173-
computed_len = computed_len.checked_add(length).ok_or_else(|| {
174-
vortex_err!("PiecewiseSequenceArray output length overflows usize")
175-
})?;
173+
computed_len = computed_len
174+
.checked_add(length)
175+
.ok_or_else(|| vortex_err!("PiecewiseSequenceArray output length overflows usize"))?;
176176
}
177177

178178
vortex_ensure!(

vortex-array/src/arrays/piecewise_sequence/tests.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,26 @@ fn primitive_take_consumes_piecewise_indices() -> VortexResult<()> {
187187
Ok(())
188188
}
189189

190+
#[test]
191+
fn primitive_take_handles_non_unit_multiplier() -> VortexResult<()> {
192+
let values = PrimitiveArray::from_iter(0i32..20).into_array();
193+
let indices = PiecewiseSequenceArray::try_new(
194+
buffer![3u64].into_array(),
195+
buffer![3u64].into_array(),
196+
buffer![2u64].into_array(),
197+
3,
198+
)?
199+
.into_array();
200+
let taken = values.take(indices)?;
201+
202+
assert_arrays_eq!(
203+
taken,
204+
PrimitiveArray::from_iter([3i32, 5, 7]).into_array(),
205+
&mut array_session().create_execution_ctx()
206+
);
207+
Ok(())
208+
}
209+
190210
#[test]
191211
fn bool_take_consumes_piecewise_indices() -> VortexResult<()> {
192212
let values = BoolArray::from_iter([true, false, true, true, false, false]).into_array();

vortex-array/src/arrays/varbinview/compute/take.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,15 @@ fn take_piecewise_sequence(
9393
// SAFETY: ranges were validated against the source views, and copied views still reference the
9494
// same backing data buffers.
9595
unsafe {
96-
Ok(Some(VarBinViewArray::new_handle_unchecked(
97-
BufferHandle::new_host(views.into_byte_buffer()),
98-
Arc::clone(array.data_buffers()),
99-
array.dtype().clone(),
100-
validity,
101-
)
102-
.into_array()))
96+
Ok(Some(
97+
VarBinViewArray::new_handle_unchecked(
98+
BufferHandle::new_host(views.into_byte_buffer()),
99+
Arc::clone(array.data_buffers()),
100+
array.dtype().clone(),
101+
validity,
102+
)
103+
.into_array(),
104+
))
103105
}
104106
}
105107

0 commit comments

Comments
 (0)