Todd Lipcon 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? Also, the fact that it's working OK despite being down here makes me wonder whether it's necessary at all. Something fishy going on. 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 we write it as 00:01:00.000000000? It's only midnight that we don't stringify the nanos? I would have guessed that we'd want to check nanos_in_day_ % NANOS_IN_SECOND != 0 or something? 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 comments below as well. 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 licensed version of timegm that is 64-bit safe on macos... the ifdefs make the code a bit hard to follow, and afraid that as we add more tests that use timestamps, we'll have to track this assumption all through the codebase (or risk having poor coverage) PS5, Line 206: KuduWriteOperation otherwise nit: to a KuduWriteOperation; otherwise, it will fail. (otherwise is an adverb, not a conjunction. I apologize on behalf of English.) PS5, Line 268: nanos_in_ady_ nit: typo (also below) Line 270: Status CheckValid() const; hm, does this need to be exported? is it possible that a client will be able to set an invalid one? -- 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
