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

sql: allow Java acceptance tests to work #4496

Merged
merged 1 commit into from
Feb 18, 2016
Merged

sql: allow Java acceptance tests to work #4496

merged 1 commit into from
Feb 18, 2016

Conversation

maddyblue
Copy link
Contributor

Three changes make this possible.

  1. Accept the VARCHAR OID and treat it like TEXT.
  2. Support the limit field in execute commands as long as there aren't
    any leftover rows. If we find a driver or test case where this assumption
    doesn't hold (i.e., we will need to page through the results), we will
    add support for it later.
  3. Silently ignore the EXTRA_FLOAT_DIGITS setting, which is sent by the
    JDBC driver during connection start.

Fixes #4035

Review on Reviewable

@tamird
Copy link
Contributor

tamird commented Feb 18, 2016

LGTM


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


sql/pgwire/v3.go, line 551 [r1] (raw file):
nit: any reason not to leave this as an int32? seems desirable to push the cast to the end user


sql/pgwire/v3.go, line 629 [r1] (raw file):
s/%v/%d/


Comments from the review on Reviewable.io

Three changes make this possible.

1. Accept the VARCHAR OID and treat it like TEXT.

2. Support the limit field in execute commands as long as there aren't
any leftover rows. If we find a driver or test case where this assumption
doesn't hold (i.e., we will need to page through the results), we will
add support for it later.

3. Silently ignore the EXTRA_FLOAT_DIGITS setting, which is sent by the
JDBC driver during connection start.

Fixes #4035
maddyblue added a commit that referenced this pull request Feb 18, 2016
sql: allow Java acceptance tests to work
@maddyblue maddyblue merged commit 35c75ff into cockroachdb:master Feb 18, 2016
@maddyblue maddyblue deleted the sql-java-acceptance branch February 18, 2016 22:35
@tamird
Copy link
Contributor

tamird commented Feb 18, 2016

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


sql/pgwire/v3.go, line 617 [r2] (raw file):
i was thinking this'd be int32


Comments from the review on Reviewable.io

@maddyblue
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 3 unresolved discussions.


sql/pgwire/v3.go, line 551 [r1] (raw file):
Done.


sql/pgwire/v3.go, line 629 [r1] (raw file):
Done.


sql/pgwire/v3.go, line 617 [r2] (raw file):
Ah. I wanted to avoid that because it has a silly cast in the logic. Opened #4518 if you'd prefer it the way you say, though.


Comments from the review on Reviewable.io

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.

2 participants