You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@calcite.apache.org by "Josh Elser (Jira)" <ji...@apache.org> on 2020/12/24 01:48:00 UTC
[jira] [Comment Edited] (CALCITE-4367) Incorrect documentation for
Avatica JSON request/response signatures
[ https://issues.apache.org/jira/browse/CALCITE-4367?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17254357#comment-17254357 ]
Josh Elser edited comment on CALCITE-4367 at 12/24/20, 1:47 AM:
----------------------------------------------------------------
{quote}
`RpcMetadata` is actually a response as opposed to a miscellaneous type per [here|https://github.com/apache/calcite-avatica/blob/4b7eee5bf430b916c7c07897b6f60d2b6b6dabb7/core/src/main/protobuf/responses.proto#L114-L116] and thus requires a {{response}} field. Note I'm not certain if this was intentional, i.e., it being a response, however it it is it seems that it should be renamed to {{RpcMetadataResponse}} for consistency.
{quote}
It's intentional that it isn't a response, as a "Response" was intended to be top-level messages which might be sent back by the Avatica server. However, it's not used by any requests, so it lives in responses.proto to avoid polluting common.proto (for no reason). Hope this makes sense. I'll update the comment to reflect this.
bq. The supplied ConnectionProperties contains an undocumented dirty field (is_dirty for protobuf).
Good catch. Another funny one -- this shouldn't ever be sent over the wire. It's just internal state used to avoid unnecessary RPCs (to sync the client's properties with the properties in the Avatica server). I'll update the docs for now, but we should circle back to avoid this getting serialized at all (for json and proto)
The rest of these were spot-on. Thanks for the easy-to-incorporate feedback, [~john.bodley@gmail.com]
was (Author: elserj):
{quote}
`RpcMetadata` is actually a response as opposed to a miscellaneous type per [here|https://github.com/apache/calcite-avatica/blob/4b7eee5bf430b916c7c07897b6f60d2b6b6dabb7/core/src/main/protobuf/responses.proto#L114-L116] and thus requires a {{response}} field. Note I'm not certain if this was intentional, i.e., it being a response, however it it is it seems that it should be renamed to {{RpcMetadataResponse}} for consistency.
It's intentional that it isn't a response, as a "Response" was intended to be top-level messages which might be sent back by the Avatica server. However, it's not used by any requests, so it lives in responses.proto to avoid polluting common.proto (for no reason). Hope this makes sense. I'll update the comment to reflect this.
bq. The supplied ConnectionProperties contains an undocumented dirty field (is_dirty for protobuf).
Good catch. Another funny one -- this shouldn't ever be sent over the wire. It's just internal state used to avoid unnecessary RPCs (to sync the client's properties with the properties in the Avatica server). I'll update the docs for now, but we should circle back to avoid this getting serialized at all (for json and proto)
The rest of these were spot-on. Thanks for the easy-to-incorporate feedback, [~john.bodley@gmail.com]
> Incorrect documentation for Avatica JSON request/response signatures
> --------------------------------------------------------------------
>
> Key: CALCITE-4367
> URL: https://issues.apache.org/jira/browse/CALCITE-4367
> Project: Calcite
> Issue Type: Bug
> Components: avatica
> Reporter: John Bodley
> Assignee: Josh Elser
> Priority: Trivial
> Time Spent: 20m
> Remaining Estimate: 0h
>
> I noticed a few inconsistencies between what is documented in the [Avatica JSON Reference|https://calcite.apache.org/avatica/docs/json_reference.html] and what the Avatica JDBC driver provides, specifically:
> # The {{DatabasePropertyRequest}} was missing the {{connection_id}} field in the example signature.
> # `RpcMetadata` is actually a response as opposed to a miscellaneous type per [here|https://github.com/apache/calcite-avatica/blob/4b7eee5bf430b916c7c07897b6f60d2b6b6dabb7/core/src/main/protobuf/responses.proto#L114-L116] and thus requires a {{response}} field. Note I'm not certain if this was intentional, i.e., it being a response, however it it is it seems that it should be renamed to {{RpcMetadataResponse}} for consistency.
> # The supplied {{ConnectionProperties}} contains an undocumented {{dirty}} field ({{is_dirty}} for protobuf).
> # For the {{SchemasRequest}} the {{catalog}} and {{schemaPattern}} are optional rather than required.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)