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

Support PreparedStatement stream parameters #208

Merged
merged 2 commits into from
Aug 30, 2019

Conversation

nicktorwald
Copy link

Add capability to set streams as a parameter of ASCII or UNICODE text as
well as SCALAR binary data. Tarantool does not support a data streaming
via ip-proto so it leads the driver materializes the streams in proper
data in advance and sends prepared data as ordinary text or raw bytes.

Closes: #190

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

Nit: ip-proto → iproto (in the commit message).

My main concern is that we can avoid materializing streams in memory. The API support is more important however, so if it is a significant amount of work, then skip it for now and file an issue re optimizatimizing this moment.

Also please consider UTF-8 / UTF-16 question.

@nicktorwald nicktorwald force-pushed the nicktorwald/gh-190-prepared-statement-stream branch 2 times, most recently from f02c9b2 to 79db6a5 Compare August 15, 2019 10:43
@Totktonada
Copy link
Member

LGTM. Just several notes on wording.

Re 'Revert changes made in d4d62e9' commit:

It is better to do so using git revert command and add your description below to the commit message. Commit hashes, issues numbers are not subject to placing into a commit header, because it should give a reader an idea what the commit is about.

Re 'Support PreparedStatement stream parameters' commit:

Typo: IPPROTO → iproto.

I would not say that the protocol does not support streaming. At least if you know a size of your data you can calculate a packet size before encode it, send a packet header and then do a bunch of writes to a socket. When a size of data is known it is quite easy to calculate msgpack size. However you are right in the case when a size of data is not known: a client should materialize it or calculate a size of data somehow before start sending.

I think it worth to formulate it more accurate in the commit message.

Add capability to set streams as a parameter of ASCII or UNICODE text as
well as SCALAR binary data. The input streams often are size unaware so
it can be difficult to calculate the data size and, consequently, the
iproto packet size. It leads the driver materializes the streams in
proper data in advance and sends prepared the data as an ordinary text
or raw bytes. Follow API is more important thing now than performance
issues with streams.

Closes: #190
This reverts commit d4d62e9.

To be consistent with PreparedStatement.setUnicodeStream which provides
UTF-8 encoding, ResultSet.getUnicodeStream has to also use the same
encoding. Changes in d4d62e9 were made under the influence of JDBC API
for getUnicodeStream that says 'a stream of two-byte 3 characters. The
first byte is the high byte; the second byte is the low byte'.
@nicktorwald nicktorwald force-pushed the nicktorwald/gh-190-prepared-statement-stream branch from 79db6a5 to 2231ecf Compare August 22, 2019 17:51
@Totktonada
Copy link
Member

Nice. It is okay. Let's proceed.

@nicktorwald nicktorwald merged commit defc3de into master Aug 30, 2019
@nicktorwald nicktorwald deleted the nicktorwald/gh-190-prepared-statement-stream branch August 30, 2019 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jdbc: Support PreparedStatement.set*Stream
2 participants