Skip to content

Commit d5164fc

Browse files
connortsui20connortsui20
committed
address comments and fix bugs
Signed-off-by: Connor Tsui <[email protected]>
1 parent 7b715e6 commit d5164fc

File tree

10 files changed

+238
-159
lines changed

10 files changed

+238
-159
lines changed

‎vortex-array/src/arrays/bool/vtable/operator.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ impl OperatorVTable<BoolVTable> for BoolVTable {
2727
// Note that validity already has the mask applied so we only need to apply it to bits.
2828
let bits = bits.filter(&mask);
2929

30-
Ok(BoolVector::new(bits, validity).into())
30+
Ok(BoolVector::try_new(bits, validity)?.into())
3131
}))
3232
}
3333
}

‎vortex-array/src/arrays/primitive/vtable/operator.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ impl OperatorVTable<PrimitiveVTable> for PrimitiveVTable {
3030
// the elements.
3131
let elements = elements.filter(&mask);
3232

33-
Ok(PVector::new(elements, validity).into())
33+
Ok(PVector::try_new(elements, validity)?.into())
3434
}))
3535
})
3636
}

‎vortex-compute/src/filter/bool.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,14 @@ use crate::filter::Filter;
88

99
impl Filter for BoolVector {
1010
fn filter(&self, mask: &Mask) -> Self {
11-
Self::new(self.bits().filter(mask), self.validity().filter(mask))
11+
let filtered_bits = self.bits().filter(mask);
12+
let filtered_validity = self.validity().filter(mask);
13+
14+
debug_assert_eq!(filtered_bits.len(), filtered_validity.len());
15+
16+
// SAFETY: We filter the bits and validity with the same mask, and since they came from an
17+
// existing and valid `BoolVector`, we know that the filtered output must have the same
18+
// length.
19+
unsafe { Self::new_unchecked(filtered_bits, filtered_validity) }
1220
}
1321
}

‎vortex-vector/src/bool/vector.rs‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,7 @@ impl BoolVector {
3030
///
3131
/// Panics if the length of the validity mask does not match the length of the bits.
3232
pub fn new(bits: BitBuffer, validity: Mask) -> Self {
33-
Self::try_new(bits, validity)
34-
.vortex_expect("`BoolVector` validity mask must have the same length as bits")
33+
Self::try_new(bits, validity).vortex_expect("Failed to create `BoolVector`")
3534
}
3635

3736
/// Tries to create a new [`BoolVector`] from the given bits and validity mask.
@@ -53,14 +52,12 @@ impl BoolVector {
5352
/// # Safety
5453
///
5554
/// The caller must ensure that the validity mask has the same length as the bits.
56-
pub fn new_unchecked(bits: BitBuffer, validity: Mask) -> Self {
57-
debug_assert_eq!(
58-
validity.len(),
59-
bits.len(),
60-
"`BoolVector` validity mask must have the same length as bits"
61-
);
62-
63-
Self { bits, validity }
55+
pub unsafe fn new_unchecked(bits: BitBuffer, validity: Mask) -> Self {
56+
if cfg!(debug_assertions) {
57+
Self::new(bits, validity)
58+
} else {
59+
Self { bits, validity }
60+
}
6461
}
6562

6663
/// Decomposes the boolean vector into its constituent parts (bit buffer and validity).

‎vortex-vector/src/bool/vector_mut.rs‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,7 @@ impl BoolVectorMut {
7777
///
7878
/// Panics if the length of the validity mask does not match the length of the bits.
7979
pub fn new(bits: BitBufferMut, validity: MaskMut) -> Self {
80-
Self::try_new(bits, validity)
81-
.vortex_expect("`BoolVector` validity mask must have the same length as bits")
80+
Self::try_new(bits, validity).vortex_expect("Failed to create `BoolVectorMut`")
8281
}
8382

8483
/// Tries to create a new [`BoolVectorMut`] from the given bits and validity mask.
@@ -104,13 +103,11 @@ impl BoolVectorMut {
104103
/// Ideally, they are taken from `into_parts`, mutated in a way that doesn't re-allocate, and
105104
/// then passed back to this function.
106105
pub unsafe fn new_unchecked(bits: BitBufferMut, validity: MaskMut) -> Self {
107-
debug_assert_eq!(
108-
bits.len(),
109-
validity.len(),
110-
"`BoolVector` validity mask must have the same length as bits"
111-
);
112-
113-
Self { bits, validity }
106+
if cfg!(debug_assertions) {
107+
Self::new(bits, validity)
108+
} else {
109+
Self { bits, validity }
110+
}
114111
}
115112

116113
/// Creates a new mutable boolean vector with the given `capacity`.

‎vortex-vector/src/primitive/generic.rs‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ impl<T: NativePType> PVector<T> {
3434
///
3535
/// Panics if the length of the validity mask does not match the length of the elements buffer.
3636
pub fn new(elements: Buffer<T>, validity: Mask) -> Self {
37-
Self::try_new(elements, validity)
38-
.vortex_expect("`PVector` validity mask must have the same length as elements")
37+
Self::try_new(elements, validity).vortex_expect("Failed to create `PVector`")
3938
}
4039

4140
/// Tries to create a new [`PVector<T>`] from the given elements buffer and validity mask.
@@ -59,14 +58,12 @@ impl<T: NativePType> PVector<T> {
5958
/// # Safety
6059
///
6160
/// The caller must ensure that the validity mask has the same length as the elements buffer.
62-
pub fn new_unchecked(elements: Buffer<T>, validity: Mask) -> Self {
63-
debug_assert_eq!(
64-
validity.len(),
65-
elements.len(),
66-
"`PVector` validity mask must have the same length as elements"
67-
);
68-
69-
Self { elements, validity }
61+
pub unsafe fn new_unchecked(elements: Buffer<T>, validity: Mask) -> Self {
62+
if cfg!(debug_assertions) {
63+
Self::new(elements, validity)
64+
} else {
65+
Self { elements, validity }
66+
}
7067
}
7168

7269
/// Decomposes the primitive vector into its constituent parts (buffer and validity).

‎vortex-vector/src/primitive/generic_mut.rs‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,7 @@ impl<T> PVectorMut<T> {
114114
///
115115
/// Panics if the length of the validity mask does not match the length of the elements buffer.
116116
pub fn new(elements: BufferMut<T>, validity: MaskMut) -> Self {
117-
Self::try_new(elements, validity)
118-
.vortex_expect("`PVectorMut` validity mask must have the same length as elements")
117+
Self::try_new(elements, validity).vortex_expect("Failed to create `PVectorMut`")
119118
}
120119

121120
/// Tries to create a new [`PVectorMut<T>`] from the given elements buffer and validity mask.
@@ -143,13 +142,11 @@ impl<T> PVectorMut<T> {
143142
/// Ideally, they are taken from `into_parts`, mutated in a way that doesn't re-allocate, and
144143
/// then passed back to this function.
145144
pub unsafe fn new_unchecked(elements: BufferMut<T>, validity: MaskMut) -> Self {
146-
debug_assert_eq!(
147-
elements.len(),
148-
validity.len(),
149-
"`PVectorMut` validity mask must have the same length as elements"
150-
);
151-
152-
Self { elements, validity }
145+
if cfg!(debug_assertions) {
146+
Self::new(elements, validity)
147+
} else {
148+
Self { elements, validity }
149+
}
153150
}
154151

155152
/// Create a new mutable primitive vector with the given capacity.

‎vortex-vector/src/struct_/vector.rs‎

Lines changed: 28 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use vortex_mask::Mask;
88

99
use crate::{StructVectorMut, Vector, VectorMutOps, VectorOps};
1010

11-
/// An immutable vector of boolean values.
11+
/// An immutable vector of struct values.
1212
///
1313
/// `StructVector` can be considered a borrowed / frozen version of [`StructVectorMut`], which is
1414
/// created via the [`freeze`](crate::VectorMutOps::freeze) method.
@@ -39,9 +39,7 @@ impl StructVector {
3939
/// - Any field vector has a length that does not match the length of other fields.
4040
/// - The validity mask length does not match the field length.
4141
pub fn new(fields: Box<[Vector]>, validity: Mask) -> Self {
42-
Self::try_new(fields, validity).vortex_expect(
43-
"`StructVector` fields must have matching length and validity constraints",
44-
)
42+
Self::try_new(fields, validity).vortex_expect("Failed to create `StructVector`")
4543
}
4644

4745
/// Tries to create a new [`StructVector`] from the given fields and validity mask.
@@ -59,7 +57,24 @@ impl StructVector {
5957
fields[0].len()
6058
};
6159

62-
Self::validate(&fields, len, &validity)?;
60+
// Validate that the validity mask has the correct length.
61+
vortex_ensure!(
62+
validity.len() == len,
63+
"Validity mask length ({}) does not match expected length ({})",
64+
validity.len(),
65+
len
66+
);
67+
68+
// Validate that all fields have the correct length.
69+
for (i, field) in fields.iter().enumerate() {
70+
vortex_ensure!(
71+
field.len() == len,
72+
"Field {} has length {} but expected length {}",
73+
i,
74+
field.len(),
75+
len
76+
);
77+
}
6378

6479
Ok(Self {
6580
fields,
@@ -83,47 +98,15 @@ impl StructVector {
8398
fields[0].len()
8499
};
85100

86-
debug_assert!(
87-
Self::validate(&fields, len, &validity).is_ok(),
88-
"`StructVector` fields must have matching length and validity constraints"
89-
);
90-
91-
Self {
92-
fields,
93-
validity,
94-
len,
95-
}
96-
}
97-
98-
/// Validates the fields and validity mask for a [`StructVector`].
99-
///
100-
/// # Errors
101-
///
102-
/// Returns an error if:
103-
///
104-
/// - Any field vector has a length that does not match the length of other fields.
105-
/// - The validity mask length does not match the field length.
106-
fn validate(fields: &[Vector], len: usize, validity: &Mask) -> VortexResult<()> {
107-
// Validate that the validity mask has the correct length.
108-
vortex_ensure!(
109-
validity.len() == len,
110-
"Validity mask length ({}) does not match expected length ({})",
111-
validity.len(),
112-
len
113-
);
114-
115-
// Validate that all fields have the correct length.
116-
for (i, field) in fields.iter().enumerate() {
117-
vortex_ensure!(
118-
field.len() == len,
119-
"Field {} has length {} but expected length {}",
120-
i,
121-
field.len(),
122-
len
123-
);
101+
if cfg!(debug_assertions) {
102+
Self::new(fields, validity)
103+
} else {
104+
Self {
105+
fields,
106+
validity,
107+
len,
108+
}
124109
}
125-
126-
Ok(())
127110
}
128111

129112
/// Decomposes the struct vector into its constituent parts (fields, validity, and length).
@@ -172,7 +155,6 @@ impl VectorOps for StructVector {
172155
Err(immutable_field) => {
173156
// We were unable to take ownership, so we must re-freeze all of the fields
174157
// vectors we took ownership over and reconstruct the original `StructVector`.
175-
176158
let mut all_fields: Vec<Vector> = mutable_fields
177159
.into_iter()
178160
.map(|mut_field| mut_field.freeze())

0 commit comments

Comments
 (0)