wgtmac commented on code in PR #35139:
URL: https://github.com/apache/arrow/pull/35139#discussion_r1167963397
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/text/ArrowFlightJdbcVarCharVectorAccessorTest.java:
##########
@@ -510,14 +510,23 @@ public void
testShouldGetDateReturnValidDateWithCalendar() throws Exception {
Text value = new Text("2021-07-02");
when(getter.get(0)).thenReturn(value.copyBytes());
- Calendar calendar =
Calendar.getInstance(TimeZone.getTimeZone("America/Sao_Paulo"));
- Date result = accessor.getDate(calendar);
+ {
+ Calendar calendar =
Calendar.getInstance(TimeZone.getTimeZone("America/Sao_Paulo"));
+ Date result = accessor.getDate(calendar);
+ calendar.setTime(result);
- calendar = Calendar.getInstance(TimeZone.getTimeZone("Etc/UTC"));
- calendar.setTime(result);
+ collector.checkThat(dateTimeFormat.format(calendar.getTime()),
+ equalTo("2021-07-01T21:00:00.000Z"));
Review Comment:
The `accessor.getDate(calendar)` above is to implement
`ResultSet.getDate(int, Calendar)` and returns a `java.sq.Date`. Given two
facts:
- `java.sql.Date`: A milliseconds value represents the number of
milliseconds that have passed since January 1, 1970 00:00:00.000 GMT. To
conform with the definition of SQL DATE, the millisecond values must be
'normalized' by setting the hours, minutes, seconds, and milliseconds to zero
in the particular time zone with which the instance is associated.
- `ResultSet.getDate(int, Calendar)`: Retrieves the value of the designated
column in the current row of this ResultSet object as a java.sql.Date object in
the Java programming language. This method uses the given calendar to construct
an appropriate millisecond value for the date if the underlying database does
not store timezone information.
Therefore I agree with you that we should assume the underlying values we
are reading do not have timezone information and the behavior would be much
easier to understand if we simply attach the user-supplied timezone to the
values to return.
The original buggy code that this PR tries to solve actually is not here.
The modification here was a result of that fix. Please see my other comments
above. Let me double check if there is something wrong.
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java:
##########
@@ -96,7 +96,7 @@ private LocalDateTime getLocalDateTime(Calendar calendar) {
TimeZone timeZone = calendar.getTimeZone();
long millis = this.timeUnit.toMillis(value);
localDateTime = localDateTime
- .minus(timeZone.getOffset(millis) - this.timeZone.getOffset(millis),
ChronoUnit.MILLIS);
+ .plus(timeZone.getOffset(millis) - this.timeZone.getOffset(millis),
ChronoUnit.MILLIS);
Review Comment:
The original buggy code I want to solve is here. It tries to convert local
date time from vector timezone to user-supplied timezone. We should add the
timezone offset but not subtract it. A number of test cases need to be fixed as
their expected values are computed with the same issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
