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