lidavidm commented on code in PR #36519:
URL: https://github.com/apache/arrow/pull/36519#discussion_r1275338545
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java:
##########
@@ -91,14 +91,18 @@ private LocalDateTime getLocalDateTime(Calendar calendar) {
long value = holder.value;
LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value);
-
- if (calendar != null) {
- TimeZone timeZone = calendar.getTimeZone();
- long millis = this.timeUnit.toMillis(value);
- localDateTime = localDateTime
- .minus(timeZone.getOffset(millis) - this.timeZone.getOffset(millis),
ChronoUnit.MILLIS);
+ ZoneId defaultTimeZone = Calendar.getInstance().getTimeZone().toZoneId();
+ ZoneId sourceTimeZone;
+
+ if (this.timeZone != null) {
+ sourceTimeZone = this.timeZone.toZoneId();
+ } else if (calendar != null) {
+ sourceTimeZone = calendar.getTimeZone().toZoneId();
+ } else {
+ sourceTimeZone = defaultTimeZone;
}
- return localDateTime;
+
+ return
localDateTime.atZone(sourceTimeZone).withZoneSameInstant(defaultTimeZone).toLocalDateTime();
Review Comment:
I'm not sure this is quite right: if the Arrow vector has a timezone, the
underlying timestamp is always relative to UTC. `this.timeZone` doesn't reflect
that.
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java:
##########
@@ -36,21 +38,17 @@ private DateTimeUtils() {
}
/**
- * Subtracts given Calendar's TimeZone offset from epoch milliseconds.
+ * Apply calendar timezone to epoch milliseconds.
*/
public static long applyCalendarOffset(long milliseconds, Calendar calendar)
{
if (calendar == null) {
calendar = Calendar.getInstance();
}
-
- final TimeZone tz = calendar.getTimeZone();
- final TimeZone defaultTz = TimeZone.getDefault();
-
- if (tz != defaultTz) {
- milliseconds -= tz.getOffset(milliseconds) -
defaultTz.getOffset(milliseconds);
- }
-
- return milliseconds;
+ Instant currInstant = Instant.ofEpochMilli(milliseconds);
+ LocalDateTime getTimestampWithoutTZ = LocalDateTime.ofInstant(currInstant,
+ TimeZone.getTimeZone("UTC").toZoneId());
+ ZonedDateTime parsedTime =
getTimestampWithoutTZ.atZone(calendar.getTimeZone().toZoneId());
+ return parsedTime.toEpochSecond() * 1000;
Review Comment:
This appears to truncate the milliseconds?
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java:
##########
@@ -36,21 +38,17 @@ private DateTimeUtils() {
}
/**
- * Subtracts given Calendar's TimeZone offset from epoch milliseconds.
+ * Apply calendar timezone to epoch milliseconds.
*/
public static long applyCalendarOffset(long milliseconds, Calendar calendar)
{
if (calendar == null) {
calendar = Calendar.getInstance();
}
-
- final TimeZone tz = calendar.getTimeZone();
- final TimeZone defaultTz = TimeZone.getDefault();
-
- if (tz != defaultTz) {
- milliseconds -= tz.getOffset(milliseconds) -
defaultTz.getOffset(milliseconds);
- }
-
- return milliseconds;
+ Instant currInstant = Instant.ofEpochMilli(milliseconds);
+ LocalDateTime getTimestampWithoutTZ = LocalDateTime.ofInstant(currInstant,
+ TimeZone.getTimeZone("UTC").toZoneId());
Review Comment:
Can we directly use
[ofEpochSecond](https://docs.oracle.com/javase/8/docs/api/java/time/LocalDateTime.html#ofEpochSecond-long-int-java.time.ZoneOffset-)
instead of bouncing through an Instant and a timezone conversion?
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java:
##########
@@ -36,21 +38,17 @@ private DateTimeUtils() {
}
/**
- * Subtracts given Calendar's TimeZone offset from epoch milliseconds.
+ * Apply calendar timezone to epoch milliseconds.
Review Comment:
Can we document this function and what steps it's doing in detail?
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/DateTimeUtils.java:
##########
@@ -36,21 +38,17 @@ private DateTimeUtils() {
}
/**
- * Subtracts given Calendar's TimeZone offset from epoch milliseconds.
+ * Apply calendar timezone to epoch milliseconds.
*/
public static long applyCalendarOffset(long milliseconds, Calendar calendar)
{
Review Comment:
Hmm, looking at how it's used, now I'm thinking both implementations are
wrong. For instance, it's used in DateVectorAccessor and is applied to the
value from the vector and the user-supplied calendar. This function appears to
_assume the given milliseconds value is a naive timestamp in the given (or
default) timezone_, and converts it to a UTC timestamp. **This is backwards.**
The value from the Arrow vector is ALREADY a UTC timestamp!
That said, this function does seem to do the right thing for _other_ places
where it is used. So I think we need to name and properly document it, and
remove it from places where it should NOT be applied.
--
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]
