Dan Hecht has posted comments on this change. Change subject: IMPALA-5539: Kudu timestamp scans wrong with -use_local_tz_for_unix_ts ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7311/1//COMMIT_MSG Commit Message: PS1, Line 22: While an argument could be made for converting to/from local : time in all places, the simpler approach is to treat : everything as UTC consistently. hmm, not sure why we should ever have this flag affect Kudu scans. http://gerrit.cloudera.org:8080/#/c/7311/1/be/src/exprs/timestamp-functions.h File be/src/exprs/timestamp-functions.h: Line 96: // Functions to convert to and from TimestampVal type what timezone are these returned in? PS1, Line 124: UtcTimestampFromUnixMicros How about just UtcFromUnixMicros() to better match UtcToUnixMicros(). The "timestamp" part can be inferred by UTC and the return type. Actually, though, aren't these analogous to "ToTimestamp"? the naming of all these routines is becoming really hard to follow. I think it would be best if we could pick a convention and be consistent. http://gerrit.cloudera.org:8080/#/c/7311/1/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: PS1, Line 108: . explain the argument, like FromUnixTimeMicros comment. http://gerrit.cloudera.org:8080/#/c/7311/1/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: PS1, Line 213: utc_timestamp_from_unix_micros is this exposed for end users or just for the frontend? if the latter, can we remove timestamp_from_unix_micros? if the former, do we have proper testing for timestamp_from_unix_micros? -- To view, visit http://gerrit.cloudera.org:8080/7311 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I423a810427353be76aa64442044133a9a22cdc9b Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
