impala-reviews mailing list archives

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

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


Patch Set 2:

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

http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS2, Line 921: UnixMillis()
> for consistency in these values, how about setting this to 'now_us / MICROS
Done


PS2, Line 1146:  Precision::Second
> seems okay to print milliseconds here
Done


PS2, Line 1845: , Precision::Second
> here too
Done


PS2, Line 1924:  Precision::Second
> and here
Done


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

Line 32: static std::string TimepointToString(const chrono::system_clock::time_point&
t,
> for static things not defined in header, it's helpful to include a short fu
Done


PS2, Line 46:   std::string s(buf);
            :   return s;
> return string(buf);
Done


Line 53: 
> nit: consider removing blank lines here and line 68 to make more code fit o
Done


Line 65:     // 1-second precision or unknown unit. Return empty string.
> nice to document (and prove) that invariant with a dcheck:
Done. But I had to change TimePrecision from an enum class to a regular enum, because DCHECK
requires the argument to have an "<<" operator.


PS2, Line 89:   chrono::system_clock::time_point t(chrono::duration_cast<chrono::seconds>(
            :         chrono::seconds(s)));
            :   return t;
> could consider combing these like suggested at line 46-47 (but given how lo
Done


PS2, Line 95: system_clock
> the docs for chrono::system_clock::time_point claim that the epoch is unspe
It's correct that the standard does not specify the epoch for chrono::system_clock. The Windows
and Linux implementations, however, are based on the Unix epoch. This is a good read http://www.modernescpp.com/index.php/the-three-clocks.


PS2, Line 101: chrono::microseconds
> given that the "to" type is the same as the "from" type, why is the duratio
Removed the cast by combining class instantiation and return statements.


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

PS2, Line 72: Precision
> "precision" seems kind of generic. How about calling that TimePrecision?
Done


PS2, Line 79: Unix timestamp
> I think we usually refer to this as "Unix time", and timestamp often invoke
Done


PS2, Line 81: p
> nit: in header comments to distinguish variables, we usually use single quo
Done


PS2, Line 85: Precision p=Precision::Second
> normally we should avoid default arguments, but in this case it does seem r
Done


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