-
Notifications
You must be signed in to change notification settings - Fork 993
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
PHOENIX-6881 Implement the applicable Date/Time features from JDBC 4.2 #1571
Conversation
829b56a
to
e2efd2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
try { | ||
rs.getObject(2, Long.class); | ||
fail("We only implement byte[]"); | ||
} catch (SQLException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could assert on the errorCode or the error message containing Type mismatch when catching these expected Exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
if (x != null) { | ||
if (connection.isApplyTimeZoneDisplacement()) { | ||
x = DateUtil.applyInputDisplacement(x, cal.getTimeZone()); | ||
return DateUtil.applyInputDisplacement(x, cal.getTimeZone()); | ||
} else { | ||
int nanos = x.getNanos(); | ||
x = new Timestamp(x.getTime()); | ||
x.setNanos(nanos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit inconsistent to the other process functions, but I see the reason why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, handling nanos is awkward with java.sql.Timestamp.
public Object toObject(byte[] bytes, int offset, int length, PDataType actualType, | ||
SortOrder sortOrder, Integer maxLength, Integer scale, Class jdbcType) | ||
throws SQLException { | ||
// FIXME according to the JDBC spec, we should support all these types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please create a follow up ticket for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened PHOENIX-6915
Thank you, I have implemented your suggestion in the test before commit. |
No description provided.