Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBOR support for parameters #175

Merged
merged 3 commits into from
Aug 28, 2019
Merged

Conversation

bpintea
Copy link
Collaborator

@bpintea bpintea commented Aug 21, 2019

This PR adds the support for sending the parameters encapsulated as
CBOR.

The implementation makes use of the already available parameter conversion
functionality, which prepares the parameter values as strings.
Consequently, except NULL values (and type), the values will be sent as
strings and have the server do a final conversion to the signaled SQL
type (which is actually also the case with JSON).

This commit adds the support for sending the paramters encapsulated as
CBOR.

The implementation makes use of the already available paramter conversion
functionality, which prepares the parameters as string values.
Consequently, except NULL values (and type), the values will be sent as
strings and have the server do a final conversion to the signaled SQL
type (which is actually also the case with JSON).
@bpintea bpintea added >feature Applicable to PRs adding new functionality v8.0.0-alpha1 v7.4.0 labels Aug 21, 2019
@bpintea bpintea added the WIP label Aug 23, 2019
- for SQL_VARCHAR targets, the JSON parameter serialization adds the
quotes, which need to be stripped before CBOR-packing the result.

This commit also change the integer types of two vars, to avoid
signed/unsigned comparision warnings.
@bpintea bpintea removed the WIP label Aug 27, 2019
@bpintea bpintea mentioned this pull request Aug 27, 2019
5 tasks
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

driver/queries.c Outdated
* but: (1) it's a simplified and tested implementation; (2) the overall
* performance impact is negligible with this driver's currently intended
* usage pattern (SELECTs only, fetching data volume far outweighing
* query's); (3) the server will convert the received value according to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "querys" should not have an apostrophe

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, addressed.

- correct comment phrasing.
@bpintea bpintea merged commit 758b8e2 into elastic:master Aug 28, 2019
@bpintea bpintea deleted the feat/pack_cbor_params branch August 28, 2019 15:24
bpintea added a commit that referenced this pull request Aug 28, 2019
* CBOR support for parameters

This commit adds the support for sending the paramters encapsulated as
CBOR.

The implementation makes use of the already available paramter conversion
functionality, which prepares the parameters as string values.
Consequently, except NULL values (and type), the values will be sent as
strings and have the server do a final conversion to the signaled SQL
type (which is actually also the case with JSON).

* split CBOR param value encoding by meta type

- for SQL_VARCHAR targets, the JSON parameter serialization adds the
quotes, which need to be stripped before CBOR-packing the result.

This commit also change the integer types of two vars, to avoid
signed/unsigned comparision warnings.

* address PR review note

- correct comment phrasing.

(cherry picked from commit 758b8e2)
@bpintea bpintea mentioned this pull request Sep 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature Applicable to PRs adding new functionality v7.4.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants