[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

["Dan Hecht (Code Review)" <ge...@cloudera.org>]

Dan Hecht has posted comments on this change.

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


Patch Set 2:

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

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.

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_PER_MILLI'?


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


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


PS2, Line 1924:  Precision::Second
and here


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 function comment for them at the function definition.


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

(and you don't the namespace in the cc files. you can also take a look at names.h and see how it's included to pull in the "common" names to .cc files).


Line 53: 
nit: consider removing blank lines here and line 68 to make more code fit on screen, since it doesn't seem like these blanks help readability.


Line 65:     // 1-second precision or unknown unit. Return empty string.
nice to document (and prove) that invariant with a dcheck:
DCHECK_EQ(p, Precision::Second);


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 long the type name is, use your judgement).


PS2, Line 95: system_clock
the docs for chrono::system_clock::time_point claim that the epoch is unspecified, so we can we be sure this works everywhere? it does seem likely to always use unix epoch, but technically it doesn't have to.


PS2, Line 101: chrono::microseconds
given that the "to" type is the same as the "from" type, why is the duration_cast needed?


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?


PS2, Line 79: Unix timestamp
I think we usually refer to this as "Unix time", and timestamp often invokes the thought of our timestamp type, so how about we say:
... the input Unix time, specified in seconds since the Unix epoch, to a ...


PS2, Line 81: p
nit: in header comments to distinguish variables, we usually use single quotes: 'p'


PS2, Line 85: Precision p=Precision::Second
normally we should avoid default arguments, but in this case it does seem reasonable to assume the output precision should be the same as the input, so I'm okay with that. but please put a space around the = just like in other parts of the code.


-- 
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 <zo...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-HasComments: Yes