lidavidm commented on code in PR #36519:
URL: https://github.com/apache/arrow/pull/36519#discussion_r1270858500
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java:
##########
@@ -91,14 +92,11 @@ private LocalDateTime getLocalDateTime(Calendar calendar) {
long value = holder.value;
LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value);
+ ZoneId defaultTimeZone = TimeZone.getDefault().toZoneId();
+ ZoneId sourceTimeZone = nonNull(this.timeZone) ? this.timeZone.toZoneId() :
+ nonNull(calendar) ? calendar.getTimeZone().toZoneId() :
defaultTimeZone;
Review Comment:
1) can we just compare to null explicitly, and 2) can we write this as an
if-else chain?
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java:
##########
@@ -91,14 +92,11 @@ private LocalDateTime getLocalDateTime(Calendar calendar) {
long value = holder.value;
LocalDateTime localDateTime = this.longToLocalDateTime.fromLong(value);
+ ZoneId defaultTimeZone = TimeZone.getDefault().toZoneId();
Review Comment:
I think we should avoid the default time zone here, since that will be
system dependent?
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java:
##########
@@ -173,17 +176,15 @@ public void
testShouldGetTimestampReturnValidTimestampWithCalendar() throws Exce
TimeZone timeZone = TimeZone.getTimeZone(AMERICA_SAO_PAULO);
Calendar calendar = Calendar.getInstance(timeZone);
- TimeZone timeZoneForVector = getTimeZoneForVector(vector);
+ TimeZone finalTimeZoneForResultWithoutCalendar =
ofNullable(getTimeZoneForVector(vector))
+ .orElse(TimeZone.getDefault());
Review Comment:
Hmm, we should be able to assert what time zone is returned here right? And
again, we should avoid using the system timezone for things?
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessor.java:
##########
@@ -177,7 +174,7 @@ protected static TimeZone
getTimeZoneForVector(TimeStampVector vector) {
String timezoneName = arrowType.getTimezone();
if (timezoneName == null) {
- return TimeZone.getTimeZone("UTC");
+ return null;
Review Comment:
OK. This change seems to be the right one: an Arrow timestamp has no defined
timezone if the timezone name is not set.
--
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]
