You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "sdf-jkl (via GitHub)" <gi...@apache.org> on 2026/04/08 19:55:07 UTC

[PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

sdf-jkl opened a new pull request, #9677:
URL: https://github.com/apache/arrow-rs/pull/9677

   # Which issue does this PR close?
   
   <!--
   We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
   -->
   
   - Closes #8474.
   
   # Rationale for this change
   Check issue
   <!--
   Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
   Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
   -->
   
   # What changes are included in this PR?
   - Added a `has_valid_extension_type` API to `Field`
   - Added a unit test to save behavior
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   # Are these changes tested?
   - yes, unit test
   <!--
   We typically require tests for all PRs in order to:
   1. Prevent the code from being accidentally broken by subsequent changes
   2. Serve as another way to document the expected behavior of the code
   
   If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
   -->
   
   # Are there any user-facing changes?
   
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   
   If there are any breaking changes to public APIs, please call them out.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3065049237


##########
arrow-schema/src/extension/mod.rs:
##########
@@ -257,6 +257,15 @@ pub trait ExtensionType: Sized {
     /// this extension type.
     fn try_new(data_type: &DataType, metadata: Self::Metadata) -> Result<Self, ArrowError>;
 
+    /// Validate this extension type for a field with the given data type and
+    /// metadata.
+    ///
+    /// The default implementation delegates to [`Self::try_new`]. Extension
+    /// types may override this to validate without constructing `Self`.
+    fn validate(data_type: &DataType, metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self::try_new(data_type, metadata).map(|_| ())

Review Comment:
   Ack, thanks!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3064915735


##########
arrow-schema/src/field.rs:
##########
@@ -504,13 +504,39 @@ impl Field {
             .map(String::as_ref)
     }
 
+    /// Returns `true` if this [`Field`] has the given [`ExtensionType`] name
+    /// and can be successfully validated as that extension type.
+    ///
+    /// This first checks the extension type name and only calls
+    /// [`ExtensionType::validate`] when the name matches.
+    ///
+    /// This is useful when you only need a boolean validity check and do not
+    /// need to retrieve the extension type instance.
+    #[inline]
+    pub fn has_valid_extension_type<E: ExtensionType>(&self) -> bool {
+        if self.extension_type_name() != Some(E::NAME) {
+            return false;
+        }
+
+        let ext_metadata = self
+            .metadata()
+            .get(EXTENSION_TYPE_METADATA_KEY)
+            .map(|s| s.as_str());

Review Comment:
   Ah,  good point. And the two `as_str` forms are the same line length, so probably no point changing it unless you like the other form better.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#issuecomment-4209985876

   It was easy to do right away here. 
   
   Now instead of building the `ExtensionType` we only follow the simple validation steps from `ExtensionType::try_new`. 
   
   For these types (`Bool8`, `Json`, `Uuid`, `Opaque`, `TimestampWithOffset`, `VariantType`, `RowNumber`, `RowGroupIndex`) it's a simple `DataType` check.
   
   `FixedShapeTensor` and `VariableShapeTensor` don't implement `validate` yet, but they don't really need it since we don't do a boolean check for them anywhere.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3075098980


##########
arrow-schema/src/extension/mod.rs:
##########
@@ -257,6 +257,15 @@ pub trait ExtensionType: Sized {
     /// this extension type.
     fn try_new(data_type: &DataType, metadata: Self::Metadata) -> Result<Self, ArrowError>;
 
+    /// Validate this extension type for a field with the given data type and
+    /// metadata.
+    ///
+    /// The default implementation delegates to [`Self::try_new`]. Extension
+    /// types may override this to validate without constructing `Self`.
+    fn validate(data_type: &DataType, metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self::try_new(data_type, metadata).map(|_| ())

Review Comment:
   How about as a follow on we remove the default impl (and we merge it for the next major breaking API release)-- that will force any implementations to implement and avoid the potential slowdown



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3075283099


##########
arrow-schema/src/extension/mod.rs:
##########
@@ -257,6 +257,15 @@ pub trait ExtensionType: Sized {
     /// this extension type.
     fn try_new(data_type: &DataType, metadata: Self::Metadata) -> Result<Self, ArrowError>;
 
+    /// Validate this extension type for a field with the given data type and
+    /// metadata.
+    ///
+    /// The default implementation delegates to [`Self::try_new`]. Extension
+    /// types may override this to validate without constructing `Self`.
+    fn validate(data_type: &DataType, metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self::try_new(data_type, metadata).map(|_| ())

Review Comment:
   Sounds good!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3076405217


##########
arrow-schema/src/extension/mod.rs:
##########
@@ -257,6 +257,15 @@ pub trait ExtensionType: Sized {
     /// this extension type.
     fn try_new(data_type: &DataType, metadata: Self::Metadata) -> Result<Self, ArrowError>;
 
+    /// Validate this extension type for a field with the given data type and
+    /// metadata.
+    ///
+    /// The default implementation delegates to [`Self::try_new`]. Extension
+    /// types may override this to validate without constructing `Self`.
+    fn validate(data_type: &DataType, metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self::try_new(data_type, metadata).map(|_| ())

Review Comment:
   filed here:
   - https://github.com/apache/arrow-rs/issues/9708



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3058464010


##########
arrow-schema/src/field.rs:
##########
@@ -504,13 +504,39 @@ impl Field {
             .map(String::as_ref)
     }
 
+    /// Returns `true` if this [`Field`] has the given [`ExtensionType`] name
+    /// and can be successfully validated as that extension type.
+    ///
+    /// This first checks the extension type name and only calls
+    /// [`ExtensionType::validate`] when the name matches.
+    ///
+    /// This is useful when you only need a boolean validity check and do not
+    /// need to retrieve the extension type instance.
+    #[inline]
+    pub fn has_valid_extension_type<E: ExtensionType>(&self) -> bool {
+        if self.extension_type_name() != Some(E::NAME) {
+            return false;
+        }
+
+        let ext_metadata = self
+            .metadata()
+            .get(EXTENSION_TYPE_METADATA_KEY)
+            .map(|s| s.as_str());

Review Comment:
   nit?
   ```suggestion
               .as_deref();
   ```



##########
arrow-schema/src/extension/canonical/bool8.rs:
##########
@@ -68,6 +68,10 @@ impl ExtensionType for Bool8 {
     fn try_new(data_type: &DataType, _metadata: Self::Metadata) -> Result<Self, ArrowError> {
         Self.supports_data_type(data_type).map(|_| Self)
     }
+
+    fn validate(data_type: &DataType, _metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self.supports_data_type(data_type)

Review Comment:
   Ah... `Self` is an empty struct, so it has no constructor (singleton).



##########
arrow-schema/src/extension/mod.rs:
##########
@@ -257,6 +257,15 @@ pub trait ExtensionType: Sized {
     /// this extension type.
     fn try_new(data_type: &DataType, metadata: Self::Metadata) -> Result<Self, ArrowError>;
 
+    /// Validate this extension type for a field with the given data type and
+    /// metadata.
+    ///
+    /// The default implementation delegates to [`Self::try_new`]. Extension
+    /// types may override this to validate without constructing `Self`.
+    fn validate(data_type: &DataType, metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self::try_new(data_type, metadata).map(|_| ())

Review Comment:
   I fear we have an API clash here:
   * `supports_data_type` receives `&self`, in order to allow configurable extension types based on their metadata
   * `validate` receives metadata but can only use it by instantiating Self (which requires the very allocation we wanted to avoid).
   
   Ideally, `supports_data_type` should be implemented in terms of `validate` instead... but I guess that would be a breaking change?
   
   The next best would be to chase down every extension type that actually has metadata, and implement the two methods in the correct direction. 
   
   Does any impl in the arrow crate actually use this provided method? Or is it just a safety net for third party impl to avoid a breaking change?



##########
arrow-schema/src/extension/canonical/bool8.rs:
##########
@@ -68,6 +68,10 @@ impl ExtensionType for Bool8 {
     fn try_new(data_type: &DataType, _metadata: Self::Metadata) -> Result<Self, ArrowError> {
         Self.supports_data_type(data_type).map(|_| Self)
     }
+
+    fn validate(data_type: &DataType, _metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self.supports_data_type(data_type)

Review Comment:
   I didn't know this was valid syntax... but if it is, why do we need `Self::default().supports_data_type` below?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3058966904


##########
arrow-schema/src/extension/mod.rs:
##########
@@ -257,6 +257,15 @@ pub trait ExtensionType: Sized {
     /// this extension type.
     fn try_new(data_type: &DataType, metadata: Self::Metadata) -> Result<Self, ArrowError>;
 
+    /// Validate this extension type for a field with the given data type and
+    /// metadata.
+    ///
+    /// The default implementation delegates to [`Self::try_new`]. Extension
+    /// types may override this to validate without constructing `Self`.
+    fn validate(data_type: &DataType, metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self::try_new(data_type, metadata).map(|_| ())

Review Comment:
   It's just a safety net now. No extension type is using the default `validate` impl



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#discussion_r3058966904


##########
arrow-schema/src/extension/mod.rs:
##########
@@ -257,6 +257,15 @@ pub trait ExtensionType: Sized {
     /// this extension type.
     fn try_new(data_type: &DataType, metadata: Self::Metadata) -> Result<Self, ArrowError>;
 
+    /// Validate this extension type for a field with the given data type and
+    /// metadata.
+    ///
+    /// The default implementation delegates to [`Self::try_new`]. Extension
+    /// types may override this to validate without constructing `Self`.
+    fn validate(data_type: &DataType, metadata: Self::Metadata) -> Result<(), ArrowError> {
+        Self::try_new(data_type, metadata).map(|_| ())

Review Comment:
   It's just a safety net now. 



##########
arrow-schema/src/field.rs:
##########
@@ -504,13 +504,39 @@ impl Field {
             .map(String::as_ref)
     }
 
+    /// Returns `true` if this [`Field`] has the given [`ExtensionType`] name
+    /// and can be successfully validated as that extension type.
+    ///
+    /// This first checks the extension type name and only calls
+    /// [`ExtensionType::validate`] when the name matches.
+    ///
+    /// This is useful when you only need a boolean validity check and do not
+    /// need to retrieve the extension type instance.
+    #[inline]
+    pub fn has_valid_extension_type<E: ExtensionType>(&self) -> bool {
+        if self.extension_type_name() != Some(E::NAME) {
+            return false;
+        }
+
+        let ext_metadata = self
+            .metadata()
+            .get(EXTENSION_TYPE_METADATA_KEY)
+            .map(|s| s.as_str());

Review Comment:
   `.as_deref()` does `Option<String> → Option<&str>`
   
   We have `Option<&String> → Option<&str>` situation here.
   
   Could do:
   ```diff
   - .map(|s| s.as_str());
   + .map(String::as_str)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#issuecomment-4209390351

   @alamb @scovich I was looking at #8474 and the doc update in #8475.
   
   The current recommendation in #8475 is good:
   
   ```rust
   if field.extension_type_name() == Some(MyExtensionType::NAME) {
       if let Ok(extension_type) = field.try_extension_type::<MyExtensionType>() {
           // ...
       }
   }
   ```
   
   Checking the name first avoids the two name-related error paths in `try_new_from_field_metadata` (missing/mismatch).
   
   For a **full validity check**, though, we currently still have to go through `try_new_from_field_metadata`, which means:
   
   1. `deserialize_metadata(...)`
   2. `try_new(...)`
   
   Both may return `Err`, and both are part of validation today.
   
   I think a clean follow-up would be a dedicated `validate` API on `ExtensionType`, with a default implementation that simply delegates to `try_new_from_field_metadata` (or equivalent). That gives us a clearer API now without changing behavior, and leaves room for specialized implementations later if needed. For extensions that override it, this could also avoid allocation/work from constructing an `ExtensionType` when we only need validation.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#issuecomment-4209394723

   I could add `ExtensionType::validate` here


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [Arrow] Add API to check if `Field` has a valid `ExtensionType` [arrow-rs]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9677:
URL: https://github.com/apache/arrow-rs/pull/9677#issuecomment-4238815852

   Thank you @scovich and @sdf-jkl 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org