Skip to content

Commit 76745ac

Browse files
connortsui20connortsui20
committed
Fix nested scalar hashing
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
1 parent 17f843a commit 76745ac

8 files changed

Lines changed: 199 additions & 8 deletions

File tree

‎vortex-array/src/dtype/dtype_impl.rs‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33

44
use std::fmt::Display;
55
use std::fmt::Formatter;
6+
use std::hash::Hash;
7+
use std::hash::Hasher;
68
use std::sync::Arc;
79

810
use DType::*;
@@ -144,6 +146,36 @@ impl DType {
144146
}
145147
}
146148

149+
/// Hash this dtype using the same equivalence relation as [`Self::eq_ignore_nullability`].
150+
pub(crate) fn hash_ignore_nullability<H: Hasher>(&self, state: &mut H) {
151+
std::mem::discriminant(self).hash(state);
152+
153+
match self {
154+
Null | Bool(_) | Utf8(_) | Binary(_) | Variant(_) => {}
155+
Primitive(ptype, _) => ptype.hash(state),
156+
Decimal(decimal, _) => decimal.hash(state),
157+
List(element, _) => element.hash_ignore_nullability(state),
158+
FixedSizeList(element, size, _) => {
159+
size.hash(state);
160+
element.hash_ignore_nullability(state);
161+
}
162+
Struct(fields, _) => {
163+
fields.names().hash(state);
164+
for field in fields.fields() {
165+
field.hash_ignore_nullability(state);
166+
}
167+
}
168+
Union(variants) => {
169+
variants.names().hash(state);
170+
variants.type_ids().hash(state);
171+
for variant in variants.variants() {
172+
variant.hash_ignore_nullability(state);
173+
}
174+
}
175+
Extension(ext) => ext.hash_ignore_nullability(state),
176+
}
177+
}
178+
147179
/// Returns `true` if `self` is a subset type of `other, otherwise `false`.
148180
///
149181
/// If `self` is nullable, this means that the other `DType` must also be nullable (since a
@@ -507,18 +539,29 @@ impl Display for DType {
507539

508540
#[cfg(test)]
509541
mod tests {
542+
use std::collections::hash_map::DefaultHasher;
543+
use std::hash::Hasher;
510544
use std::sync::Arc;
511545

546+
use vortex_error::VortexResult;
547+
512548
use crate::dtype::DType;
513549
use crate::dtype::Nullability::NonNullable;
514550
use crate::dtype::Nullability::Nullable;
515551
use crate::dtype::PType;
552+
use crate::dtype::UnionVariants;
516553
use crate::dtype::decimal::DecimalDType;
517554
use crate::extension::datetime::Date;
518555
use crate::extension::datetime::Time;
519556
use crate::extension::datetime::TimeUnit;
520557
use crate::extension::datetime::Timestamp;
521558

559+
fn hash_ignore_nullability(dtype: &DType) -> u64 {
560+
let mut hasher = DefaultHasher::new();
561+
dtype.hash_ignore_nullability(&mut hasher);
562+
hasher.finish()
563+
}
564+
522565
#[test]
523566
fn test_ext_dtype_eq_ignore_nullability() {
524567
let d1 = DType::Extension(Time::new(TimeUnit::Seconds, Nullable).erased());
@@ -534,6 +577,31 @@ mod tests {
534577
assert!(!t1.eq_ignore_nullability(&t2));
535578
}
536579

580+
#[test]
581+
fn test_union_dtype_hash_ignores_variant_nullability() -> VortexResult<()> {
582+
let lhs = DType::Union(UnionVariants::try_new(
583+
["int", "string"].into(),
584+
vec![
585+
DType::Primitive(PType::I32, Nullable),
586+
DType::Utf8(NonNullable),
587+
],
588+
vec![5, 9],
589+
)?);
590+
let rhs = DType::Union(UnionVariants::try_new(
591+
["int", "string"].into(),
592+
vec![
593+
DType::Primitive(PType::I32, NonNullable),
594+
DType::Utf8(Nullable),
595+
],
596+
vec![5, 9],
597+
)?);
598+
599+
assert!(lhs.eq_ignore_nullability(&rhs));
600+
assert_eq!(hash_ignore_nullability(&lhs), hash_ignore_nullability(&rhs));
601+
602+
Ok(())
603+
}
604+
537605
#[test]
538606
fn element_size_null() {
539607
assert_eq!(DType::Null.element_size(), Some(0));

‎vortex-array/src/dtype/extension/erased.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,14 @@ impl ExtDTypeRef {
7575
.eq_ignore_nullability(other.storage_dtype())
7676
}
7777

78+
/// Hash this extension dtype using the same equivalence relation as
79+
/// [`Self::eq_ignore_nullability`].
80+
pub(crate) fn hash_ignore_nullability<H: Hasher>(&self, state: &mut H) {
81+
self.id().hash(state);
82+
self.0.metadata_hash(state);
83+
self.storage_dtype().hash_ignore_nullability(state);
84+
}
85+
7886
// TODO(connor): We should add a different type that returns something that can be serialized.
7987
/// Serialize the metadata into a byte vector.
8088
pub fn serialize_metadata(&self) -> VortexResult<Vec<u8>> {

‎vortex-array/src/scalar/scalar_impl.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ impl Scalar {
282282
/// values have equal hashes.
283283
impl Hash for Scalar {
284284
fn hash<H: Hasher>(&self, state: &mut H) {
285-
self.dtype.as_nonnullable().hash(state);
285+
self.dtype.hash_ignore_nullability(state);
286286
self.value.hash(state);
287287
}
288288
}

‎vortex-array/src/scalar/tests/primitives.rs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
66
#[cfg(test)]
77
mod tests {
8+
use std::collections::hash_map::DefaultHasher;
9+
use std::hash::Hash;
10+
use std::hash::Hasher;
811
use std::sync::Arc;
912

1013
use vortex_buffer::ByteBuffer;
@@ -24,6 +27,12 @@ mod tests {
2427
use crate::scalar::Scalar;
2528
use crate::scalar::ScalarValue;
2629

30+
fn scalar_hash(scalar: &Scalar) -> u64 {
31+
let mut hasher = DefaultHasher::new();
32+
scalar.hash(&mut hasher);
33+
hasher.finish()
34+
}
35+
2736
#[test]
2837
fn default_value_for_complex_dtype() {
2938
let struct_dtype = DType::struct_(
@@ -412,6 +421,23 @@ mod tests {
412421
assert_eq!(set.len(), 5);
413422
}
414423

424+
#[test]
425+
fn test_scalar_hash_ignores_nested_nullability() {
426+
let nullable = Scalar::list(
427+
DType::Primitive(PType::I32, Nullability::Nullable),
428+
vec![Scalar::primitive(42_i32, Nullability::Nullable)],
429+
Nullability::NonNullable,
430+
);
431+
let non_nullable = Scalar::list(
432+
DType::Primitive(PType::I32, Nullability::NonNullable),
433+
vec![Scalar::primitive(42_i32, Nullability::NonNullable)],
434+
Nullability::NonNullable,
435+
);
436+
437+
assert_eq!(nullable, non_nullable);
438+
assert_eq!(scalar_hash(&nullable), scalar_hash(&non_nullable));
439+
}
440+
415441
#[test]
416442
fn test_scalar_partial_ord_incompatible_types() {
417443
let int_scalar = Scalar::primitive(42i32, Nullability::NonNullable);

‎vortex-array/src/scalar/typed_view/extension/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ impl PartialOrd for ExtScalar<'_> {
139139

140140
impl Hash for ExtScalar<'_> {
141141
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
142-
self.ext_dtype.hash(state);
142+
self.ext_dtype.hash_ignore_nullability(state);
143143
self.to_storage_scalar().hash(state);
144144
}
145145
}

‎vortex-array/src/scalar/typed_view/extension/tests.rs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
// SPDX-License-Identifier: Apache-2.0
22
// SPDX-FileCopyrightText: Copyright the Vortex contributors
33

4+
use std::collections::hash_map::DefaultHasher;
5+
use std::hash::Hash;
6+
use std::hash::Hasher;
7+
48
use vortex_error::VortexResult;
59
use vortex_error::vortex_bail;
610

@@ -174,6 +178,28 @@ fn test_ext_scalar_hash() {
174178
assert_eq!(set.len(), 2);
175179
}
176180

181+
#[test]
182+
fn test_ext_scalar_hash_ignores_storage_nullability() {
183+
let nullable = Scalar::extension::<TestI32Ext>(
184+
EmptyMetadata,
185+
Scalar::primitive(42_i32, Nullability::Nullable),
186+
);
187+
let non_nullable = Scalar::extension::<TestI32Ext>(
188+
EmptyMetadata,
189+
Scalar::primitive(42_i32, Nullability::NonNullable),
190+
);
191+
let nullable = nullable.as_extension();
192+
let non_nullable = non_nullable.as_extension();
193+
194+
assert_eq!(nullable, non_nullable);
195+
196+
let mut nullable_hasher = DefaultHasher::new();
197+
nullable.hash(&mut nullable_hasher);
198+
let mut non_nullable_hasher = DefaultHasher::new();
199+
non_nullable.hash(&mut non_nullable_hasher);
200+
assert_eq!(nullable_hasher.finish(), non_nullable_hasher.finish());
201+
}
202+
177203
#[test]
178204
fn test_ext_scalar_storage() {
179205
let storage_scalar = Scalar::primitive(42i32, Nullability::NonNullable);

‎vortex-array/src/scalar/typed_view/list.rs‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ impl PartialOrd for ListScalar<'_> {
9090

9191
impl Hash for ListScalar<'_> {
9292
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
93-
self.dtype.hash(state);
93+
self.dtype.hash_ignore_nullability(state);
9494
self.elements().hash(state);
9595
}
9696
}
@@ -225,6 +225,9 @@ impl<'a> ListScalar<'a> {
225225

226226
#[cfg(test)]
227227
mod tests {
228+
use std::collections::hash_map::DefaultHasher;
229+
use std::hash::Hash;
230+
use std::hash::Hasher;
228231
use std::sync::Arc;
229232

230233
use super::*;
@@ -422,10 +425,6 @@ mod tests {
422425

423426
#[test]
424427
fn test_list_hash() {
425-
use std::collections::hash_map::DefaultHasher;
426-
use std::hash::Hash;
427-
use std::hash::Hasher;
428-
429428
let element_dtype = Arc::new(DType::Primitive(PType::I32, Nullability::NonNullable));
430429
let children = vec![
431430
Scalar::primitive(1i32, Nullability::NonNullable),
@@ -446,6 +445,30 @@ mod tests {
446445
assert_eq!(hash1, hash2);
447446
}
448447

448+
#[test]
449+
fn test_list_hash_ignores_nested_nullability() {
450+
let nullable = Scalar::list(
451+
DType::Primitive(PType::I32, Nullability::Nullable),
452+
vec![Scalar::primitive(42_i32, Nullability::Nullable)],
453+
Nullability::NonNullable,
454+
);
455+
let non_nullable = Scalar::list(
456+
DType::Primitive(PType::I32, Nullability::NonNullable),
457+
vec![Scalar::primitive(42_i32, Nullability::NonNullable)],
458+
Nullability::NonNullable,
459+
);
460+
let nullable = nullable.as_list();
461+
let non_nullable = non_nullable.as_list();
462+
463+
assert_eq!(nullable, non_nullable);
464+
465+
let mut nullable_hasher = DefaultHasher::new();
466+
nullable.hash(&mut nullable_hasher);
467+
let mut non_nullable_hasher = DefaultHasher::new();
468+
non_nullable.hash(&mut non_nullable_hasher);
469+
assert_eq!(nullable_hasher.finish(), non_nullable_hasher.finish());
470+
}
471+
449472
#[test]
450473
fn test_vec_conversion() {
451474
let element_dtype = Arc::new(DType::Primitive(PType::I32, Nullability::NonNullable));

‎vortex-array/src/scalar/typed_view/struct_.rs‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ impl PartialOrd for StructScalar<'_> {
103103

104104
impl Hash for StructScalar<'_> {
105105
fn hash<H: Hasher>(&self, state: &mut H) {
106-
self.dtype.hash(state);
106+
self.dtype.hash_ignore_nullability(state);
107107
if let Some(fields) = self.fields_iter() {
108108
for f in fields {
109109
f.hash(state);
@@ -329,6 +329,10 @@ impl Scalar {
329329

330330
#[cfg(test)]
331331
mod tests {
332+
use std::collections::hash_map::DefaultHasher;
333+
use std::hash::Hash;
334+
use std::hash::Hasher;
335+
332336
use super::*;
333337
use crate::dtype::DType;
334338
use crate::dtype::Nullability;
@@ -614,6 +618,42 @@ mod tests {
614618
assert_ne!(scalar1.as_struct(), scalar3.as_struct());
615619
}
616620

621+
#[test]
622+
fn test_struct_hash_ignores_nested_nullability() {
623+
let nullable_dtype = DType::Struct(
624+
StructFields::new(
625+
["value"].into(),
626+
vec![DType::Primitive(I32, Nullability::Nullable)],
627+
),
628+
Nullability::NonNullable,
629+
);
630+
let non_nullable_dtype = DType::Struct(
631+
StructFields::new(
632+
["value"].into(),
633+
vec![DType::Primitive(I32, Nullability::NonNullable)],
634+
),
635+
Nullability::NonNullable,
636+
);
637+
let nullable = Scalar::struct_(
638+
nullable_dtype,
639+
[Scalar::primitive(42_i32, Nullability::Nullable)],
640+
);
641+
let non_nullable = Scalar::struct_(
642+
non_nullable_dtype,
643+
[Scalar::primitive(42_i32, Nullability::NonNullable)],
644+
);
645+
let nullable = nullable.as_struct();
646+
let non_nullable = non_nullable.as_struct();
647+
648+
assert_eq!(nullable, non_nullable);
649+
650+
let mut nullable_hasher = DefaultHasher::new();
651+
nullable.hash(&mut nullable_hasher);
652+
let mut non_nullable_hasher = DefaultHasher::new();
653+
non_nullable.hash(&mut non_nullable_hasher);
654+
assert_eq!(nullable_hasher.finish(), non_nullable_hasher.finish());
655+
}
656+
617657
#[test]
618658
fn test_struct_partial_ord() {
619659
let (_, _, dtype) = setup_types();

0 commit comments

Comments
 (0)