Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201709191954.v8JJsu8C004875@ip-10-146-233-104.ec2.internal>
Date: Tue, 19 Sep 2017 19:54:55 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Zoram Thanga <zoram@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mjacobs@apache.org>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5599=3A_Fix_for_mis-use_of_TimestampValue=0A?=
In-Reply-To: <gerrit.1505515089000.I9b0ae06f6d94968c87a199625aa3332b26988142@gerrit.cloudera.org>
References: <gerrit.1505515089000.I9b0ae06f6d94968c87a199625aa3332b26988142@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Tue, 19 Sep 2017 19:55:02 -0000

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