Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/11057 )
Change subject: IMPALA-5050: Add support to read TIMESTAMP_MILLIS and TIMESTAMP_MICROS from Parquet ...................................................................... Patch Set 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/exprs/timezone_db.h File be/src/exprs/timezone_db.h: http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/exprs/timezone_db.h@79 PS12, Line 79: Add WARN_UNUSED_RESULT. http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/exprs/timezone_db.h@78 PS12, Line 78: /// Public proxy for LoadZoneInfo. Should be only used in BE tests. : static Status LoadZoneInfoBeTestOnly(const std::string& zone_info_dir) { : return LoadZoneInfo(zone_info_dir); : } Maybe instead of this function add a "friend class TimestampTest;" after L87. http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-test.cc@909 PS12, Line 909: CET CET is a deprecated name and its usage is not recommended. It would be better to use an IANA tz name, eg. Europe/Budapest. http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-test.cc@928 PS12, Line 928: imestamps that fall to UTC+2->UTC+1 DST change. More detailed information would be nice: e.g: - Up to 2017-10-29 02:59:59.999999999 AM: - Offset is UTC+02:00 - After 2017-10-29 02:59:59.999999999 AM: - Clocks were moved backward to become 2017-10-29 02:00:00 AM - Offset is UTC+01:00. http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-test.cc@950 PS12, Line 950: Timestamp that falls to UTC+1->UTC+2 DST change More detailed information would be nice: e.g: - Up to 2017-03-26 01:59:59.999999999 AM: - Offset is UTC+01:00 - After 2017-03-26 01:59:59.999999999 AM: - Clocks were moved forward to become 2017-03-26 03:00:00 AM - Offset is UTC+02:00. http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-value.h@238 PS12, Line 238: any timestamp that is earlier in UTC This is a little confusing. Earlier than what? http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-value.h@239 PS12, Line 239: and any : /// timestamp later in UTC Later than what? http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/11057/12/be/src/runtime/timestamp-value.cc@137 PS12, Line 137: simple typo: simply http://gerrit.cloudera.org:8080/#/c/11057/12/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/11057/12/testdata/data/README@251 PS12, Line 251: CET Again, using a real IANA name would be better. http://gerrit.cloudera.org:8080/#/c/11057/12/testdata/workloads/functional-query/queries/QueryTest/parquet-int64-timestamps.test File testdata/workloads/functional-query/queries/QueryTest/parquet-int64-timestamps.test: http://gerrit.cloudera.org:8080/#/c/11057/12/testdata/workloads/functional-query/queries/QueryTest/parquet-int64-timestamps.test@4 PS12, Line 4: CET It would be better to use an IANA timezone name here and below. CET is a deprecated name. -- To view, visit http://gerrit.cloudera.org:8080/11057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c7c01fffa31b3d2ca3480adf6ff851137dadac3 Gerrit-Change-Number: 11057 Gerrit-PatchSet: 12 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 12 Oct 2018 11:30:44 +0000 Gerrit-HasComments: Yes
