Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: Add a DATE type backed by INT32 (Part 1, C++ client) ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.cc@659 PS2, Line 659: nit: fix the indent http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@582 PS2, Line 582: struct DataTypeTraits<DATE> : public DerivedTypeTraits<INT32>{ Does it make sense to add a unit test for DataTypeTraits<DATA>::AppendDebugStringForValue() to cover the conversion of date as number of seconds from the start of the Unix epoch into a string? Something like TestTypes.TestTimestampPrinting ? http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@583 PS2, Line 583: %Y-%m-%d Maybe, simply use %F and add a comment that this is ISO 8601 date format? http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@595 PS2, Line 595: strftime Though highly unlikely to get some unexpected output here, but does it make sense to check for the return code of the strftime() function, handling the case when 'date' would contain unexpected contents as the result? -- 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: 2 Gerrit-Owner: Volodymyr Verovkin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 15 Oct 2019 00:28:03 +0000 Gerrit-HasComments: Yes
