impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5137: pt1, Refactor TimestampValue constructors
Date Thu, 30 Mar 2017 03:01:00 GMT
Dan Hecht 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? in any case, okay
to leave this change as purely non-functional and we can deal with this later.
File be/src/exprs/

PS1, Line 354: TimestampValue
maybe use "const TimetampValue& tv = ..." here (and elsewhere), though maybe unnecessary
if the compiler is able to optimize away the copy anyway.
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() (or maybe rename
DebugString() to ToString())?
File be/src/runtime/

PS1, Line 619: 

PS1, Line 632: coverted

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 really the same.
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

PS1, Line 287: operator
do we need this other than that lexical_cast I commented on?

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-HasComments: Yes

View raw message