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 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.h@450 PS11, Line 450: > style nit: if continuing a statement on the next line, shift is two times o Done http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc@50 PS11, Line 50: const Schema& schema > style nit: I'm not sure we widely use constant pointers as input parameters Done http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc@715 PS11, Line 715: int32_t* d > nit: fix the alignment Done http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc File src/kudu/common/types-test.cc: http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc@56 PS11, Line 56: TestDateToString("1-01-01", *DataTypeTraits<DATE>::min_value()); : TestDateToString("9999-12-31", *DataTypeTraits<DATE>::max_value()); > Does it make sense to add a couple more cases to test for off-by-one issues Done http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types.h@604 PS11, Line 604: }; > nit: remove the extra line Done http://gerrit.cloudera.org:8080/#/c/14427/7/src/kudu/integration-tests/all_types-itest.cc File src/kudu/integration-tests/all_types-itest.cc: http://gerrit.cloudera.org:8080/#/c/14427/7/src/kudu/integration-tests/all_types-itest.cc@578 PS7, Line 578: KuduScanner scanner(table_.get()); > This seems like pretty important test coverage for data types that can be p 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: 12 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: Tue, 17 Dec 2019 01:36:44 +0000 Gerrit-HasComments: Yes
