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 3: (16 comments) http://gerrit.cloudera.org:8080/#/c/5965/2/src/kudu/common/timestamp_value-test.cc File src/kudu/common/timestamp_value-test.cc: Line 37: TEST(TestTimestampValue, TestGetTimes) { > hm, I find it a little surprising that the nanosecond portion is only inclu yeah http://gerrit.cloudera.org:8080/#/c/5965/2/src/kudu/common/timestamp_value.cc File src/kudu/common/timestamp_value.cc: PS2, Line 57: kUnixEpoch > kUnixEpoch Done PS2, Line 112: } : temp += boost::posix_time::nanoseconds(nanos); : FromPTime(tem > why not just FromPTime(temp, &out->date_, &out->time_duration_)? remnats from when we delegated the validation to ptime, done Line 168: } > I find it slightly strange that Increment() has a side effect when it retur Done Line 171: return true; > same Done http://gerrit.cloudera.org:8080/#/c/5965/2/src/kudu/common/timestamp_value.h File src/kudu/common/timestamp_value.h: PS2, Line 46: /// : #if !defined(__APPLE__) : /// The minimum possible value for the date component of a TimestampValue. Corresponds to : /// 1400-01-01. > is this different than the range supported by Impala? I'm nervous because I I digged further and it seems like the bug I observed (in converting a ptime to an unix time for big negative numbers) only happens on macOS, at least I couldn't repro on centOS. I've added some ifdefs and now we have the same range. PS2, Line 51: /// The minimum unix time possible. Corresponds to 1400-01-01 00:00:00. : const int64_t TIMESTAMP_MIN_UNIX_TIME = -17987443200L; : #else : // Note: On macOS, libc's timegm(), which is used to convert to an unix time, fails for numbers : / > I see that this is doxygen format, but also references the internal represe Done Line 61: const int64_t TIMESTAMP_MIN_NANOS_IN_SECOND = 0L; > maybe MIN_NANOS_IN_SECOND and MAX_NANOS_IN_SECOND here, vs MIN_NANOS_IN_DAY Done PS2, Line 73: Represents a date and ti > nit: *a* date and time Done Line 77: /// DATETIME datatype, but unlike these types it has nanosecond precision. > should note that, unlike those types, we are storing nanosecond precision. Done PS2, Line 83: /// Usage examples: : /// : /// Write path : /// @code : /// KuduPartialRow* row; // Row from a KuduWriteOperation > not sure what documentation value this is providing. It's an interesting si right, copy pasted from impala, removed. PS2, Line 107: : /// @note The fields of this class match Impala's > this is super confusing to me... I don't understand what you mean by "the o no, you're right. ToUnixTime doesnt take the local time zone into account. Done PS2, Line 133: /// int64_t unix_time, nanos; > per above, an example would be useful here just to clarify. eg: Done PS2, Line 161: > I think here and everywhere else in the file we should s/time_duration/nano Done PS2, Line 208: TimestampValue() : date_(TIMESTAMP_MIN_DAT > hm, the test seems to indicate that if the nanos is 0 then they are not inc ambivalent towards this. maybe the best option is to retain impala compatibility. fwiw i had it originally the way you but ended up changing to retain compatibility (and because there were a couple of minor issues, which I can't recall right now :( ) PS2, Line 212: int64_t nanos_in_ady() const { : return nanos_in_day_; : > again I think a very specific example would be helpful. added a redirect to the methods that have full examples. -- 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: 3 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: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
