[GitHub] drill pull request #1184: DRILL-6242 - Use java.sql.[Date|Time|Timestamp] cl...

[parthchandra <gi...@git.apache.org>]

Github user parthchandra commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1184#discussion_r180914676
  
    --- Diff: exec/vector/src/main/codegen/templates/FixedValueVectors.java ---
    @@ -509,15 +509,15 @@ public long getTwoAsLong(int index) {
         public ${friendlyType} getObject(int index) {
           org.joda.time.DateTime date = new org.joda.time.DateTime(get(index), org.joda.time.DateTimeZone.UTC);
           date = date.withZoneRetainFields(org.joda.time.DateTimeZone.getDefault());
    -      return date;
    +      return new java.sql.Date(date.getMillis());
    --- End diff --
    
    @jiang-wu Thanks for making these changes. Your fix is on the right track.  However, I'm not sure if we want to introduce a dependency on JDBC classes in the vectors. 
    Take a look at DateAccessor, TimeAccessor, and TimeStampAccessor. These are generated from [SqlAccessor](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/codegen/templates/SqlAccessors.java). 
    The get methods in these convert from UTC to a Local{Date|Time|TimeStamp}.  Subsequently they convert to the JDBC type since they are used by the JDBC driver.
    The vectors should be able to do the same, just returning the Local{Date|Time|TimeStamp} object.
    I'm not sure if that might affect tests that depend on timezone though. Perhaps @vdiravka can comment. 


---