Skip to content

Commit 8f9ca31

Browse files
committed
fix
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
1 parent 4dfb89b commit 8f9ca31

10 files changed

Lines changed: 141 additions & 74 deletions

File tree

encodings/alp/public-api.lock

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ pub fn vortex_alp::ALPArray::into_parts(self) -> (vortex_array::array::ArrayRef,
118118

119119
pub fn vortex_alp::ALPArray::new(encoded: vortex_array::array::ArrayRef, exponents: vortex_alp::Exponents, patches: core::option::Option<vortex_array::patches::Patches>) -> Self
120120

121-
pub fn vortex_alp::ALPArray::patches(&self) -> core::option::Option<&vortex_array::patches::Patches>
121+
pub fn vortex_alp::ALPArray::patches(&self) -> core::option::Option<vortex_array::patches::Patches>
122122

123123
pub fn vortex_alp::ALPArray::ptype(&self) -> vortex_array::dtype::ptype::PType
124124

encodings/alp/src/alp/array.rs

Lines changed: 95 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ use vortex_error::VortexExpect;
3434
use vortex_error::VortexResult;
3535
use vortex_error::vortex_bail;
3636
use vortex_error::vortex_ensure;
37+
use vortex_error::vortex_err;
3738
use vortex_error::vortex_panic;
3839
use vortex_session::VortexSession;
3940

@@ -76,14 +77,14 @@ impl VTable for ALP {
7677
array.dtype.hash(state);
7778
array.encoded().array_hash(state, precision);
7879
array.exponents.hash(state);
79-
array.patches.array_hash(state, precision);
80+
array.patches().array_hash(state, precision);
8081
}
8182

8283
fn array_eq(array: &ALPArray, other: &ALPArray, precision: Precision) -> bool {
8384
array.dtype == other.dtype
8485
&& array.encoded().array_eq(other.encoded(), precision)
8586
&& array.exponents == other.exponents
86-
&& array.patches.array_eq(&other.patches, precision)
87+
&& array.patches().array_eq(&other.patches(), precision)
8788
}
8889

8990
fn nbuffers(_array: &ALPArray) -> usize {
@@ -107,30 +108,9 @@ impl VTable for ALP {
107108
}
108109

109110
fn with_slots(array: &mut ALPArray, slots: Vec<Option<ArrayRef>>) -> VortexResult<()> {
110-
vortex_ensure!(
111-
slots.len() == NUM_SLOTS,
112-
"ALPArray expects {} slots, got {}",
113-
NUM_SLOTS,
114-
slots.len()
115-
);
116-
117-
// Reconstruct patches from slots + existing metadata
118-
array.patches = match (&slots[PATCH_INDICES_SLOT], &slots[PATCH_VALUES_SLOT]) {
119-
(Some(indices), Some(values)) => {
120-
let old = array
121-
.patches
122-
.as_ref()
123-
.vortex_expect("ALPArray had patch slots but no patches metadata");
124-
Some(Patches::new(
125-
old.array_len(),
126-
old.offset(),
127-
indices.clone(),
128-
values.clone(),
129-
slots[PATCH_CHUNK_OFFSETS_SLOT].clone(),
130-
)?)
131-
}
132-
_ => None,
133-
};
111+
let slots: [Option<ArrayRef>; NUM_SLOTS] = slots.try_into().map_err(|v: Vec<_>| {
112+
vortex_err!("ALPArray expects {} slots, got {}", NUM_SLOTS, v.len())
113+
})?;
134114
array.slots = slots;
135115
Ok(())
136116
}
@@ -142,6 +122,7 @@ impl VTable for ALP {
142122
exp_f: exponents.f as u32,
143123
patches: array
144124
.patches()
125+
.as_ref()
145126
.map(|p| p.to_metadata(array.len(), array.dtype()))
146127
.transpose()?,
147128
}))
@@ -239,8 +220,9 @@ pub(super) const SLOT_NAMES: [&str; NUM_SLOTS] = [
239220

240221
#[derive(Clone, Debug)]
241222
pub struct ALPArray {
242-
slots: Vec<Option<ArrayRef>>,
243-
patches: Option<Patches>,
223+
slots: [Option<ArrayRef>; NUM_SLOTS],
224+
patches_offset: usize,
225+
patches_offset_within_chunk: Option<usize>,
244226
dtype: DType,
245227
exponents: Exponents,
246228
stats_set: ArrayStats,
@@ -408,13 +390,15 @@ impl ALPArray {
408390
_ => unreachable!(),
409391
};
410392

411-
let slots = Self::make_slots(&encoded, &patches);
393+
let (slots, patches_offset, patches_offset_within_chunk) =
394+
Self::decompose(encoded, patches);
412395

413396
Ok(Self {
414397
dtype,
415398
slots,
399+
patches_offset,
400+
patches_offset_within_chunk,
416401
exponents,
417-
patches,
418402
stats_set: Default::default(),
419403
})
420404
}
@@ -429,32 +413,37 @@ impl ALPArray {
429413
patches: Option<Patches>,
430414
dtype: DType,
431415
) -> Self {
432-
let slots = Self::make_slots(&encoded, &patches);
416+
let (slots, patches_offset, patches_offset_within_chunk) =
417+
Self::decompose(encoded, patches);
433418

434419
Self {
435420
dtype,
436421
slots,
422+
patches_offset,
423+
patches_offset_within_chunk,
437424
exponents,
438-
patches,
439425
stats_set: Default::default(),
440426
}
441427
}
442428

443-
fn make_slots(encoded: &ArrayRef, patches: &Option<Patches>) -> Vec<Option<ArrayRef>> {
444-
let (patch_indices, patch_values, patch_chunk_offsets) = match patches {
445-
Some(p) => (
446-
Some(p.indices().clone()),
447-
Some(p.values().clone()),
448-
p.chunk_offsets().clone(),
449-
),
450-
None => (None, None, None),
451-
};
452-
vec![
453-
Some(encoded.clone()),
454-
patch_indices,
455-
patch_values,
456-
patch_chunk_offsets,
457-
]
429+
/// Decompose encoded + patches into slots and patch metadata, moving ArrayRefs
430+
/// without cloning.
431+
fn decompose(
432+
encoded: ArrayRef,
433+
patches: Option<Patches>,
434+
) -> ([Option<ArrayRef>; NUM_SLOTS], usize, Option<usize>) {
435+
match patches {
436+
Some(p) => {
437+
let (_array_len, offset, indices, values, chunk_offsets, offset_within_chunk) =
438+
p.into_parts();
439+
let slots = [Some(encoded), Some(indices), Some(values), chunk_offsets];
440+
(slots, offset, offset_within_chunk)
441+
}
442+
None => {
443+
let slots = [Some(encoded), None, None, None];
444+
(slots, 0, None)
445+
}
446+
}
458447
}
459448

460449
pub fn ptype(&self) -> PType {
@@ -472,17 +461,69 @@ impl ALPArray {
472461
self.exponents
473462
}
474463

475-
pub fn patches(&self) -> Option<&Patches> {
476-
self.patches.as_ref()
464+
/// Reconstruct patches from slots on demand.
465+
pub fn patches(&self) -> Option<Patches> {
466+
let indices = self.slots[PATCH_INDICES_SLOT].as_ref()?;
467+
let values = self.slots[PATCH_VALUES_SLOT]
468+
.as_ref()
469+
.vortex_expect("ALPArray has patch indices but no values");
470+
// SAFETY: the patches were validated at construction time.
471+
Some(unsafe {
472+
Patches::new_unchecked(
473+
self.encoded().len(),
474+
self.patches_offset,
475+
indices.clone(),
476+
values.clone(),
477+
self.slots[PATCH_CHUNK_OFFSETS_SLOT].clone(),
478+
self.patches_offset_within_chunk,
479+
)
480+
})
477481
}
478482

479483
/// Consumes the array and returns its parts.
480484
#[inline]
481-
pub fn into_parts(mut self) -> (ArrayRef, Exponents, Option<Patches>, DType) {
482-
let encoded = self.slots[ENCODED_SLOT]
483-
.take()
484-
.vortex_expect("ALPArray encoded slot");
485-
(encoded, self.exponents, self.patches, self.dtype)
485+
pub fn into_parts(self) -> (ArrayRef, Exponents, Option<Patches>, DType) {
486+
let (encoded, exponents, patches_offset, patches_offset_within_chunk, slots, dtype) =
487+
self.into_raw_parts();
488+
let patches = slots[PATCH_INDICES_SLOT].as_ref().map(|_| {
489+
let [_, indices, values, chunk_offsets] = slots;
490+
// SAFETY: validated at construction time.
491+
unsafe {
492+
Patches::new_unchecked(
493+
encoded.len(),
494+
patches_offset,
495+
indices.vortex_expect("ALPArray has patch indices"),
496+
values.vortex_expect("ALPArray has patch values"),
497+
chunk_offsets,
498+
patches_offset_within_chunk,
499+
)
500+
}
501+
});
502+
(encoded, exponents, patches, dtype)
503+
}
504+
505+
/// Consumes the array and returns raw slot components without reconstructing Patches.
506+
#[inline]
507+
pub(crate) fn into_raw_parts(
508+
self,
509+
) -> (
510+
ArrayRef,
511+
Exponents,
512+
usize,
513+
Option<usize>,
514+
[Option<ArrayRef>; NUM_SLOTS],
515+
DType,
516+
) {
517+
let [encoded, indices, values, chunk_offsets] = self.slots;
518+
let encoded = encoded.vortex_expect("ALPArray encoded slot");
519+
(
520+
encoded,
521+
self.exponents,
522+
self.patches_offset,
523+
self.patches_offset_within_chunk,
524+
[None, indices, values, chunk_offsets],
525+
self.dtype,
526+
)
486527
}
487528
}
488529

encodings/alp/src/alp/compute/cast.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ impl CastReduce for ALP {
2929
.patches()
3030
.map(|p| {
3131
if p.values().dtype() == dtype {
32-
Ok(p.clone())
32+
Ok(p)
3333
} else {
3434
Patches::new(
3535
p.array_len(),

encodings/fastlanes/src/bitpacking/array/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ pub struct BitPackedArrayParts {
4747

4848
#[derive(Clone, Debug)]
4949
pub struct BitPackedArray {
50-
pub(super) slots: Vec<Option<ArrayRef>>,
50+
pub(super) slots: [Option<ArrayRef>; NUM_SLOTS],
5151
/// The offset within the first block (created with a slice).
5252
/// 0 <= offset < 1024
5353
pub(super) offset: u16,
@@ -109,7 +109,7 @@ impl BitPackedArray {
109109
patches: &Option<Patches>,
110110
validity: &Validity,
111111
len: usize,
112-
) -> Vec<Option<ArrayRef>> {
112+
) -> [Option<ArrayRef>; NUM_SLOTS] {
113113
let (pi, pv, pco) = match patches {
114114
Some(p) => (
115115
Some(p.indices().clone()),
@@ -119,7 +119,7 @@ impl BitPackedArray {
119119
None => (None, None, None),
120120
};
121121
let validity_slot = validity_to_child(validity, len);
122-
vec![pi, pv, pco, validity_slot]
122+
[pi, pv, pco, validity_slot]
123123
}
124124

125125
/// A safe constructor for a `BitPackedArray` from its components:

encodings/fastlanes/src/bitpacking/vtable/mod.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,13 @@ impl VTable for BitPacked {
152152
}
153153

154154
fn with_slots(array: &mut BitPackedArray, slots: Vec<Option<ArrayRef>>) -> VortexResult<()> {
155-
vortex_ensure!(
156-
slots.len() == NUM_SLOTS,
157-
"BitPackedArray expects {} slots, got {}",
158-
NUM_SLOTS,
159-
slots.len()
160-
);
155+
let slots: [Option<ArrayRef>; NUM_SLOTS] = slots.try_into().map_err(|v: Vec<_>| {
156+
vortex_err!(
157+
"BitPackedArray expects {} slots, got {}",
158+
NUM_SLOTS,
159+
v.len()
160+
)
161+
})?;
161162

162163
// Reconstruct patches from slots + existing metadata
163164
array.patches = match (&slots[PATCH_INDICES_SLOT], &slots[PATCH_VALUES_SLOT]) {

vortex-array/public-api.lock

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15204,6 +15204,8 @@ pub fn vortex_array::patches::Patches::indices_ptype(&self) -> vortex_error::Vor
1520415204

1520515205
pub fn vortex_array::patches::Patches::into_indices(self) -> vortex_array::ArrayRef
1520615206

15207+
pub fn vortex_array::patches::Patches::into_parts(self) -> (usize, usize, vortex_array::ArrayRef, vortex_array::ArrayRef, core::option::Option<vortex_array::ArrayRef>, core::option::Option<usize>)
15208+
1520715209
pub fn vortex_array::patches::Patches::into_values(self) -> vortex_array::ArrayRef
1520815210

1520915211
pub fn vortex_array::patches::Patches::map_values<F>(self, f: F) -> vortex_error::VortexResult<Self> where F: core::ops::function::FnOnce(vortex_array::ArrayRef) -> vortex_error::VortexResult<vortex_array::ArrayRef>

vortex-array/src/arrays/dict/array.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ pub(super) const SLOT_NAMES: [&str; NUM_SLOTS] = ["codes", "values"];
3838

3939
#[derive(Debug, Clone)]
4040
pub struct DictArray {
41-
pub(super) slots: Vec<Option<ArrayRef>>,
41+
pub(super) slots: [Option<ArrayRef>; NUM_SLOTS],
4242
pub(super) stats_set: ArrayStats,
4343
pub(super) dtype: DType,
4444
/// Indicates whether all dictionary values are definitely referenced by at least one code.
@@ -67,7 +67,7 @@ impl DictArray {
6767
.dtype()
6868
.union_nullability(codes.dtype().nullability());
6969
Self {
70-
slots: vec![Some(codes), Some(values)],
70+
slots: [Some(codes), Some(values)],
7171
stats_set: Default::default(),
7272
dtype,
7373
all_values_referenced: false,

vortex-array/src/arrays/dict/vtable/mod.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -179,12 +179,13 @@ impl VTable for Dict {
179179
}
180180

181181
fn with_slots(array: &mut DictArray, slots: Vec<Option<ArrayRef>>) -> VortexResult<()> {
182-
vortex_ensure!(
183-
slots.len() == NUM_SLOTS,
184-
"DictArray expects exactly {} slots, got {}",
185-
NUM_SLOTS,
186-
slots.len()
187-
);
182+
let slots: [Option<ArrayRef>; NUM_SLOTS] = slots.try_into().map_err(|v: Vec<_>| {
183+
vortex_err!(
184+
"DictArray expects exactly {} slots, got {}",
185+
NUM_SLOTS,
186+
v.len()
187+
)
188+
})?;
188189
array.slots = slots;
189190
Ok(())
190191
}

vortex-array/src/patches.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,28 @@ impl Patches {
236236
}
237237
}
238238

239+
/// Consume the patches and return all components.
240+
#[inline]
241+
pub fn into_parts(
242+
self,
243+
) -> (
244+
usize,
245+
usize,
246+
ArrayRef,
247+
ArrayRef,
248+
Option<ArrayRef>,
249+
Option<usize>,
250+
) {
251+
(
252+
self.array_len,
253+
self.offset,
254+
self.indices,
255+
self.values,
256+
self.chunk_offsets,
257+
self.offset_within_chunk,
258+
)
259+
}
260+
239261
#[inline]
240262
pub fn array_len(&self) -> usize {
241263
self.array_len

vortex-btrblocks/src/compressor/float/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ impl Scheme for ALPScheme {
341341
Excludes::int_only(&int_excludes),
342342
)?;
343343

344-
let patches = alp.patches().map(compress_patches).transpose()?;
344+
let patches = alp.patches().as_ref().map(compress_patches).transpose()?;
345345

346346
Ok(ALPArray::new(compressed_alp_ints, alp.exponents(), patches).into_array())
347347
}

0 commit comments

Comments
 (0)