GH-725: Added ExtensionReader #726
Conversation
This comment has been minimized.
This comment has been minimized.
xxlaykxx
requested review from
lidavidm,
laurentgo,
wgtmac and
jbonofre
as code owners
|
Thanks for the PR, let me take a look. |
|
@xxlaykxx sure. I will. |
|
still need review, plz |
| @@ -108,6 +108,23 @@ public void copyAsField(String name, ${name}Writer writer) { | |||
| } | |||
|
|
|||
| </#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.
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.