[
https://issues.apache.org/jira/browse/CALCITE-840?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14708493#comment-14708493
]
Josh Elser commented on CALCITE-840:
------------------------------------
Big thank you for taking a look, Julian. Much appreciated!
bq. I was surprised how large the patch is - but not your fault at all, I guess
it's quite a big protocol. Nor is its protobuf's fault - everything seems to
translate quite naturally.
Heh, yeah, things exploded a bit trying to unwrap Frame :). Which is also one
bit that translated very poorly due to its {{List<Object>}}. If you look
carefully here (and I think there was another place), I still use the Jackson
serialization to just put JSON into a protocol buffer because I had no
reasonable means to say what these {{Object}}s actually were. I think this
could be improved but is perhaps not necessary for a first pass (I assume
anyone leverage the protobuf endpoint has a reasonable JSON parser which can
unwrap what the server sent it)
bq. In a few places you have default constructors that assign null to final
members of type Map or List and I presume that protobuf uses Unsafe to
re-assign them. Is it valid for users of those classes to assume that the Map
and List members are not null?
Yeah, I need to revisit these. I was trying to leverage a visitor pattern as
much as possible to prevent the massive if/elseif blocks. This was one
unintended consequence of it. I'm not 100% happy with it, but I'm not sure of a
better fix at the moment. Will hopefully go away before a final patch.
bq. And it would be good to fix CALCITE-839 before committing this. It seems
wrong that any file would have both jackson and protobuf imports, and 839 would
concentrate the jackson imports in a smaller number of files.
Agreed.
bq. Please be sure to use 3rd person declarative for javadoc method comments.
Ok.
bq. It should be possible for some of the tests to be re-used between Protobuf
and Jackson. Tests of the end-to-end service call variety. Such tests probably
already exist. (There are thread-safety issues, see CALCITE-687, but I believe
the tests are basically sound.)
This was my hope as well. I've been working through tests for protobuf from
low->high with the hopes that when I get to a high-level query, I can just
replace the client and the Handler in the server and I can reuse the same test.
> Protobuf transport for Avatica
> ------------------------------
>
> Key: CALCITE-840
> URL: https://issues.apache.org/jira/browse/CALCITE-840
> Project: Calcite
> Issue Type: Bug
> Components: avatica
> Reporter: Julian Hyde
> Assignee: Josh Elser
> Attachments: CALCITE-840.001.patch
>
>
> Create a transport for Avatica that uses Protobuf.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
