-
Notifications
You must be signed in to change notification settings - Fork 171
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
SNOW-928973: Date field is returning one day less when getting through getString method #1692
SNOW-928973: Date field is returning one day less when getting through getString method #1692
Conversation
src/main/java/net/snowflake/client/core/json/StringConverter.java
Outdated
Show resolved
Hide resolved
…h getDate, getString, getObject; set default tz based on JDBC_USE_SESSION_TIMEZONE; add tests
assertThat(((Date) obj).getTime(), is(((Date) utcObj).getTime())); | ||
break; | ||
default: | ||
fail("Unknown timezone"); |
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.
do we really want to fail for other timezones? we should handle this case
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.
My intention was if other timezones are added to test, this should be updated. But I can instead just change it to write a comment to console
TimeZone.getTimeZone(System.getProperty("user.timezone"))); | ||
|
||
String tz = System.getProperty("user.timezone"); | ||
switch (System.getProperty("user.timezone")) { |
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.
instead of using the system property in the unit tests we can pass the value here and write several test cases with different timezones given as strings
@@ -86,7 +98,7 @@ public BigDecimal toBigDecimal(int index) { | |||
|
|||
@Override | |||
public Timestamp toTimestamp(int index, TimeZone tz) throws SFException { | |||
Date date = toDate(index, tz, true); | |||
Date date = toDate(index, tz, this.useDateFormat); |
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.
when we use the field here instead of the const it could cause the breaking change for the users - I think that we should minimize the impact on the users and add the session property to keep the previous behaviour for the users in the changed methods
PR moved to 1715 |
SNOW-928973: Date field is returning one day less when getting through getString method
Fixes date field issue by setting timezone to UTC when converting from date to string and objects. For 761