impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5539: Kudu timestamp scans wrong with -use local tz for unix ts
Date Tue, 11 Jul 2017 17:56:06 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message