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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
