Matthew Jacobs 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. I agree it shouldn't, I can remove this if you think it's confusing. 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? the first is a unix time to timestamp conversion, so if FLAGS_use_local_tz_for_unix_ts_conversions is true, then the unix time is converted to local time. the second and third do not involve unix times and thus involve no timezone conversions. I have a separate patch where I'm tracking doc updates, I'll improve the comments for these methods separately. PS1, Line 124: UtcTimestampFromUnixMicros > How about just UtcFromUnixMicros() to better match UtcToUnixMicros(). The You're right that this is similar to the ToTimestamp(BigIntVal) method. This method differs in that it returns UTC and the input is in microseconds. A follow up patch will attempt to consolidate the naming, where I think the naming pattern for conversion fns should be: [SrcTz][SrcType[precision]]To[DstTz][DstType[precision]] I wrote a longer comment in IMPALA-5609. For now, what that means for this fn is that I'll add this as UnixMicrosToUtcTimestamp() ToTimestamp(BigIntVal) would later be renamed UnixToTimestamp() I'll also attempt to group/sort the fns better for clarity. 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. Done 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 Yes this is, because this gets printed in things like "show create table" output. That said, I think we can still consider removing the previous fn since it was never documented. We can always add it back later if that turns out to be useful. I'll add some tests for the new fn, thanks for the reminder. -- 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-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
