Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client) ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG@7 PS2, Line 7: Add a DATE type backed by INT32 (Part 1, C++ client) > Tag with KUDU-2632. Done http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG@9 PS2, Line 9: This adds a new DATE type, represented by an INT32 and > Could you add some commentary about validation (or lack thereof)? For examp Done http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@141 PS2, Line 141: Status SetUnixTimeMicros(int col_idx, int64_t micros_since_utc_epoch) WARN_UNUSED_RESULT; > warning: function 'kudu::KuduPartialRow::SetUnixTimeMicros' has a definitio Done http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@142 PS2, Line 142: Status SetDate(int col_idx, int32_t days_since_unix_epoch) WARN_UNUSED_RESULT; > warning: function 'kudu::KuduPartialRow::SetDate' has a definition with dif Done http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@365 PS2, Line 365: Status GetUnixTimeMicros(const Slice& col_name, : int64_t* micros_since_utc_epoch) const WARN_UNUSED_RESULT; : Status GetDate(const Slice& col_name, : int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT; > Nit: could you fix the indentation here? Done http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/row.h File src/kudu/common/row.h: http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/row.h@669 PS2, Line 669: void AddDate(int32_t seconds_utc_since_epoch) { > Should be days_since, right? Done http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@582 PS2, Line 582: struct DataTypeTraits<DATE> : public DerivedTypeTraits<INT32>{ > Does it make sense to add a unit test for DataTypeTraits<DATA>::AppendDebug Done http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@583 PS2, Line 583: %Y-%m-%d > Maybe, simply use %F and add a comment that this is ISO 8601 date format? Done http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@593 PS2, Line 593: gmtime_r(&days_since_unix_epoch, &tm_info); > I assume this does the right thing when days_since_unix_epoch is negative? Yes, I checked. gmtime_r produces correct output for all int64_t values. http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@595 PS2, Line 595: strftime > Though highly unlikely to get some unexpected output here, but does it make Done -- To view, visit http://gerrit.cloudera.org:8080/14427 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986 Gerrit-Change-Number: 14427 Gerrit-PatchSet: 2 Gerrit-Owner: Volodymyr Verovkin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Wed, 16 Oct 2019 08:39:40 +0000 Gerrit-HasComments: Yes
