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 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@12 PS3, Line 12: > Where is this enforced/validated? Done http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@13 PS3, Line 13: Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986 > See KUDU-2632: the SQL spec says we should start with 0001. Done http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@15 PS3, Line 15: > Too long, wrap. Done 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 elsewher UTC (GMT) - Universal Time Coordinated, is used to express timezone offset. https://en.wikipedia.org/wiki/Coordinated_Universal_Time So, it would be correct to rename "micros_since_utc_epoch" to "micros_since_unix_epoch" here and elsewhere. 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 Why ? I would place TIMESTAMP right after UNIXTIME_MICROS. http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@365 PS2, Line 365: Status GetUnixTimeMicros(const Slice& col_name, : int64_t* micros_since_utc_epoch) const WARN_UNUSED_RESULT; : Status GetDate(const Slice& col_name, : int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT; > Not done? Done 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, val); > Do we validate the range of values anywhere? Per the ANSI SQL spec, the val Done 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 ma Done 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 an Done 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: ASSERT_EQ("1923-12-01T00:44:36.876544Z", result); > These values should be invalid. Done 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@553 PS3, Line 553: static const int US_TO_S = 1000L * 1000L; > warning: invalid case style for global constant 'US_TO_S' [readability-iden Done http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@583 PS3, Line 583: constexpr static const char* kDateFormat = "%Y-%m-%d"; > Can the kDateFormat above be used? The kDateFormat above contains hours, minutes and seconds, which do are not present in DATE type. http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@584 PS3, Line 584: > This could be moved up by US_TO_S to keep conversions in the same place. These conversions are specific for each type. So, it should be kept in namespaces of respective classes. http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@593 PS3, Line 593: gmtime_r(&days_since_unix_epoch, &tm_info); > Could you simplify with StringAppendStrftime (from gutil/walltime.h)? Done http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@600 PS3, Line 600: template<> : struct DataTypeTraits<DECIMAL32> : public DerivedTypeTraits<INT32>{ : static const char* name() { : return "decimal"; > Rewrite as a ternary: 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: 2 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: Thu, 17 Oct 2019 00:10:37 +0000 Gerrit-HasComments: Yes
