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

java/client: Return JDBC-compatible types for BIT and DATE/TIME #1499

Merged
merged 3 commits into from
Feb 16, 2016

Conversation

enisoc
Copy link
Member

@enisoc enisoc commented Feb 11, 2016

@sougou

These match the types used by Connector/J, with one exception: we don't support the special case that BIT(1) is returned as Boolean instead of byte[], because that would require sending field size info along with every query result. If that turns out to be an important use case for users, we can consider workarounds like adding getBoolean() as a convenience to check the first byte.

I haven't found any internal users of the old Joda-based getDateTime(), so it should be safe to break it.

They are binary data, not Unicode strings.
@sougou
Copy link
Contributor

sougou commented Feb 11, 2016

LGTM

Approved with PullApprove

@enisoc
Copy link
Member Author

enisoc commented Feb 11, 2016

It turns out that toString() and valueOf() on java.sql.(Date|Time|Timestamp) all use the local machine's default timezone. This caused the test to fail on Travis although it passes on my machine.

Ideally we would do everything in UTC to avoid confusion, but they don't provide any parsing or formatting methods for that. I'm looking into it.

@enisoc
Copy link
Member Author

enisoc commented Feb 11, 2016

@sougou Could you take a look at the new commits?

The valueOf() methods on java.sql.(Date|Time|Timestamp) don't support
MySQL-specific syntax, and will throw exceptions while parsing certain
values returned from MySQL. The toString() methods on those classes
also may produce results that a MySQL user would not want or expect.

This new class contains utility methods for working with MySQL DATE,
TIME, TIMESTAMP, and DATETIME formats, including MySQL-specific syntax.
Unlike the built-in valueOf() and toString() methods, these also support
time zones through the Calendar class, which is necessary when implementing
the JDBC PreparedStatement and ResultSet interfaces. When no Calendar is
specified, we use the default time zone like the built-in methods do.
These are the standard types used in JDBC and Connector/J.
@enisoc
Copy link
Member Author

enisoc commented Feb 12, 2016

@sougou I think I figured it out now. The right way to do it is to offer both getDate(...) and getDate(..., Calendar) which is how it's done in both JDBC PreparedStatement (sending side) and JDBC ResultSet (receiving side). When the Calendar isn't specified, we use the user's default time zone. Otherwise, we use the given Calendar to do the conversion. This lets the user tell us, "I know this data is in time zone X, so treat it that way."

To implement this, I had to keep my new MySQL DateTime class, because java.sql.(Date|Time|Timestamp).(toString|valueOf) don't support converting in different time zones. As an added benefit of my custom converters, we support the full MySQL syntax for these values, as opposed to choking on things like fractional seconds in a TIME value, which MySQL allows as of 5.6.

@sougou
Copy link
Contributor

sougou commented Feb 16, 2016

lgtm

Approved with PullApprove

enisoc added a commit that referenced this pull request Feb 16, 2016
java/client: Return JDBC-compatible types for BIT and DATE/TIME
@enisoc enisoc merged commit 91aa6fc into vitessio:master Feb 16, 2016
@enisoc enisoc deleted the java-types branch February 16, 2016 21:33
dbussink pushed a commit that referenced this pull request Jan 30, 2023
…er, but executed via normal MySQL statements (#1499)

* OnlineDDL: 'mysql' strategy, managed by the scheduler, but executed via normal MySQL statements (#12027)

* OnlineDDL: 'mysql' strategy: managed by the scheduler, but executed via normal MySQL 'ALTER TABLE'

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* use 'mysql' strategy

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* fail mysql strategy on incompatible strategy flags

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* endtoend tests

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* release notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* Fix bad merge. (#12087)

GitHub did not seem to pick up the merge conflict between
these two commits (older to newer):
  - 673573a
  - 836b3c1

The first commit changed the function signature for
testOnlineDDLStatement(), while the later commit
used the old signature.

Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Co-authored-by: Matt Lord <mattalord@gmail.com>
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.

3 participants