[kudu-CR] [timestamp] Add a new TIMESTAMP type

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [timestamp] Add a new TIMESTAMP type
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/client/schema.h
File src/kudu/client/schema.h:

Line 135: TIMESTAMP = 10
> I think we're good, here's my reasoning:
> compilation of old user code against the new client lib would fail since the 
> setters accept different types.

Wouldn't it only fail at runtime?

Maybe we should name this new type TIMESTAMP_NANOS anyway, since Impala is 
likely to eventually add TIMESTAMP(6) with millisecond precision down the line, 
and we'll want a way to distinguish them? This way we'd avoid another future 
rename, plus we wouldn't have the back-compat issue.

(meanwhile we could now remove the 'TIMESTAMP =' for a couple versions to force 
people to switch to the new names)


http://gerrit.cloudera.org:8080/#/c/5819/3/src/kudu/util/timestamp_value.cc
File src/kudu/util/timestamp_value.cc:

Line 128:   pt += boost::posix_time::nanoseconds(1);
> hum apparently tidy bot is not picking up the definition that enables nanos
Maybe we should just be #defining it at the top of this file? then tidybot 
would see it. (we don't expose the ptimes ever in the header, right?)


PS3, Line 138:   CHECK_OK(ToPTime(_, _duration_, ));
 :   pt -= boost::posix_time::nanoseconds(1);
 :   if (!pt.is_not_a_date_time()) {
 : FromPTime(pt, _, _duration_);
 : return true;
 :   }
 :  
> this is slightly wrong (nanos_ must be set to 863999L) but changed 
oh, right... i wish days were power-of-ten number of seconds :)


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [timestamp] Add a new TIMESTAMP type

[David Ribeiro Alves (Code Review)]

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(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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [timestamp] Add a new TIMESTAMP type

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5819

to look at the new patch set (#4).

Change subject: [timestamp] Add a new TIMESTAMP type
..

[timestamp] Add a new TIMESTAMP type

This adds a new type to Kudu and adds the ability to use
it in table primary keys. The new type is similar to Impala's
and takes the form of the new TimestampValue class introduced
in a previous patch.

This also adds tests for the new type to all_types-itest both
as a regular column and as part of the primary key.

Change-Id: I63271adf6b87f048e30bf2d6d04c758884752afc
---
M src/kudu/cfile/type_encodings.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/common.proto
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_encoder.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/timestamp_value.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/integration-tests/all_types-itest.cc
20 files changed, 448 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/5819/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63271adf6b87f048e30bf2d6d04c758884752afc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon