Adar Dembo 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 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@12 PS3, Line 12: Range: Where is this enforced/validated? http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@13 PS3, Line 13: 0000-01-01 to 9999-12-31 See KUDU-2632: the SQL spec says we should start with 0001. http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@15 PS3, Line 15: -719528 to 2932896 (days; computed with mktime()/(24*60*60), timezone = GMT, daylight saving = 0) Too long, wrap. 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; > Done Not done? http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@593 PS3, Line 593: static void AppendDebugStringForValue(const void* val, std::string* str) { Could you simplify with StringAppendStrftime (from gutil/walltime.h)? http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@600 PS3, Line 600: if (rc == 0) : str->append("invalid arguments for strftime()"); : else : str->append(date); Rewrite as a ternary: str->append(rc == 0 ? "..." : date); -- 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: 3 Gerrit-Owner: Volodymyr Verovkin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[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 16:27:16 +0000 Gerrit-HasComments: Yes
