Skip to content

Commit 7d0495f

Browse files
joacocjoacoc
committed
Move hash/insert/contains scalar into partial.rs, add more documentation comments and fix primitive test for validation
1 parent 8d81b78 commit 7d0495f

6 files changed

Lines changed: 197 additions & 155 deletions

File tree

‎vortex-layout/src/layouts/zoned/aggregates/bloom_filter/canonical/bool.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ mod tests {
109109
bloom_filter.contains_valid_scalar(&Scalar::bool(false, nullability))?,
110110
expect_false
111111
);
112+
112113
Ok(())
113114
}
114115
}

‎vortex-layout/src/layouts/zoned/aggregates/bloom_filter/canonical/extension.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ pub(super) fn accumulate_extension(
1616
) -> VortexResult<()> {
1717
let storage = array.storage_array().clone();
1818
let canonical = storage.execute::<Canonical>(ctx)?;
19+
1920
super::accumulate_canonical(&canonical, partial, ctx)
2021
}
2122

‎vortex-layout/src/layouts/zoned/aggregates/bloom_filter/canonical/primitive.rs‎

Lines changed: 60 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -170,61 +170,75 @@ mod tests {
170170
Ok(())
171171
}
172172

173+
/// The following three test will check if the validity cases
174+
/// are correctly implemented for the three branches.
173175
#[rstest]
174176
#[case(&[10i8, 20, 30, 40, 50])]
175-
fn validity<T>(#[case] present: &[T]) -> VortexResult<()>
177+
fn validity_all_true<T>(#[case] present: &[T]) -> VortexResult<()>
176178
where
177179
T: Copy + NativePType + Into<PValue>,
178180
{
179-
{
180-
let ctx = setup()?;
181-
let all_valid = PrimitiveArray::from_option_iter(present.iter().map(|&v| Some(v)));
182-
let bloom_filter = build_filter(
183-
all_valid.into_array(),
184-
DType::Primitive(T::PTYPE, Nullability::Nullable),
185-
ctx,
186-
)?;
187-
for &v in present {
188-
let scalar = Scalar::primitive(v, Nullability::Nullable);
189-
assert!(bloom_filter.contains_valid_scalar(&scalar)?);
190-
}
181+
let ctx = setup()?;
182+
let all_valid = PrimitiveArray::from_option_iter(present.iter().map(|&v| Some(v)));
183+
let bloom_filter = build_filter(
184+
all_valid.into_array(),
185+
DType::Primitive(T::PTYPE, Nullability::Nullable),
186+
ctx,
187+
)?;
188+
189+
for &v in present {
190+
let scalar = Scalar::primitive(v, Nullability::Nullable);
191+
assert!(bloom_filter.contains_valid_scalar(&scalar)?);
191192
}
192193

193-
// Mask::AllFalse: every position null. Nothing should ever be found — including the
194-
// `T::default()` filler that `from_option_iter` writes into null slots internally.
195-
{
196-
let ctx = setup()?;
197-
let all_invalid = PrimitiveArray::from_option_iter(present.iter().map(|_| None::<T>));
198-
let bloom_filter = build_filter(
199-
all_invalid.into_array(),
200-
DType::Primitive(T::PTYPE, Nullability::Nullable),
201-
ctx,
202-
)?;
203-
for &v in present {
204-
let scalar = Scalar::primitive(v, Nullability::Nullable);
205-
assert!(!bloom_filter.contains_valid_scalar(&scalar)?);
206-
}
194+
Ok(())
195+
}
196+
197+
#[rstest]
198+
#[case(&[10i8, 20, 30, 40, 50])]
199+
fn validity_all_false<T>(#[case] present: &[T]) -> VortexResult<()>
200+
where
201+
T: Copy + NativePType + Into<PValue>,
202+
{
203+
let ctx = setup()?;
204+
let all_invalid = PrimitiveArray::from_option_iter(present.iter().map(|_| None::<T>));
205+
let bloom_filter = build_filter(
206+
all_invalid.into_array(),
207+
DType::Primitive(T::PTYPE, Nullability::Nullable),
208+
ctx,
209+
)?;
210+
211+
for &v in present {
212+
let scalar = Scalar::primitive(v, Nullability::Nullable);
213+
assert!(!bloom_filter.contains_valid_scalar(&scalar)?);
207214
}
208215

209-
// Mask::Values: alternating valid/null. Only the valid positions should be members,
210-
// and the default filler at null slots must not leak through as a false member.
211-
{
212-
let ctx = setup()?;
213-
let mixed: Vec<Option<T>> = present
214-
.iter()
215-
.enumerate()
216-
.map(|(i, &v)| if i % 2 == 0 { Some(v) } else { None })
217-
.collect();
218-
let bloom_filter = build_filter(
219-
PrimitiveArray::from_option_iter(mixed).into_array(),
220-
DType::Primitive(T::PTYPE, Nullability::Nullable),
221-
ctx,
222-
)?;
223-
for (i, &v) in present.iter().enumerate() {
224-
if i % 2 == 0 {
225-
let scalar = Scalar::primitive(v, Nullability::Nullable);
226-
assert!(bloom_filter.contains_valid_scalar(&scalar)?);
227-
}
216+
Ok(())
217+
}
218+
219+
#[rstest]
220+
#[case(&[10i8, 20, 30, 40, 50])]
221+
fn validity_mixed<T>(#[case] present: &[T]) -> VortexResult<()>
222+
where
223+
T: Copy + NativePType + Into<PValue>,
224+
{
225+
let ctx = setup()?;
226+
let mixed: Vec<Option<T>> = present
227+
.iter()
228+
.enumerate()
229+
.map(|(i, &v)| if i % 2 == 0 { Some(v) } else { None })
230+
.collect();
231+
232+
let bloom_filter = build_filter(
233+
PrimitiveArray::from_option_iter(mixed).into_array(),
234+
DType::Primitive(T::PTYPE, Nullability::Nullable),
235+
ctx,
236+
)?;
237+
238+
for (i, &v) in present.iter().enumerate() {
239+
if i % 2 == 0 {
240+
let scalar = Scalar::primitive(v, Nullability::Nullable);
241+
assert!(bloom_filter.contains_valid_scalar(&scalar)?);
228242
}
229243
}
230244

‎vortex-layout/src/layouts/zoned/aggregates/bloom_filter/constant.rs‎

Lines changed: 2 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,7 @@
22
// SPDX-FileCopyrightText: Copyright the Vortex contributors
33

44
use vortex_array::arrays::ConstantArray;
5-
use vortex_array::dtype::DType;
6-
use vortex_array::dtype::PType;
7-
use vortex_array::match_each_float_ptype;
8-
use vortex_array::match_each_integer_ptype;
9-
use vortex_array::scalar::DecimalValue;
10-
use vortex_array::scalar::Scalar;
11-
use vortex_error::VortexExpect;
125
use vortex_error::VortexResult;
13-
use vortex_error::vortex_err;
146

157
use crate::layouts::zoned::aggregates::bloom_filter::BloomPartial;
168

@@ -25,92 +17,9 @@ pub(super) fn accumulate_constant(
2517
return Ok(());
2618
}
2719

28-
partial.insert_hash(partial.hash_valid_scalar(scalar)?);
29-
Ok(())
30-
}
31-
32-
impl BloomPartial {
33-
/// Scalar values must be valid otherwise the function will raise an err.
34-
///
35-
/// This function is used by both, for accumulating scalars,
36-
/// but also to get a scalar value membership.
37-
pub(in crate::layouts::zoned) fn hash_valid_scalar(
38-
&self,
39-
scalar: &Scalar,
40-
) -> VortexResult<u64> {
41-
if scalar.is_null() {
42-
return Err(vortex_err!("cannot hash invalid scalars in bloom filter"));
43-
}
44-
45-
Ok(match scalar.dtype() {
46-
DType::Extension(_) => {
47-
self.hash_valid_scalar(&scalar.as_extension().to_storage_scalar())?
48-
}
49-
DType::Bool(_) => self.hash(
50-
scalar
51-
.as_bool()
52-
.value()
53-
.vortex_expect("non-null boolean value"),
54-
),
55-
DType::Primitive(ptype, _) => match ptype {
56-
PType::F16 | PType::F32 | PType::F64 => {
57-
match_each_float_ptype!(ptype, |T| {
58-
let value = scalar
59-
.as_primitive()
60-
.typed_value::<T>()
61-
.vortex_expect("non-null primitive value");
62-
self.hash(value.to_bits())
63-
})
64-
}
65-
_ => match_each_integer_ptype!(ptype, |T| {
66-
let value = scalar
67-
.as_primitive()
68-
.typed_value::<T>()
69-
.vortex_expect("non-null primitive value");
70-
self.hash(value)
71-
}),
72-
},
73-
DType::Decimal(..) => {
74-
let decimal = scalar
75-
.as_decimal()
76-
.decimal_value()
77-
.vortex_expect("non-null decimal value");
78-
match decimal {
79-
DecimalValue::I8(v) => self.hash(v),
80-
DecimalValue::I16(v) => self.hash(v),
81-
DecimalValue::I32(v) => self.hash(v),
82-
DecimalValue::I64(v) => self.hash(v),
83-
DecimalValue::I128(v) => self.hash(v),
84-
DecimalValue::I256(v) => self.hash(v),
85-
}
86-
}
87-
DType::Utf8(_) => {
88-
let buffer = scalar
89-
.as_utf8()
90-
.value()
91-
.vortex_expect("non-null utf8 value");
92-
self.hash(buffer.as_bytes())
93-
}
94-
DType::Binary(_) => {
95-
let buffer = scalar
96-
.as_binary()
97-
.value()
98-
.vortex_expect("non-null binary value");
99-
self.hash(buffer.as_slice())
100-
}
101-
other => {
102-
return Err(vortex_err!("bloom filter does not support dtype {other}"));
103-
}
104-
})
105-
}
20+
partial.insert_valid_scalar(scalar)?;
10621

107-
pub(in crate::layouts::zoned) fn contains_valid_scalar(
108-
&self,
109-
scalar: &Scalar,
110-
) -> VortexResult<bool> {
111-
let hash = self.hash_valid_scalar(scalar)?;
112-
Ok(self.find_hash(hash))
113-
}
22+
Ok(())
11423
}
11524

11625
#[cfg(test)]

‎vortex-layout/src/layouts/zoned/aggregates/bloom_filter/mod.rs‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -363,16 +363,11 @@ pub(in crate::layouts::zoned::aggregates::bloom_filter) mod test_utils {
363363
);
364364

365365
for i in 0..100i64 {
366-
let hash = partial.hash(i);
367-
assert!(partial.find_hash(hash), "value {i} missing after merge");
366+
assert!(partial.contains(i), "value {i} missing after merge");
368367
}
369368

370369
for i in 101..200i64 {
371-
let hash = partial.hash(i);
372-
assert!(
373-
!partial.find_hash(hash),
374-
"value {i} shouldn't be present after"
375-
);
370+
assert!(!partial.contains(i), "value {i} shouldn't be present after");
376371
}
377372

378373
Ok(())

0 commit comments

Comments
 (0)