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

(10 comments)

http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exec/data-source-scan-node.cc
File be/src/exec/data-source-scan-node.cc:

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).


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

PS1, Line 354: TimestampValue
> maybe use "const TimetampValue& tv = ..." here (and elsewhere), though mayb
Done


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:

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


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/runtime/timestamp-test.cc
File be/src/runtime/timestamp-test.cc:

PS1, Line 632: coverted
> converted
Done


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 
Done


http://gerrit.cloudera.org:8080/#/c/6510/1/be/src/runtime/timestamp-value.h
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 ....
Done


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


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


PS1, Line 105: unix_time
> 'unix_time'
Done


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. literal.cc and raw-value.cc


-- 
To view, visit http://gerrit.cloudera.org:8080/6510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id25e19f7984e5ebf9073d9c569faf69cec142fa1
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