zeroshade commented on PR #34631: URL: https://github.com/apache/arrow/pull/34631#issuecomment-1505724165
@disq Ah that explains it! We're basically decoding the type twice in the current implementation: Over [here](https://github.com/apache/arrow/pull/34631/files#diff-0ca8ee00b3bab4abf032cc92b9053fc94f528c103fdb05caabffe58a8e849152R960): we're grabbing the origin type that is an extension type and we *add* the extension type metadata to the field. Then over [here](https://github.com/apache/arrow/pull/34631/files#diff-0ca8ee00b3bab4abf032cc92b9053fc94f528c103fdb05caabffe58a8e849152R1070): we look for that metadata and recreate the extension type by calling deserialize, but we use `inferred.Field.Type`, which we just set equal to the extension type up above. Thinking about this a bit more, I realize that the only way you end up in the initial case to begin with is if we already successfully deserialized the extension type when we called `flight.DeserializeSchema` in the first place (inside of `getOriginSchema`). So, we actually don't need the check for the metadata and manual call to `Deserialize` in `applyOriginalStorageMetadata`. We can just do a quick check that the storage type of `extType` matches `inferred.Field.Type` and then we're all good. I've created a commit for the PR which removes the need for storageTypeEquals and adds a test to ensure that if the extension type is *not* currently registered, we get the storage type + extension metadata when we convert back to an Arrow Schema. But I think you need to check the box to allow maintainers to push to your fork. Alternately, I could just share the diff with you and you can add it yourself. let me know what you think -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
