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