Skip to content

Commit b9b02f6

Browse files
authored
Change extension dtype nullability without validating again (#11012)
## Summary `take` and `mask` can make an extension array nullable, while `min_max` builds non-null result dtypes. These operations need to change nullability without failing. Previously, `ExtDType::with_nullability` called the plugin validator again and panicked if it returned an error. An extension type must accept either top-level nullability, so an already valid dtype can be reused with that flag changed. `with_nullability` now relies on this contract instead of validating again. ## Changes Keeps metadata and storage types intact when changing nullability. Also fixes erased dtype `Debug` output, which wrote metadata before the struct and displayed `metadata: Ok(())` in its place. Regression tests cover both nullabilities, validation call counts, and compact and pretty `Debug` output. Checks for the style cleanup are left to CI. ## API Changes Documents that `ExtVTable::validate_dtype` must be independent of the storage dtype's top-level nullability. Validators can still constrain nullability inside nested storage types. --------- Signed-off-by: Connor Tsui <[email protected]>
1 parent ec6c7fb commit b9b02f6

5 files changed

Lines changed: 149 additions & 14 deletions

File tree

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -180,11 +180,9 @@ impl Hash for ExtDTypeRef {
180180

181181
impl fmt::Debug for ExtDTypeRef {
182182
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
183-
let metadata = self.0.metadata_debug(f);
184-
185183
f.debug_struct("ExtDType")
186184
.field("id", &self.id())
187-
.field("metadata", &metadata)
185+
.field("metadata", &MetadataDebug(self.0.as_ref()))
188186
.field("storage_dtype", &self.storage_dtype())
189187
.finish()
190188
}
@@ -206,6 +204,14 @@ impl fmt::Display for ExtDTypeRef {
206204

207205
// Private formatting helpers for Display and Debug impls.
208206

207+
struct MetadataDebug<'a>(&'a dyn DynExtDType);
208+
209+
impl fmt::Debug for MetadataDebug<'_> {
210+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
211+
self.0.metadata_debug(f)
212+
}
213+
}
214+
209215
struct MetadataDisplay<'a>(&'a dyn DynExtDType);
210216
impl fmt::Display for MetadataDisplay<'_> {
211217
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,3 +55,6 @@ mod sealed {
5555
/// This can be the **only** implementor for [`super::typed::DynExtDType`].
5656
impl<V: ExtVTable> Sealed for ExtDType<V> {}
5757
}
58+
59+
#[cfg(test)]
60+
mod tests;
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// SPDX-FileCopyrightText: Copyright the Vortex contributors
3+
4+
//! Regression tests for extension dtype nullability changes and debug output.
5+
6+
use std::sync::atomic::AtomicUsize;
7+
use std::sync::atomic::Ordering;
8+
9+
use rstest::rstest;
10+
use vortex_error::VortexResult;
11+
use vortex_error::vortex_err;
12+
use vortex_session::registry::CachedId;
13+
14+
use super::ExtDType;
15+
use super::ExtId;
16+
use super::ExtVTable;
17+
use crate::dtype::DType;
18+
use crate::dtype::Nullability::NonNullable;
19+
use crate::dtype::Nullability::Nullable;
20+
use crate::dtype::PType;
21+
use crate::extension::datetime::TimeUnit;
22+
use crate::extension::datetime::Timestamp;
23+
use crate::scalar::ScalarValue;
24+
25+
/// The number of times [`CountingValidator::validate_dtype`] ran.
26+
static VALIDATIONS: AtomicUsize = AtomicUsize::new(0);
27+
28+
/// An extension type that counts how often its dtype is validated.
29+
#[derive(Clone, Debug, Default, Eq, Hash, PartialEq)]
30+
struct CountingValidator;
31+
32+
impl ExtVTable for CountingValidator {
33+
type Metadata = u32;
34+
type NativeValue<'a> = &'a ScalarValue;
35+
36+
fn id(&self) -> ExtId {
37+
static ID: CachedId = CachedId::new("test.counting_validator");
38+
*ID
39+
}
40+
41+
fn serialize_metadata(&self, metadata: &Self::Metadata) -> VortexResult<Vec<u8>> {
42+
Ok(metadata.to_le_bytes().to_vec())
43+
}
44+
45+
fn deserialize_metadata(&self, metadata: &[u8]) -> VortexResult<Self::Metadata> {
46+
let bytes = metadata
47+
.try_into()
48+
.map_err(|_| vortex_err!("Expected four metadata bytes, got {}", metadata.len()))?;
49+
50+
Ok(u32::from_le_bytes(bytes))
51+
}
52+
53+
fn validate_dtype(_ext_dtype: &ExtDType<Self>) -> VortexResult<()> {
54+
VALIDATIONS.fetch_add(1, Ordering::Relaxed);
55+
56+
Ok(())
57+
}
58+
59+
fn unpack_native<'a>(
60+
_ext_dtype: &'a ExtDType<Self>,
61+
storage_value: &'a ScalarValue,
62+
) -> VortexResult<Self::NativeValue<'a>> {
63+
Ok(storage_value)
64+
}
65+
}
66+
67+
// One test owns `VALIDATIONS`, so test threads cannot race on the counter.
68+
#[test]
69+
fn test_nullability_changes_do_not_validate_again() -> VortexResult<()> {
70+
for initial in [NonNullable, Nullable] {
71+
VALIDATIONS.store(0, Ordering::Relaxed);
72+
let dtype =
73+
ExtDType::<CountingValidator>::try_new(42, DType::Primitive(PType::I64, initial))?;
74+
assert_eq!(VALIDATIONS.load(Ordering::Relaxed), 1);
75+
76+
for target in [Nullable, NonNullable] {
77+
let changed = dtype.with_nullability(target);
78+
79+
assert_eq!(
80+
changed.storage_dtype(),
81+
&DType::Primitive(PType::I64, target)
82+
);
83+
assert_eq!(changed.metadata::<CountingValidator>(), &42);
84+
assert_eq!(changed.id(), dtype.id());
85+
}
86+
87+
assert_eq!(VALIDATIONS.load(Ordering::Relaxed), 1);
88+
}
89+
90+
Ok(())
91+
}
92+
93+
#[rstest]
94+
#[case::compact(false)]
95+
#[case::pretty(true)]
96+
fn test_debug_preserves_metadata(#[case] pretty: bool) -> VortexResult<()> {
97+
let dtype =
98+
Timestamp::try_new_with_tz(TimeUnit::Milliseconds, Some("UTC".into()), Nullable)?.erased();
99+
let output = if pretty {
100+
format!("{dtype:#?}")
101+
} else {
102+
format!("{dtype:?}")
103+
};
104+
105+
assert!(output.starts_with("ExtDType {"), "{output}");
106+
assert!(output.contains("metadata: TimestampOptions {"), "{output}");
107+
assert!(output.contains("unit: Milliseconds"), "{output}");
108+
assert_eq!(
109+
output.contains("metadata: TimestampOptions {\n"),
110+
pretty,
111+
"{output}"
112+
);
113+
assert_eq!(output.matches("UTC").count(), 1, "{output}");
114+
115+
Ok(())
116+
}

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

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ use std::hash::Hash;
1212
use std::hash::Hasher;
1313
use std::sync::Arc;
1414

15-
use vortex_error::VortexExpect;
1615
use vortex_error::VortexResult;
1716

1817
use crate::dtype::DType;
@@ -88,14 +87,16 @@ impl<V: ExtVTable> ExtDType<V> {
8887
}
8988

9089
/// Returns a new [`ExtDTypeRef`] with the given nullability.
90+
///
91+
/// This does not validate the dtype again, because [`ExtVTable::validate_dtype`] must not
92+
/// depend on the top-level nullability of the storage dtype.
9193
pub fn with_nullability(&self, nullability: Nullability) -> ExtDTypeRef {
92-
let storage_dtype = self.storage_dtype.with_nullability(nullability);
93-
ExtDType::<V>::try_with_vtable(self.vtable.clone(), self.metadata.clone(), storage_dtype)
94-
.vortex_expect(
95-
"Extension DType should not fail validation with the same storage type \
96-
but different nullability",
97-
)
98-
.erased()
94+
Self {
95+
vtable: self.vtable.clone(),
96+
metadata: self.metadata.clone(),
97+
storage_dtype: self.storage_dtype.with_nullability(nullability),
98+
}
99+
.erased()
99100
}
100101

101102
/// Serializes the metadata into a byte vector.

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,15 @@ pub trait ExtVTable: 'static + Sized + Send + Sync + Clone + Debug + Eq + Hash {
4646
///
4747
/// This is called when constructing an [`ExtDType`] and should check both storage dtype and
4848
/// extension metadata.
49+
///
50+
/// The result **must not** depend on the top-level nullability of the storage dtype. Array
51+
/// operations control whether values can be null, so extension types must accept either
52+
/// nullability. [`ExtDType::with_nullability`] relies on this contract to change nullability
53+
/// without validating again. Validators can still constrain nullability inside nested storage
54+
/// types.
55+
///
56+
/// Violating this contract can produce a dtype that the validator rejects. It does not cause
57+
/// undefined behavior.
4958
fn validate_dtype(ext_dtype: &ExtDType<Self>) -> VortexResult<()>;
5059

5160
// Methods related to the extension scalar values.
@@ -67,9 +76,9 @@ pub trait ExtVTable: 'static + Sized + Send + Sync + Clone + Debug + Eq + Hash {
6776

6877
/// Validate and unpack a native value from the storage [`ScalarValue`].
6978
///
70-
/// Note that [`ExtVTable::validate_dtype()`] is always called first to validate the storage
71-
/// [`crate::dtype::DType`], and the [`Scalar`](crate::scalar::Scalar) implementation will
72-
/// verify that the storage value is compatible with the storage dtype on construction.
79+
/// The storage dtype and metadata satisfy [`Self::validate_dtype`].
80+
/// [`Scalar`](crate::scalar::Scalar) construction also verifies that the storage value is
81+
/// compatible with the storage dtype.
7382
///
7483
/// # Errors
7584
///

0 commit comments

Comments
 (0)