Attila Jeges 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 Done 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 Timesta Done 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 Done 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 "iff" stands for "if and only if". http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/filesystem-util.h@70 PS3, Line 70: iff > typo: if Same as above 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 Done http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/filesystem-util.h@79 PS3, Line 79: path > Same as line 71. Done 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 Done http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/time.cc@168 PS3, Line 168: > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/9986/3/be/src/util/time.cc@171 PS3, Line 171: > nit: extra space Done -- 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: Tue, 08 May 2018 19:35:52 +0000 Gerrit-HasComments: Yes
