Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/9986 )
Change subject: IMPALA-3307: Add support for IANA time-zone db ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/exprs/timestamp-functions.cc File be/src/exprs/timestamp-functions.cc: http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/exprs/timestamp-functions.cc@95 PS3, Line 95: time_t unix_time; : if (UNLIKELY(!ts_value.UtcToUnixTime(&unix_time))) return TimestampVal::null(); : cctz::time_point<cctz::sys_seconds> from_tp = UnixSecondsToTimePoint(unix_time); : : // Convert 'from_tp' time_point to civil_second assuming 'timezone' time-zone. : cctz::civil_second to_cs = cctz::convert(from_tp, *timezone); : : if (UNLIKELY(CheckIfDateOutOfRange(cctz::civil_day(to_cs)))) { : const string& msg = Substitute( : "Timestamp '$0' did not convert to a valid local time in timezone '$1'", : ts_value.ToString(), tz_string_value.DebugString()); : context->AddWarning(msg.c_str()); : return TimestampVal::null(); : } : : // Note that 'to_cs' has second granularity. Since time-zone rules do not affect : // fractional seconds, the fractional second part of the returned TimestampVal should be : // equal to ts_value.time().fractional_seconds(). : return CivilSecondToTimestampVal(to_cs, ts_value.time().fractional_seconds()); This logic is the same as TimestampValue::UtcToLocal(), plus warning if the resulting timestamp is not valid. As local time and generic timezone conversions are the same now, it would make sense to keep them at one place. http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/exprs/timestamp-functions.cc@144 PS3, Line 144: cctz::time_point<cctz::sys_seconds> from_tp = from_cl.pre; : : // Convert 'from_tp' time_point to civil_second assuming 'UTC' time-zone. : cctz::civil_second to_cs = cctz::convert(from_tp, TimezoneDatabase::GetUtcTimezone()); : : if (UNLIKELY(CheckIfDateOutOfRange(cctz::civil_day(to_cs)))) { : const string& msg = : Substitute("Timestamp '$0' in timezone '$1' could not be converted to UTC", : ts_value.ToString(), tz_string_value.DebugString()); : context->AddWarning(msg.c_str()); : return TimestampVal::null(); : } : : // Note that 'to_cs' has second granularity. Since time-zone rules do not affect : // fractional seconds, the fractional second part of the returned TimestampVal should be : // equal to ts_value.time().fractional_seconds(). : return CivilSecondToTimestampVal(to_cs, ts_value.time().fractional_seconds()); Similarly to my comment at line 113, this logic could be moved to a TimestampValue function. Removing these calculations from this file would mean that the helper functions (line 47-74) could be removed too. http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/runtime/timestamp-value.cc@117 PS3, Line 117: time_ = boost::posix_time::time_duration(to_cs.hour(), to_cs.minute(), to_cs.second(), nit: long line http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/filesystem-util.h File be/src/util/filesystem-util.h: http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/filesystem-util.h@66 PS3, Line 66: iff typo: if http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/filesystem-util.h@70 PS3, Line 70: iff typo: if http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/filesystem-util.h@71 PS3, Line 71: path Maybe writing "string" instead of "path" express better that no file system is accessed. http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/filesystem-util.h@79 PS3, Line 79: path Same as line 71. http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/time.cc File be/src/util/time.cc: http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/time.cc@165 PS3, Line 165: nit: extra space http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/time.cc@168 PS3, Line 168: nit: extra space http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/time.cc@171 PS3, Line 171: nit: extra space -- To view, visit http://gerrit.cloudera.org:8080/9986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I93c1fbffe81f067919706e30db0a34d0e58e7e77 Gerrit-Change-Number: 9986 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 04 May 2018 12:19:21 +0000 Gerrit-HasComments: Yes
