GH-725: Added ExtensionReader #726
Conversation
This comment has been minimized.
This comment has been minimized.
xxlaykxx
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
|
Thanks for the PR, let me take a look. |
|
@xxlaykxx sure. I will. |
|
still need review, plz |
| </#list></#list> | ||
|
|
||
| public void read(ExtensionHolder holder) { | ||
| fail("Extension"); |
There was a problem hiding this comment.
It might be clearer to use the signature as the error message (like other methods do below)?
| public void read(ExtensionHolder holder) { | ||
| UuidHolder uuidHolder = (UuidHolder) holder; | ||
| vector.get(idx(), uuidHolder); | ||
| } |
There was a problem hiding this comment.
Why don't we need to override the other methods in AbstractFieldReader?
There was a problem hiding this comment.
read(int arrayIndex, ExtensionHolder holder), copyAsValue, copyAsField still aren't implemented?
There was a problem hiding this comment.
added read, copyAsValue. copyAsField can't find any usage for other classes where this impl exists.
| FieldReader uuidReader = rootReader.reader("uuid1"); | ||
| uuidReader.setPosition(0); | ||
| UuidHolder uuidHolder = new UuidHolder(); | ||
| uuidReader.read(uuidHolder); |
There was a problem hiding this comment.
Similarly we test read but not the other methods?
There was a problem hiding this comment.
read(int arrayIndex, ExtensionHolder holder), copyAsValue, copyAsField aren't tested?
There was a problem hiding this comment.
Added tests with read and copyAsValue usage. For copyAsField, there are no examples of how it should be used, so not sure what is the right test case for it.
|
argh...I forgot to reapprove workflows...let me fix thigns |
ExtensionReader was added to support reading extension types from a complex vector. It contains **read(ExtensionHolder)** method for reading to the holder. And **readObject** - for reading the value explicitly. Closes apache#725.
ExtensionReader was added to support reading extension types from a complex vector. It contains **read(ExtensionHolder)** method for reading to the holder. And **readObject** - for reading the value explicitly. Closes apache#725.
3 participants
What's Changed
ExtensionReader was added to support reading extension types from a complex vector.
It contains read(ExtensionHolder) method for reading to the holder. And readObject - for reading the value explicitly.
Closes #725.