[Arrow] Add API to check if Field has a valid ExtensionType#9677
[Arrow] Add API to check if Field has a valid ExtensionType#9677sdf-jkl wants to merge 4 commits intoapache:mainfrom
Field has a valid ExtensionType#9677Conversation
|
@alamb @scovich I was looking at #8474 and the doc update in #8475. The current recommendation in #8475 is good: 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 For a full validity check, though, we currently still have to go through
Both may return I think a clean follow-up would be a dedicated |
|
I could add |
|
It was easy to do right away here. Now instead of building the For these types (
|
scovich
left a comment
There was a problem hiding this comment.
Generally LGTM, but one design question.
| /// 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(|_| ()) |
There was a problem hiding this comment.
I fear we have an API clash here:
supports_data_typereceives&self, in order to allow configurable extension types based on their metadatavalidatereceives 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?
There was a problem hiding this comment.
It's just a safety net now. No extension type is using the default validate impl
| let ext_metadata = self | ||
| .metadata() | ||
| .get(EXTENSION_TYPE_METADATA_KEY) | ||
| .map(|s| s.as_str()); |
There was a problem hiding this comment.
nit?
| .map(|s| s.as_str()); | |
| .as_deref(); |
There was a problem hiding this comment.
.as_deref() does Option<String> → Option<&str>
We have Option<&String> → Option<&str> situation here.
Could do:
- .map(|s| s.as_str());
+ .map(String::as_str)
Which issue does this PR close?
Fieldhas an extension type #8474.Rationale for this change
Check issue
What changes are included in this PR?
has_valid_extension_typeAPI toFieldAre these changes tested?
Are there any user-facing changes?