impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue
Date Tue, 19 Sep 2017 19:54:55 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
......................................................................


Patch Set 3:

(5 comments)

> (15 comments)
 > 
 > > (15 comments)
 > >
 > > Do you plan to take care of the other cases noted in the jira?
 > > Okay to do it in a follow on commit but wondering the plan.
 > >
 > Other cases, such as use of TimestampValue as a class member (see
 > ImpalaServer, for instance where it is used to track start and end
 > times of queries), need more analysis. The plan is to look at these
 > other cases in a separate commit.
 > 

Sounds good. Let's be sure to not resolve the jira until all the cases it lists are handled
(or file a new jira to handle those).

 > > A unit test would be good for testing this kind of code.  You can
 > > take a look at the various *-test.cc files be/src/*/ for
 > examples.
 > 
 > Adding a unit test. Will refresh the patch again once I have it. In
 > the meantime, please let me know if I have addressed all of your
 > comments on the implementation.

Just a few more minor comments. I'll take another look after the unit test is added.

http://gerrit.cloudera.org:8080/#/c/8084/3/be/src/util/time.cc
File be/src/util/time.cc:

PS3, Line 33: time zone specified by the boolean argument 'utc'
since 'utc' doesn't tell you the other time zone, how about:

... in UTC if 'utc' is true, or the local time zone if 'utc' is false.

or similar.


PS3, Line 75: // Convert time point 't' into date-time string at precision 'p' in the local
            : // time zone.
            : static string ToString(const chrono::system_clock::time_point& t, TimePrecision
p)
            : {
            :   stringstream ss;
            :   ss << TimepointToString(t, false);
            :   ss << FormatSubSecond(t, p);
            :   return ss.str();
            : }
            : 
            : // Convert time point 't' into date-time string at precision 'p' in the UTC
            : // time zone.
            : static string ToUtcString(const chrono::system_clock::time_point& t, TimePrecision
p)
            : {
            :   stringstream ss;
            :   ss << TimepointToString(t, true);
            :   ss << FormatSubSecond(t, p);
            :   return ss.str();
            : }
up to you, but you could combine these into a single ToString() function that takes the bool
utc, since these functions are the same otherwise.


http://gerrit.cloudera.org:8080/#/c/8084/3/be/src/util/time.h
File be/src/util/time.h:

PS3, Line 79: ,
maybe add: 's'
just to clarify which input.


PS3, Line 80: strin
string


PS3, Line 81: form
format


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoram Thanga <zoram@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Zoram Thanga <zoram@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message