Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: Add a DATE type backed by INT32 (Part 1, C++ client) ...................................................................... Patch Set 2: (6 comments) Looks good overall. I presume you basically looked at all the places Attila had to modify in his VARCHAR patch? 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. 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 example, we could validate that the date range falls between 0001-01-01 and 9999-12-31, but doesn't look like this patch does that. Should it? Or not? Why? 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@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? 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? 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@590 PS2, Line 590: const int32_t * Nit: "const int32_t*" 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? -- 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-Comment-Date: Mon, 14 Oct 2019 19:09:05 +0000 Gerrit-HasComments: Yes
