Grant Henke 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 3: (8 comments) Looks good. Mainly comments on properly handling DATE_MIN and DATE_MAX. http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/scan_batch.h File src/kudu/client/scan_batch.h: http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/scan_batch.h@246 PS3, Line 246: Status GetDate(int col_idx, int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT; nit: use days_since_utc_epoch to match GetUnixTimeMicros. Here and elsewhere too. http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/schema.h@192 PS3, Line 192: TIMESTAMP = UNIXTIME_MICROS, //!< deprecated, use UNIXTIME_MICROS Nit: Keep timestamp last http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partial_row.cc@265 PS3, Line 265: return Set<TypeTraits<DATE> >(col_name, days_since_unix_epoch); Do we validate the range of values anywhere? Per the ANSI SQL spec, the valid date range is 0001-01-01 to 9999-12-31 http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc@1013 PS3, Line 1013: case DATE: I think this should be DATE_MIN (0001-01-01) + 1 given the minimum and maximum date values allowed. http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc@1100 PS3, Line 1100: if (value < INT32_MAX) { I think this should be checking DATE_MAX (9999-12-3) given the minimum and maximum date values allowed. http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types-test.cc File src/kudu/common/types-test.cc: http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types-test.cc@59 PS3, Line 59: TestDateToString("-5877641-06-23", std::numeric_limits<int32_t>::min()); These values should be invalid. http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@583 PS3, Line 583: constexpr static const char* kDateFormat = "%F"; // the ISO 8601 date format Can the kDateFormat above be used? http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@584 PS3, Line 584: static constexpr time_t kSecondsInDay = 24 * 60 * 60; This could be moved up by US_TO_S to keep conversions in the same place. -- 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: 3 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: Wed, 16 Oct 2019 13:30:37 +0000 Gerrit-HasComments: Yes
