[GitHub] [phoenix] dbwong commented on a change in pull request #796: PHOENIX-5066 The TimeZone is incorrectly used during writing or reading data

[GitBox <gi...@apache.org>]

dbwong commented on a change in pull request #796:
URL: https://github.com/apache/phoenix/pull/796#discussion_r488961019



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixPreparedStatement.java
##########
@@ -491,15 +517,39 @@ public void setString(int parameterIndex, String x) throws SQLException {
     @Override
     public void setTime(int parameterIndex, Time x) throws SQLException {
         if (x != null) { // Since Time is mutable, make a copy
-            x = new Time(x.getTime());
+            String tz = this.connection.getQueryServices().getProps()
+                    .get(QueryServices.DATE_FORMAT_TIMEZONE_ATTRIB, "GMT");
+            int offset = TimeZone.getTimeZone(tz).getRawOffset();
+
+            TimeZone timezone = Calendar.getInstance().getTimeZone();
+            long msFromEpochGmt = x.getTime();
+            int offsetFromUTC = timezone.getOffset(msFromEpochGmt);
+
+            Calendar gmtCal = Calendar.getInstance(TimeZone.getTimeZone(tz));
+            gmtCal.setTime(x);
+            gmtCal.add(Calendar.MILLISECOND, offsetFromUTC);
+
+            x  = new Time(gmtCal.getTimeInMillis() - offset);
         }
         setParameter(parameterIndex, x);
     }
 
     @Override
     public void setTime(int parameterIndex, Time x, Calendar cal) throws SQLException {
         if (x != null) { // Since Time is mutable, make a copy
-            x = new Time(x.getTime());
+            String tz = this.connection.getQueryServices().getProps()
+                    .get(QueryServices.DATE_FORMAT_TIMEZONE_ATTRIB, "GMT");
+            int offset = TimeZone.getTimeZone(tz).getRawOffset();
+
+            TimeZone timezone = Calendar.getInstance().getTimeZone();
+            long msFromEpochGmt = x.getTime();
+            int offsetFromUTC = timezone.getOffset(msFromEpochGmt);
+
+            Calendar gmtCal = Calendar.getInstance(TimeZone.getTimeZone(tz));

Review comment:
       Looks like similar code again thoughts on utility function?

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java
##########
@@ -20,6 +20,7 @@
 import java.io.DataInput;
 import java.io.DataOutput;
 import java.io.IOException;
+import java.sql.Connection;

Review comment:
       nit: remove

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java
##########
@@ -150,14 +153,16 @@ protected long getRoundUpAmount() {
     
     
     protected long roundTime(long time) {

Review comment:
       Can we add some comments on input/output?  This appears like you are converting an absolute time to a localTime for rounding which is correct.

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java
##########
@@ -331,11 +339,16 @@ public ReadOnlyProps getProps() {
         int maxSizeBytes = this.services.getProps().getInt(
                 QueryServices.MAX_MUTATION_SIZE_BYTES_ATTRIB,
                 QueryServicesOptions.DEFAULT_MAX_MUTATION_SIZE_BYTES);
-        String timeZoneID = this.services.getProps().get(QueryServices.DATE_FORMAT_TIMEZONE_ATTRIB,
-                DateUtil.DEFAULT_TIME_ZONE_ID);
-        Format dateFormat = DateUtil.getDateFormatter(datePattern, timeZoneID);
-        Format timeFormat = DateUtil.getDateFormatter(timePattern, timeZoneID);
-        Format timestampFormat = DateUtil.getDateFormatter(timestampPattern, timeZoneID);
+
+        String timeZoneID = this.services.getProps().get(
+                QueryServices.DATE_FORMAT_TIMEZONE_ATTRIB, DateUtil.DEFAULT_TIME_ZONE_ID);

Review comment:
       I saw the discussion that jdbc differs from phoenix on default timezone being local vs phoenix's gmt, we should likely document in the code a bit here.

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundJodaDateExpression.java
##########
@@ -48,9 +49,10 @@ public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
             }
             PDataType dataType = getDataType();
             long time = dataType.getCodec().decodeLong(ptr, children.get(0).getSortOrder());
-            DateTime dt = new DateTime(time,ISOChronology.getInstanceUTC());
+            int offset = Calendar.getInstance().getTimeZone().getRawOffset();
+            DateTime dt = new DateTime(time + offset,ISOChronology.getInstanceUTC());
             long value = roundDateTime(dt);
-            Date d = new Date(value);
+            Date d = new Date(value - offset);

Review comment:
       In RoundDate expression offset was handled inside of the rounding function.  I'd either recommend the same pattern or maybe handling this in a utility class?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org