-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Arrow] Add API to check if Field has a valid ExtensionType
#9677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(|_| ()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fear we have an API clash here:
Ideally, 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just a safety net now. No extension type is using the default
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack, thanks!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| /// Construct this extension type from field metadata and data type. | ||
| /// | ||
| /// This is a provided method that extracts extension type information from | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.