Alexey Serbin 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 7: (5 comments) Few nits 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 of the standard scope shift, i.e. this should be 4, not 2 spaces 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. Why not to use constant reference here instead? http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc@715 PS11, Line 715: nit: fix the alignment 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("1970-01-01", 0); : TestDateToString("1942-08-16", -10000); Does it make sense to add a couple more cases to test for off-by-one issues, if any? Something like this: TestDateToString("value ...", *DataTypeTraits<DATE>::min_value() - 1); TestDateToString("value ...", *DataTypeTraits<DATE>::max_value() + 1); 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 -- 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: 7 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: Fri, 13 Dec 2019 06:11:42 +0000 Gerrit-HasComments: Yes
