Dan Hecht has posted comments on this change. Change subject: IMPALA-3504: UDF for current timestamp in UTC ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/7203/3/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 343: /// Use pointer to avoid inclusion of timestampvalue.h and avoid clang issues. Given the names are so different that it's not obvious (yet the names are okay since they are consistent with the built-ins they implement), I think it would help to document that these are the same point in time but that now is in local time and utc_timestamp_ is in UTC. http://gerrit.cloudera.org:8080/#/c/7203/3/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: Line 124: } not your change and you shouldn't fix it in this commit, but all remaining uses of LocalTime() seem wrong. We should't be using TimestampValue as a general timer mechanism -- it should just be used for the TIMESTAMP datatype (and additionally some of the uses don't make much sense in local time). http://gerrit.cloudera.org:8080/#/c/7203/3/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 371: // String containing a timestamp (in UTC) set at the query submission time. document same point in time as 'now_string' -- To view, visit http://gerrit.cloudera.org:8080/7203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I969fc805922f2bb9c8101e84f85ff2cc3b1b6729 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
