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 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc@298 PS4, Line 298: if (!DataTypeTraits<DATE>::isValidValue(val)) { Could add a PREDICT_FALSE here. http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc@296 PS4, Line 296: namespace { : Status CheckDateValueInRange(int col_idx, int32_t val, const Schema* schema) { : if (!DataTypeTraits<DATE>::isValidValue(val)) { : const ColumnSchema& col = schema->column(col_idx); : return Status::InvalidArgument( : Substitute("value $0 out of range for date column '$1'", val, col.name())); : } : return Status::OK(); : } : } // anonymous namespace Nit: move to the top of the file. That's where we usually stash static functions and anonymous namespaces. http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types-test.cc File src/kudu/common/types-test.cc: http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types-test.cc@54 PS4, Line 54: TestDateToString("1-01-01", *DataTypeTraits<DATE>::min_value()); What are the expectations around leading 0s? Should this be "0001-01-01"? http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h@583 PS4, Line 583: constexpr static const char* kDateFormat = "%F"; // the ISO 8601 date format : static constexpr time_t kSecondsInDay = 24 * 60 * 60; These two could be static variables declared in the body of AppendDebugStringForValue as that's the only place they're used. http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h@603 PS4, Line 603: static bool isValidValue(int32_t val) { Nit: should be IsValidValue http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc File src/kudu/common/types.cc: http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc@116 PS4, Line 116: std:: Nit: add using and drop. http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc@122 PS4, Line 122: Date Nit: "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: 4 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: Thu, 17 Oct 2019 00:22:49 +0000 Gerrit-HasComments: Yes
