impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors
Date Fri, 31 Mar 2017 00:42:09 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5137: pt1, Refactor TimestampValue constructors

Patch Set 1:

File be/src/exec/

PS1, Line 284: FromUnixTimeNanos
> does it make sense that this conversion is subjected to use local tz flag? 
Hm I'm not sure, though if so I'll file a separate JIRA so it can be tracked externally as
this would be a breaking change (which probably affects nearly nobody).
File be/src/exprs/

PS1, Line 354: TimestampValue
> maybe use "const TimetampValue& tv = ..." here (and elsewhere), though mayb
File be/src/exprs/

PS1, Line 81: lexical_cast<string>(t)
> how about getting rid of this implicit code as well by defining a ToString(
yeah this is weird, agreed
File be/src/runtime/

PS1, Line 632: coverted
> converted

PS1, Line 641:   EXPECT_TRUE(leap_tv1.ToUnixTime(&leap_time_t));
             :   EXPECT_EQ(915148800, leap_time_t);
             :   // Converted to the Unix time representation
             :   EXPECT_EQ("1999-01-01 00:00:00", leap_tv1.DebugString());
> maybe do these cases on leap_ptime2 as well, just to be sure everything is 
File be/src/runtime/timestamp-value.h:

PS1, Line 91: Conversion to local time will be done if
> Return the corresponding timestamp in the local timezone if ....

PS1, Line 92: .
> Otherwise, return the corresponding timestamp in UTC.

PS1, Line 97: 'FromUnixTime'
> For functions, we usually write: FromUnixTime()

PS1, Line 105: unix_time
> 'unix_time'

PS1, Line 287: operator
> do we need this other than that lexical_cast I commented on?
I think we could work around it, but this does get used in a few places where we branch on
the type and directly use this operator, e.g. and

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message