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 5: (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: Status KuduPartialRow::SetInt32(int col_idx, int32_t val) { > Could add a PREDICT_FALSE here. Done http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc@296 PS4, Line 296: return Set<TypeTraits<INT16> >(col_idx, val); : } : Status KuduPartialRow::SetInt32(int col_idx, int32_t val) { : return Set<TypeTraits<INT32> >(col_idx, val); : } : Status KuduPartialRow::SetInt64(int col_idx, int64_t val) { : return Set<TypeTraits<INT64> >(col_idx, val); : } : Status KuduPartialRow::SetUnixTimeMicros(int col_idx, int64_t micros_since_utc_epoch) { : return Set<TypeTraits< > Nit: move to the top of the file. That's where we usually stash static func Done 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"? 1) It's just AppendDebugStringForValue() function. 2) There is no built-in format for 4 digit year. 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: static constexpr int32_t kMinValue = -719162; // mktime(0001-01-01) : static constexpr int32_t kMaxValue = 2932896; // mkti > These two could be static variables declared in the body of AppendDebugStri Done http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h@603 PS4, Line 603: } > Nit: should be IsValidValue Done 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: strin > Nit: add using and drop. Done http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc@122 PS4, Line 122: ix_e > Nit: "DATE" 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: 5 Gerrit-Owner: Volodymyr Verovkin <verjov...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <verjov...@cloudera.com> Gerrit-Comment-Date: Thu, 17 Oct 2019 01:29:23 +0000 Gerrit-HasComments: Yes
