Some adjustments to methods on ExtDTypeRef (#6716)

## Summary

Makes some adjustments to code that slipped into the a previous PR.
Mostly related to display logic. Ideally we would make it so that all of
them don't have this `impl Display` logic but we can do that another
time.

## API Changes

Some of these methods shouldn't be public, and this reverts it.

## Testing

N/A

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

Author: connortsui20

vortex-array/public-api.lock

@@ -6612,8 +6612,6 @@ impl vortex_array::dtype::extension::ExtDTypeRef
 
 pub fn vortex_array::dtype::extension::ExtDTypeRef::display_metadata(&self) -> impl core::fmt::Display + '_
 
-pub fn vortex_array::dtype::extension::ExtDTypeRef::display_storage_value<'a>(&'a self, storage_value: &'a vortex_array::scalar::ScalarValue) -> impl core::fmt::Display + 'a
-
 pub fn vortex_array::dtype::extension::ExtDTypeRef::eq_ignore_nullability(&self, other: &Self) -> bool
 
 pub fn vortex_array::dtype::extension::ExtDTypeRef::id(&self) -> vortex_array::dtype::extension::ExtId
@@ -6626,8 +6624,6 @@ pub fn vortex_array::dtype::extension::ExtDTypeRef::serialize_metadata(&self) ->
 
 pub fn vortex_array::dtype::extension::ExtDTypeRef::storage_dtype(&self) -> &vortex_array::dtype::DType
 
-pub fn vortex_array::dtype::extension::ExtDTypeRef::validate_storage_value(&self, storage_value: &vortex_array::scalar::ScalarValue) -> vortex_error::VortexResult<()>
-
 pub fn vortex_array::dtype::extension::ExtDTypeRef::with_nullability(&self, nullability: vortex_array::dtype::Nullability) -> Self
 
 impl vortex_array::dtype::extension::ExtDTypeRef

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

@@ -67,6 +67,16 @@ impl ExtDTypeRef {
         }
     }
 
+    /// Compute equality ignoring nullability.
+    pub fn eq_ignore_nullability(&self, other: &Self) -> bool {
+        self.id() == other.id()
+            && self.0.metadata_eq(other.0.metadata_any())
+            && self
+                .storage_dtype()
+                .eq_ignore_nullability(other.storage_dtype())
+    }
+
+    // TODO(connor): We should add a different type that returns something that can be serialized.
     /// Serialize the metadata into a byte vector.
     pub fn serialize_metadata(&self) -> VortexResult<Vec<u8>> {
         self.0.metadata_serialize()
@@ -78,26 +88,18 @@ impl ExtDTypeRef {
     }
 
     /// Formats an extension scalar value using the current dtype for metadata context.
-    pub fn display_storage_value<'a>(
+    pub(crate) fn fmt_storage_value<'a>(
         &'a self,
+        f: &mut fmt::Formatter<'_>,
         storage_value: &'a ScalarValue,
-    ) -> impl fmt::Display + 'a {
-        StorageValueDisplay(&*self.0, storage_value)
+    ) -> fmt::Result {
+        self.0.value_display(f, storage_value)
     }
 
     /// Validates that the given storage scalar value is valid for this dtype.
-    pub fn validate_storage_value(&self, storage_value: &ScalarValue) -> VortexResult<()> {
+    pub(crate) fn validate_storage_value(&self, storage_value: &ScalarValue) -> VortexResult<()> {
         self.0.value_validate(storage_value)
     }
-
-    /// Compute equality ignoring nullability.
-    pub fn eq_ignore_nullability(&self, other: &Self) -> bool {
-        self.id() == other.id()
-            && self.0.metadata_eq(other.0.metadata_any())
-            && self
-                .storage_dtype()
-                .eq_ignore_nullability(other.storage_dtype())
-    }
 }
 
 /// Methods for downcasting type-erased extension dtypes.
@@ -154,28 +156,6 @@ impl ExtDTypeRef {
     }
 }
 
-impl fmt::Display for ExtDTypeRef {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let metadata = self.display_metadata().to_string();
-        if metadata.is_empty() {
-            write!(f, "{}", self.id())?;
-        } else {
-            write!(f, "{}[{}]", self.id(), metadata)?;
-        }
-        write!(f, "({})", self.storage_dtype())
-    }
-}
-
-impl fmt::Debug for ExtDTypeRef {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.debug_struct("ExtDType")
-            .field("id", &self.id())
-            .field("metadata", &MetadataDebug(&*self.0))
-            .field("storage_dtype", &self.storage_dtype())
-            .finish()
-    }
-}
-
 impl PartialEq for ExtDTypeRef {
     fn eq(&self, other: &Self) -> bool {
         self.id() == other.id()
@@ -193,28 +173,54 @@ impl Hash for ExtDTypeRef {
     }
 }
 
-// Private formatting helpers for Display and Debug impls.
-
-struct MetadataDisplay<'a>(&'a dyn DynExtDType);
-
-impl fmt::Display for MetadataDisplay<'_> {
+impl fmt::Debug for ExtDTypeRef {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.0.metadata_display(f)
+        let metadata = self.0.metadata_debug(f);
+
+        f.debug_struct("ExtDType")
+            .field("id", &self.id())
+            .field("metadata", &metadata)
+            .field("storage_dtype", &self.storage_dtype())
+            .finish()
     }
 }
 
-struct MetadataDebug<'a>(&'a dyn DynExtDType);
-
-impl fmt::Debug for MetadataDebug<'_> {
+impl fmt::Display for ExtDTypeRef {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.0.metadata_debug(f)
+        let metadata = MetadataDisplay(&*self.0).to_string();
+
+        if metadata.is_empty() {
+            write!(f, "{}", self.id())?;
+        } else {
+            write!(f, "{}[{}]", self.id(), metadata)?;
+        }
+
+        write!(f, "({})", self.storage_dtype())
     }
 }
 
-struct StorageValueDisplay<'a>(&'a dyn DynExtDType, &'a ScalarValue);
+// Private formatting helpers for Display and Debug impls.
 
-impl fmt::Display for StorageValueDisplay<'_> {
+struct MetadataDisplay<'a>(&'a dyn DynExtDType);
+impl fmt::Display for MetadataDisplay<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.0.value_display(f, self.1)
+        self.0.metadata_display(f)
     }
 }
+
+// struct PythonDisplay<'a>(&'a dyn DynExtDType);
+// impl fmt::Display for PythonDisplay<'_> {
+//     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+//         let metadata = MetadataDisplay(self.0).to_string();
+
+//         let id = self.0.id();
+//         let escaped_id = id.as_ref().escape_default();
+//         if metadata.is_empty() {
+//             write!(f, "\"{escaped_id}\"",)?;
+//         } else {
+//             write!(f, "\"{escaped_id}\"[{}]", metadata)?;
+//         }
+
+//         write!(f, "({})", self.0.storage_dtype())
+//     }
+// }

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

@@ -39,7 +39,8 @@ impl fmt::Display for ExtScalar<'_> {
         let Some(value) = self.value else {
             return write!(f, "null");
         };
-        self.ext_dtype.display_storage_value(value).fmt(f)
+
+        self.ext_dtype.fmt_storage_value(f, value)
     }
 }
 

vortex-python/src/python_repr.rs

@@ -23,6 +23,7 @@ impl PythonRepr for DType {
     }
 }
 
+// TODO(connor): We should probably just use the `Display` impl on `DType`.
 impl Display for DTypePythonRepr<'_> {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         let DTypePythonRepr(dtype) = self;