GH-1109: Fix ComplexCopier for extension-type holder readers#1137
Open
bodduv wants to merge 1 commit intoapache:mainfrom
Open
GH-1109: Fix ComplexCopier for extension-type holder readers#1137bodduv wants to merge 1 commit intoapache:mainfrom
bodduv wants to merge 1 commit intoapache:mainfrom
Conversation
|
Thank you for opening a pull request! Please label the PR with one or more of:
Also, add the 'breaking-change' label if appropriate. See CONTRIBUTING.md for details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
ComplexCopier.copydoes a deep-copy using aFieldReaderfrom the source and aFieldWriterto write to the destination. On the read side in theEXTENSIONTYPEbranch, it usesreader.getField().getType()to get theArrowTypeit needs to forward towriter.writeExtension(value, type). Vector-backed extension readers (UuidReaderImpl, VariantReaderImpl) override getField(), so this works. Holder readers (NullableUuidHolderReaderImpl, NullableVariantHolderReaderImpl) on the other hand do not.AbstractFieldReader.getFieldthrows viafail("getField"), so any ComplexCopier.copy call with a holder reader as input fails.As #1109 points out, overriding getField() on a holder reader is a simple fix but a holder does not have a notion of
Field, the same way primitive (as opposed to extension) holder readers also have no notion ofgetField()and the method is not overriden.What's Changed
ComplexCopieronly needs theArrowTypehere, not the whole Field. So this PR adds a narrower accessor for that:on
ExtensionReader. The default delegates togetField(), so every existing vector-backed extension reader keeps working unchanged. Holder readers overridegetExtensionType()to returnholder.type()— theExtensionHolder.type()method added in #892 already returns the right ArrowType (UuidType.INSTANCE, VariantType.INSTANCE).ComplexCopier call
reader.getExtensionType()instead ofreader.getField().getType(). The new method goes on ExtensionReader rather than AbstractFieldReader to sit next to the other extension-specific methods already there (read(ExtensionHolder), readObject(), isSet()). Adding a default method is source- and binary-compatible.Closes #1109 .