diff --git a/vortex-array/src/arrays/extension/vtable/rules.rs b/vortex-array/src/arrays/extension/vtable/rules.rs index 0ff04f6cd42..92a34c76cab 100644 --- a/vortex-array/src/arrays/extension/vtable/rules.rs +++ b/vortex-array/src/arrays/extension/vtable/rules.rs @@ -73,7 +73,7 @@ mod tests { use crate::expr::Operator; use crate::optimizer::ArrayOptimizer; - #[derive(Clone, Debug, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct TestExt; impl ExtDTypeVTable for TestExt { type Metadata = EmptyMetadata; @@ -82,7 +82,11 @@ mod tests { ExtID::new_ref("test_ext") } - fn validate(&self, _options: &Self::Metadata, _storage_dtype: &DType) -> VortexResult<()> { + fn validate_dtype( + &self, + _options: &Self::Metadata, + _storage_dtype: &DType, + ) -> VortexResult<()> { Ok(()) } } @@ -153,7 +157,7 @@ mod tests { #[test] fn test_scalar_fn_no_pushdown_different_ext_types() { - #[derive(Clone, Debug, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct TestExt2; impl ExtDTypeVTable for TestExt2 { type Metadata = EmptyMetadata; @@ -162,7 +166,7 @@ mod tests { ExtID::new_ref("test_ext_2") } - fn validate( + fn validate_dtype( &self, _options: &Self::Metadata, _storage_dtype: &DType, diff --git a/vortex-dtype/src/datetime/date.rs b/vortex-dtype/src/datetime/date.rs index 66738a2cbb4..96040186f10 100644 --- a/vortex-dtype/src/datetime/date.rs +++ b/vortex-dtype/src/datetime/date.rs @@ -15,7 +15,7 @@ use crate::extension::ExtDTypeVTable; use crate::extension::ExtID; /// Date DType. -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] pub struct Date; impl Date { @@ -54,7 +54,7 @@ impl ExtDTypeVTable for Date { TimeUnit::try_from(tag) } - fn validate(&self, metadata: &Self::Metadata, storage_dtype: &DType) -> VortexResult<()> { + fn validate_dtype(&self, metadata: &Self::Metadata, storage_dtype: &DType) -> VortexResult<()> { let ptype = date_ptype(metadata) .ok_or_else(|| vortex_err!("Date type does not support time unit {}", metadata))?; diff --git a/vortex-dtype/src/datetime/time.rs b/vortex-dtype/src/datetime/time.rs index f1f45014058..b654a30b4fd 100644 --- a/vortex-dtype/src/datetime/time.rs +++ b/vortex-dtype/src/datetime/time.rs @@ -15,7 +15,7 @@ use crate::extension::ExtDTypeVTable; use crate::extension::ExtID; /// Time DType. -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] pub struct Time; impl Time { @@ -50,7 +50,7 @@ impl ExtDTypeVTable for Time { TimeUnit::try_from(tag) } - fn validate(&self, metadata: &Self::Metadata, storage_dtype: &DType) -> VortexResult<()> { + fn validate_dtype(&self, metadata: &Self::Metadata, storage_dtype: &DType) -> VortexResult<()> { let ptype = time_ptype(metadata) .ok_or_else(|| vortex_err!("Time type does not support time unit {}", metadata))?; diff --git a/vortex-dtype/src/datetime/timestamp.rs b/vortex-dtype/src/datetime/timestamp.rs index 56bbdcdd094..d86d62ffc11 100644 --- a/vortex-dtype/src/datetime/timestamp.rs +++ b/vortex-dtype/src/datetime/timestamp.rs @@ -22,11 +22,11 @@ use crate::extension::ExtDTypeVTable; use crate::extension::ExtID; /// Timestamp DType. -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] pub struct Timestamp; impl Timestamp { - /// Creates a new Timestamp extension dtype with the given time unit and nullability. + /// Creates a new Timestamp extension =dtype with the given time unit and nullability. pub fn new(time_unit: TimeUnit, nullability: Nullability) -> ExtDType { Self::new_with_tz(time_unit, None, nullability) } @@ -120,7 +120,11 @@ impl ExtDTypeVTable for Timestamp { }) } - fn validate(&self, _metadata: &Self::Metadata, storage_dtype: &DType) -> VortexResult<()> { + fn validate_dtype( + &self, + _metadata: &Self::Metadata, + storage_dtype: &DType, + ) -> VortexResult<()> { vortex_ensure!( matches!(storage_dtype, DType::Primitive(PType::I64, _)), "Timestamp storage dtype must be i64" diff --git a/vortex-dtype/src/extension/mod.rs b/vortex-dtype/src/extension/mod.rs index d287120aa04..d6d955346c5 100644 --- a/vortex-dtype/src/extension/mod.rs +++ b/vortex-dtype/src/extension/mod.rs @@ -47,7 +47,7 @@ impl ExtDType { metadata: V::Metadata, storage_dtype: DType, ) -> VortexResult { - vtable.validate(&metadata, &storage_dtype)?; + vtable.validate_dtype(&metadata, &storage_dtype)?; Ok(Self(Arc::new(ExtDTypeAdapter:: { vtable, metadata, diff --git a/vortex-dtype/src/extension/vtable.rs b/vortex-dtype/src/extension/vtable.rs index b650c187c5e..2832c60c3a5 100644 --- a/vortex-dtype/src/extension/vtable.rs +++ b/vortex-dtype/src/extension/vtable.rs @@ -14,7 +14,7 @@ use crate::ExtID; use crate::extension::ExtDTypeRef; /// The public API for defining new extension DTypes. -pub trait ExtDTypeVTable: 'static + Sized + Send + Sync + Clone + Debug { +pub trait ExtDTypeVTable: 'static + Sized + Send + Sync + Clone + Debug + Eq + Hash { /// Associated type containing the deserialized metadata for this extension type type Metadata: 'static + Send + Sync + Clone + Debug + Display + Eq + Hash; @@ -40,23 +40,20 @@ pub trait ExtDTypeVTable: 'static + Sized + Send + Sync + Clone + Debug { } /// Validate that the given storage type is compatible with this extension type. - fn validate(&self, metadata: &Self::Metadata, storage_dtype: &DType) -> VortexResult<()>; + fn validate_dtype(&self, metadata: &Self::Metadata, storage_dtype: &DType) -> VortexResult<()>; } /// A dynamic vtable for extension types, used for type-erased deserialization. // TODO(ngates): consider renaming this to ExtDTypePlugin or similar? -pub trait DynVTable: 'static + Send + Sync + Debug { +pub trait DynExtDTypeVTable: 'static + Send + Sync + Debug { /// Returns the ID for this extension type. fn id(&self) -> ExtID; /// Deserialize an extension type from serialized metadata. fn deserialize(&self, data: &[u8], storage_dtype: DType) -> VortexResult; - - /// Clones this vtable into a boxed trait object. - fn clone_box(&self) -> Box; } -impl DynVTable for V { +impl DynExtDTypeVTable for V { fn id(&self) -> ExtID { ExtDTypeVTable::id(self) } @@ -65,10 +62,6 @@ impl DynVTable for V { let metadata = ExtDTypeVTable::deserialize(self, data)?; Ok(ExtDType::try_with_vtable(self.clone(), metadata, storage_dtype)?.erased()) } - - fn clone_box(&self) -> Box { - Box::new(self.clone()) - } } /// An empty metadata struct for extension dtypes that do not require any metadata. diff --git a/vortex-dtype/src/session.rs b/vortex-dtype/src/session.rs index 5f8afcd780e..d17fa1c94b5 100644 --- a/vortex-dtype/src/session.rs +++ b/vortex-dtype/src/session.rs @@ -12,11 +12,11 @@ use vortex_session::registry::Registry; use crate::datetime::Date; use crate::datetime::Time; use crate::datetime::Timestamp; -use crate::extension::DynVTable; +use crate::extension::DynExtDTypeVTable; use crate::extension::ExtDTypeVTable; /// Registry for extension dtypes. -pub type ExtDTypeRegistry = Registry>; +pub type ExtDTypeRegistry = Registry>; /// Session for managing extension dtypes. #[derive(Debug)] @@ -43,7 +43,7 @@ impl DTypeSession { /// Register an extension DType with the Vortex session. pub fn register(&self, vtable: V) { self.registry - .register(vtable.id(), Arc::new(vtable) as Arc); + .register(vtable.id(), Arc::new(vtable) as Arc); } /// Return the registry of extension dtypes. diff --git a/vortex-duckdb/src/convert/dtype.rs b/vortex-duckdb/src/convert/dtype.rs index 6c468811f54..d6481cafc70 100644 --- a/vortex-duckdb/src/convert/dtype.rs +++ b/vortex-duckdb/src/convert/dtype.rs @@ -567,7 +567,7 @@ mod tests { use vortex::dtype::ExtID; use vortex::dtype::PType; - #[derive(Clone, Debug, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct TestExt; impl ExtDTypeVTable for TestExt { type Metadata = EmptyMetadata; @@ -576,7 +576,7 @@ mod tests { ExtID::new_ref("unknown.extension") } - fn validate( + fn validate_dtype( &self, _options: &Self::Metadata, _storage_dtype: &DType, diff --git a/vortex-scalar/src/arrow/tests.rs b/vortex-scalar/src/arrow/tests.rs index e4eb451ac42..5b1d0390e00 100644 --- a/vortex-scalar/src/arrow/tests.rs +++ b/vortex-scalar/src/arrow/tests.rs @@ -262,7 +262,7 @@ fn test_list_scalar_to_arrow_todo() { fn test_non_temporal_extension_to_arrow_todo() { use vortex_dtype::ExtID; - #[derive(Debug, Clone, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct SomeExt; impl ExtDTypeVTable for SomeExt { type Metadata = String; @@ -279,7 +279,11 @@ fn test_non_temporal_extension_to_arrow_todo() { vortex_bail!("not implemented") } - fn validate(&self, _options: &Self::Metadata, _storage_dtype: &DType) -> VortexResult<()> { + fn validate_dtype( + &self, + _options: &Self::Metadata, + _storage_dtype: &DType, + ) -> VortexResult<()> { Ok(()) } } diff --git a/vortex-scalar/src/extension.rs b/vortex-scalar/src/extension.rs index d64ef9f22c4..5983b9a13ab 100644 --- a/vortex-scalar/src/extension.rs +++ b/vortex-scalar/src/extension.rs @@ -167,7 +167,7 @@ mod tests { use crate::Scalar; use crate::ScalarValue; - #[derive(Debug, Clone, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct TestExt; impl ExtDTypeVTable for TestExt { type Metadata = EmptyMetadata; @@ -176,7 +176,11 @@ mod tests { ExtID::new_ref("test_ext") } - fn validate(&self, _options: &Self::Metadata, _storage_dtype: &DType) -> VortexResult<()> { + fn validate_dtype( + &self, + _options: &Self::Metadata, + _storage_dtype: &DType, + ) -> VortexResult<()> { Ok(()) } } @@ -234,7 +238,7 @@ mod tests { #[test] fn test_ext_scalar_partial_ord_different_types() { - #[derive(Clone, Debug, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct TestExt2; impl ExtDTypeVTable for TestExt2 { type Metadata = EmptyMetadata; @@ -243,7 +247,7 @@ mod tests { ExtID::new_ref("test_ext_2") } - fn validate( + fn validate_dtype( &self, _options: &Self::Metadata, _storage_dtype: &DType, @@ -413,7 +417,7 @@ mod tests { #[test] fn test_ext_scalar_with_metadata() { - #[derive(Clone, Debug, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct TestExtMetadata; impl ExtDTypeVTable for TestExtMetadata { type Metadata = usize; @@ -422,7 +426,7 @@ mod tests { ExtID::new_ref("test_ext_metadata") } - fn validate( + fn validate_dtype( &self, _options: &Self::Metadata, _storage_dtype: &DType, diff --git a/vortex-scalar/src/tests/casting.rs b/vortex-scalar/src/tests/casting.rs index cb05f1fcd8a..3d38699f426 100644 --- a/vortex-scalar/src/tests/casting.rs +++ b/vortex-scalar/src/tests/casting.rs @@ -24,7 +24,7 @@ mod tests { use crate::Scalar; use crate::ScalarValue; - #[derive(Clone, Debug, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct Apples; impl ExtDTypeVTable for Apples { type Metadata = usize; @@ -33,7 +33,11 @@ mod tests { ExtID::new_ref("apples") } - fn validate(&self, _options: &Self::Metadata, _storage_dtype: &DType) -> VortexResult<()> { + fn validate_dtype( + &self, + _options: &Self::Metadata, + _storage_dtype: &DType, + ) -> VortexResult<()> { Ok(()) } } @@ -311,7 +315,7 @@ mod tests { #[test] fn test_extension_dtype_coercion() { // Create an extension type with f16 storage - #[derive(Debug, Clone, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct F16Ext; impl ExtDTypeVTable for F16Ext { type Metadata = usize; @@ -320,7 +324,7 @@ mod tests { ExtID::new_ref("f16_ext") } - fn validate( + fn validate_dtype( &self, _options: &Self::Metadata, _storage_dtype: &DType, @@ -356,7 +360,7 @@ mod tests { #[test] fn test_extension_dtype_nested_struct_coercion() { // Create an extension type with struct storage that contains f16 field - #[derive(Debug, Clone, Default)] + #[derive(Clone, Debug, Default, PartialEq, Eq, Hash)] struct StructExt; impl ExtDTypeVTable for StructExt { type Metadata = usize; @@ -365,7 +369,7 @@ mod tests { ExtID::new_ref("struct_ext") } - fn validate( + fn validate_dtype( &self, _options: &Self::Metadata, _storage_dtype: &DType,