Bryan, Sorry for the delay. I did a round of review of the API change (high level).
I understand the proposed API changes allows users of the Arrow Java library to implement Arrow reader that reads Arrow data to on-heap memory directly. However, my main feedback is that the proposed API changes introduced a new style of read API - instead of return the Arrow message and data directly from the read method, there are new APIs introduced to read the message into a "holder" object. I am a little concerned about the complexity of maintaining both style APIs. There is another concern about memory ownership, which I have replied in more details in the PR - the buffer allocator is moved into the message reader class itself, instead of being provided by the caller - this could cause some confusion about memory ownership. That being said, I feel I am not 100% confident to say +1, +0 or -1 to the proposed change alone and I request some other Java committer review this change with me together. Thank you, Li On Mon, Jul 9, 2018 at 11:50 PM, Li Jin <ice.xell...@gmail.com> wrote: > Bryan, > > Sorry I am traveling now but I will try to take to look in the next few > days. > > Li > > On Mon, Jul 9, 2018 at 11:16 PM, Bryan Cutler <cutl...@gmail.com> wrote: > >> Hi All, >> >> I'm proposing some Java API changes to MessageReader, with minor changes >> to >> ArrowStreamReader and MessageSerializer, as part of ARROW-2704 [1] and can >> be seen in the PR [2]. These changes are to improve processing an Arrow >> stream on a per Message basis. >> >> A while ago I introduced the MessageReader interface in anticipation of >> some future work, but it fell a little short of what I needed. So after >> tweaking the APIs a bit, it can now allow the user to implement message >> stream processing without knowing specifics about the stream format and in >> an optimal way that avoids unnecessary buffer copying. If you have some >> concerns about these APIs, please discuss in the PR at [2]. If no one >> objects, it would be great to get these changes in version 0.10.0. >> Thanks! >> >> Bryan >> >> >> [1]: https://issues.apache.org/jira/browse/ARROW-2704 >> [2]: https://github.com/apache/arrow/pull/2139 >> > >
