David Ribeiro Alves has posted comments on this change. Change subject: [timestamp] Add a new TIMESTAMP type ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/client/schema.h File src/kudu/client/schema.h: Line 135: TIMESTAMP = 10 > hrm... what does this mean for people who might have been using the depreca I think we're good, here's my reasoning: old clients (user code + kudu lib) on new servers would be fine, the server would silently transform the TIMESTAMP into UNIXTIME_MICROS like it does now. (type 9). compilation of old user code against the new client lib would fail since the setters accept different types. new clients against old servers would also fail since the server doesn't know the new type. Any guard rails you'd suggest? http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/client/value-internal.h File src/kudu/client/value-internal.h: Line 46: > Done Actually this was not done. The compiler complains with default constructor of 'Data' is implicitly deleted because variant field 'timestamp_val_' has a non-trivial default constructor http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/client/value.cc File src/kudu/client/value.cc: PS3, Line 90: auto > did this for the other methods too and pushed another patch. actually kept this in this patch. Line 209: RETURN_NOT_OK(timestamp_val_.CheckValid()); > Done Made all client methods that accept timestamp call IsValid() http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/common/encoded_key-test.cc File src/kudu/common/encoded_key-test.cc: Line 276: TEST_F(EncodedKeyTest, TestTimestampKey) { > what about some test with a negative day? is that a legal value? no, the valid ranges for all the fields are now consts in the class. Line 299: ASSERT_LT(to_decode.compare(to_decode2), 0); > some little comments for each test case would be nice. eg here // Test a la Done http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/common/key_encoder.h File src/kudu/common/key_encoder.h: Line 160: Encode(*reinterpret_cast<const Slice*>(key), dst); > same as above, could we reuse the INT32/INT64 decoder? Done -- To view, visit http://gerrit.cloudera.org:8080/5819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63271adf6b87f048e30bf2d6d04c758884752afc Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
