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 4: Code-Review+1 (6 comments) Few nits otherwise looks good to me. The LocalToUtc performance part is optional - as it does not affect other parts of the code, it can be easily done later when general structure is already accepted by other reviewers. http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/exprs/timestamp-functions-ir.cc@526 PS4, Line 526: const string& tz_name = (start_lookup.abbr != nullptr) ? start_lookup.abbr : : context->impl()->state()->local_time_zone()->name(); What is the goal of this logic? To print timezone abbreviations instead of the full names, or to distinguish between summer/winter time, or both? A comment would be nice, and maybe the logic could be moved to a TimestampValue member function like string GetTimezoneName(Timezone*). http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/runtime/timestamp-value.cc@93 PS4, Line 93: inline bool CheckIfDateOutOfRange(const cctz::civil_day& date) { : static const cctz::civil_day max_date(TimestampFunctions::MAX_YEAR, 12, 31); : static const cctz::civil_day min_date(TimestampFunctions::MIN_YEAR, 1, 1); : return date < min_date || date > max_date; : } This could be simpler and possibly faster by expecting a cctz::civil_second argument and check if 1400<=year<10000. http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/runtime/timestamp-value.cc@128 PS4, Line 128: cctz explains pretty well the handling of dst boundaries, maybe we could add a link to it, for example to https://github.com/google/cctz/blob/a2dd3d0fbc811fe0a1d4d2dbb0341f1a3d28cb2a/include/cctz/time_zone.h#L147 http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/runtime/timestamp-value.cc@129 PS4, Line 129: // In case of ambiguity invalidate TimestampValue. : const cctz::time_zone::civil_lookup from_cl = local_tz->lookup(from_cs); : if (UNLIKELY(from_cl.kind != cctz::time_zone::civil_lookup::UNIQUE)) { : SetToInvalidDateTime(); : } else { : 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()); : : // boost::gregorian::date() throws boost::gregorian::bad_year if year is not in the : // 1400..9999 range. Need to check validity before creating the date object. : if (UNLIKELY(CheckIfDateOutOfRange(cctz::civil_day(to_cs)))) { I may be possible to get TimestampValue from cctz::time_zone::civil_lookup in a faster way - splitting from_tp to a day part (since a constant date) and the remainder seconds part is enough for us and should be faster then getting cctz::civil_second (which contains year/month/day split). The code could look something like this: int64 secs_since_base = from_tp - BASETIME_AS_CCTZ_SYS_SEC; time_=sec_since_base%(24*60*60)+time_.fractional_seconds(); int32 days_since_base = sec_since_base/(24*60*60); if(out_of_range(days_since_base)) SetToInvalidDateTime(); date_ = days_since_base - BASEDATE_AS_BOOST_GREG_DATE; http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/runtime/timestamp-value.cc@146 PS4, Line 146: 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 > "iff" stands for "if and only if". Thanks for the explanation! -- 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: 4 Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 09 May 2018 11:47:13 +0000 Gerrit-HasComments: Yes
