Skip to content

Commit c6d6bad

Browse files
connortsui20connortsui20
authored
Resolve more small TODO(connor) items (#10985)
1 parent 7e70afc commit c6d6bad

7 files changed

Lines changed: 44 additions & 25 deletions

File tree

‎vortex-array/src/arrays/list/array.rs‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ impl ListData {
280280
// TODO(connor)[ListView]: Create 2 functions `reset_offsets` and `recursive_reset_offsets`,
281281
// where `reset_offsets` is infallible.
282282
// Also, `reset_offsets` can be made more efficient by replacing `sub_scalar` with a match on
283-
// the offset type and manual subtraction and fast path where `offsets[0] == 0`.
283+
// the offset type and manual subtraction.
284284
}
285285

286286
pub trait ListArrayExt: ListArraySlotsExt {
@@ -351,10 +351,14 @@ pub trait ListArrayExt: ListArraySlotsExt {
351351

352352
let offsets = self.offsets();
353353
let first_offset = offsets.execute_scalar(0, ctx)?;
354-
let adjusted_offsets = offsets.clone().binary(
355-
ConstantArray::new(first_offset, offsets.len()).into_array(),
356-
Operator::Sub,
357-
)?;
354+
let adjusted_offsets = if first_offset.is_zero() == Some(true) {
355+
offsets.clone()
356+
} else {
357+
offsets.clone().binary(
358+
ConstantArray::new(first_offset, offsets.len()).into_array(),
359+
Operator::Sub,
360+
)?
361+
};
358362

359363
// SAFETY: By resetting the offsets we simply "shift" everything left and discard trailing garbage, so all invariants remain the same.
360364
Ok(unsafe { ListArray::new_unchecked(elements, adjusted_offsets, self.list_validity()) })

‎vortex-array/src/arrays/list/tests.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use crate::IntoArray;
1919
use crate::VortexSessionExecute;
2020
use crate::arrays::FilterArray;
2121
use crate::arrays::List;
22+
use crate::arrays::Primitive;
2223
use crate::arrays::PrimitiveArray;
2324
use crate::assert_arrays_eq;
2425
use crate::builders::ArrayBuilder;
@@ -1035,6 +1036,24 @@ fn test_recursive_compact_list_of_lists() {
10351036
assert_arrays_eq!(recursive_array, sliced, &mut ctx);
10361037
}
10371038

1039+
#[test]
1040+
fn test_reset_offsets_zero_first_offset_keeps_offsets() -> VortexResult<()> {
1041+
let mut ctx = SESSION.create_execution_ctx();
1042+
let list = ListArray::try_new(
1043+
buffer![1i32, 2, 3, 4].into_array(),
1044+
buffer![0u32, 1, 4].into_array(),
1045+
Validity::NonNullable,
1046+
&mut ctx,
1047+
)?;
1048+
1049+
let reset = list.reset_offsets(false, &mut ctx)?;
1050+
1051+
// The offsets already start at zero, so they are reused instead of wrapped in a subtraction.
1052+
assert!(reset.offsets().is::<Primitive>());
1053+
assert_arrays_eq!(reset.into_array(), list.into_array(), &mut ctx);
1054+
Ok(())
1055+
}
1056+
10381057
#[test]
10391058
fn test_filter_sliced_list_array() -> VortexResult<()> {
10401059
let list = ListArray::try_new(

‎vortex-array/src/arrays/listview/conversion.rs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ pub fn list_view_from_list(list: ListArray, ctx: &mut ExecutionCtx) -> VortexRes
4545
// garbage data when they turn it back into a `ListArray`.
4646
let list = list.reset_offsets(false, ctx)?;
4747

48-
// `reset_offsets` leaves a lazy subtraction in the offsets slot, which both the `sizes` below
48+
// `reset_offsets` can leave a lazy subtraction in the offsets slot, which both the `sizes` below
4949
// and the view's own offsets read. Execute it once here so that it does not run per reader.
5050
let list_offsets = list.offsets().clone().execute::<PrimitiveArray>(ctx)?;
5151

@@ -104,14 +104,13 @@ fn build_sizes_from_offsets<O: IntegerPType>(
104104
Ok(sizes_builder.finish_into_primitive().into_array())
105105
}
106106

107-
// TODO(connor)[ListView]: Note that it is not exactly zero-copy because we have to add a single
108-
// offset at the end, but it is fast enough.
109107
/// Creates a `ListArray` from a `ListViewArray`.
110108
///
111109
/// The resulting `ListArray` will not have any leading or trailing garbage data.
112110
///
113-
/// This operation is fast when `ListViewArray::is_zero_copy_to_list` is `true`. Otherwise it
114-
/// falls back to the (very) expensive path and rebuilds the `ListArray` from scratch.
111+
/// This operation is fast when `ListViewArray::is_zero_copy_to_list` is `true`. It is not exactly
112+
/// zero-copy, since it must append one final offset. Otherwise it falls back to the (very)
113+
/// expensive path and rebuilds the `ListArray` from scratch.
115114
pub fn list_from_list_view(
116115
list_view: ListViewArray,
117116
ctx: &mut ExecutionCtx,

‎vortex-array/src/builders/fixed_size_list.rs‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ use std::any::Any;
55
use std::sync::Arc;
66

77
use vortex_buffer::BufferAllocatorRef;
8-
use vortex_error::VortexExpect;
98
use vortex_error::VortexResult;
109
use vortex_error::vortex_bail;
1110
use vortex_error::vortex_ensure_eq;
@@ -226,14 +225,16 @@ impl FixedSizeListBuilder {
226225
"elements length must be equal to the array length times the list size"
227226
);
228227

229-
// TODO(connor): Use `new_unchecked` here.
230-
FixedSizeListArray::try_new(
231-
self.elements_builder.finish(),
232-
self.list_size(),
233-
self.nulls.finish_with_nullability(self.dtype.nullability()),
234-
final_len,
235-
)
236-
.vortex_expect("tried to create an invalid `FixedSizeListArray` from a builder")
228+
// SAFETY: The assertion above checks that the elements length is `final_len * list_size`,
229+
// and the validity is built from `self.nulls`, whose length is `final_len`.
230+
unsafe {
231+
FixedSizeListArray::new_unchecked(
232+
self.elements_builder.finish(),
233+
self.list_size(),
234+
self.nulls.finish_with_nullability(self.dtype.nullability()),
235+
final_len,
236+
)
237+
}
237238
}
238239

239240
/// The [`DType`] of the inner elements. Note that this is **not** the same as the [`DType`] of

‎vortex-bench/src/vector_dataset/layout.rs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,6 @@ pub enum VectorMetric {
2828
Cosine,
2929
/// `sum((a - b)^2)`.
3030
L2,
31-
// TODO(connor): Do we even need this?
32-
/// `dot(a, b)`.
33-
InnerProduct,
3431
}
3532

3633
/// A specific train layout published for a dataset, plus the shard count when partitioned

‎vortex-duckdb/src/exporter/fixed_size_list.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ impl ColumnExporter for FixedSizeListExporter {
8080
self.elements_exporter
8181
.export(offset * list_size, len * list_size, elements_vector, ctx)?;
8282

83-
// TODO(connor): We must flatten the child vector to ensure any child dictionary views
84-
// (namely UTF-8 string views in dictionaries) are materialized.
83+
// We must flatten the child vector to ensure any child dictionary views (namely UTF-8
84+
// string views in dictionaries) are materialized.
8585
// See https://github.com/vortex-data/vortex/pull/4610#issuecomment-3286676825 for a
8686
// detailed explanation on why we need this for now.
8787
elements_vector.flatten();

‎vortex-ffi/src/dtype.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@ pub enum vx_dtype_variant {
6161
DTYPE_FIXED_SIZE_LIST = 9,
6262
}
6363

64-
// TODO(connor)[Union]
6564
impl From<&DType> for vx_dtype_variant {
6665
fn from(value: &DType) -> Self {
6766
match value {

0 commit comments

Comments
 (0)