David Ribeiro Alves has posted comments on this change. Change subject: [timestamp] Add a new TimestampValue class to support the TIMESTAMP_NANOS type ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/5965/5/src/kudu/common/timestamp_value.cc File src/kudu/common/timestamp_value.cc: Line 31: #define BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG 1 > doesn't this #define need to go above where the boost stuff is included? Al not sure what it is though, since this just applies to clang-tidy it might have different behavior PS5, Line 198: if (nanos_in_day_ == TIMESTAMP_MIN_NANOS_IN_DAY) { : ss << "00:00:00"; > this is a bit surprising to me actually. So, if the time is 00:01:00 then ah good catch, this was meant to be: if (nanos_in_day_ == TIMESTAMP_MIN_NANOS_IN_DAY - 1) to test if nanos_in_day_ had been initialized (and print the hours all the same in that case) boost::posix_time::to_simple_string(pt.time_of_day()) already takes care of not printing the nanos if there are none. http://gerrit.cloudera.org:8080/#/c/5965/5/src/kudu/common/timestamp_value.h File src/kudu/common/timestamp_value.h: PS5, Line 40: FromDateAndTimeDuration > This method got renamed -- I think it's mentioned in a couple other comment Done PS5, Line 54: // Note: On macOS, libc's timegm(), which is used to convert to an unix time, fails for numbers : // lower than -2147483648 (the minimum integer that fits an int32_t). Because of this, in macOS, : // the minimum possible date represented by a TimestampValue is 1901-12-14 00:00:00. > chatted offline, but I think we should consider pulling in a friendly licen we looked at https://github.com/evalEmpire/y2038/blob/master/time64.c but it seems like it has no maintenance (and some scary prs outstanding https://github.com/evalEmpire/y2038/pull/14) looked into pulling localtime.c other implementations of libc but usually they need to pull a lot of stuff to work. One of the points of using these simple constants is that people wouldn't have to guess or hard code what the limits are. I've externed the constants, which doesn't solve the problem but makes the header a little better. We can always solve it if it becomes a pain. Note that if it materialized It's going to be our pain and not the users pain and it's something we can refactor without big issues since it is hidden. PS5, Line 206: KuduWriteOperation otherwise > nit: to a KuduWriteOperation; otherwise, it will fail. Done PS5, Line 268: nanos_in_ady_ > nit: typo (also below) Done Line 270: Status CheckValid() const; > hm, does this need to be exported? is it possible that a client will be abl the empty ctor builds an invalid one. we double check on the client before writes, but I think allowing the users to do it whenever is not farfetched. -- To view, visit http://gerrit.cloudera.org:8080/5965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5a0763d3a28501b7f9fb0b94552e3227bd5b38cc Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
