Dan Hecht 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 7: (2 comments) I focused mostly on the non-timestamp/timezone/time and test/infra parts. It looks fine to me. Would be good to get Gabor's to finish his review to +1 and Tim can do the final +2. Just a heads up regarding exceptions: in the past we've had a lot of issues with timestamp boost routines throwing exceptions for out of range values. You should make sure you exercise any path that can do that with tests. We generally either have to reason about why the boost function can't throw an exception (maybe we check the range before hand) or we wrap the boost call with try/catch so we don't expose the exception. Also IIRC, something to keep in mind is that codegen code can't properly handle try/catch, so in cases we needed to use that, we factored the try/catch code into native code and call out to it from the IR. Again, just a heads up, not sure if your change introduced any problem in this regard or not. http://gerrit.cloudera.org:8080/#/c/9986/7/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/9986/7/be/src/exprs/timestamp-functions-ir.cc@502 PS7, Line 502: : namespace { : inline cctz::time_point<cctz::sys_seconds> UnixTimeToTimePoint(time_t t) { : return std::chrono::time_point_cast<cctz::sys_seconds>( : std::chrono::system_clock::from_time_t(0)) + cctz::sys_seconds(t); : } : : } : : StringVal TimestampFunctions::TimeOfDay(FunctionContext* context) { : const TimestampVal curr = Now(context); : if (curr.is_null) return StringVal::null(); : const string& day = ShortDayName(context, curr); : const string& month = ShortMonthName(context, curr); : IntVal dayofmonth = DayOfMonth(context, curr); : IntVal hour = Hour(context, curr); : IntVal min = Minute(context, curr); : IntVal sec = Second(context, curr); : IntVal year = Year(context, curr); : : // Calculate 'start' time point at which query execution started. : cctz::time_point<cctz::sys_seconds> start = UnixTimeToTimePoint( : context->impl()->state()->query_ctx().start_unix_millis / MILLIS_PER_SEC); : // Find 'tz_name' time-zone abbreviation that corresponds to 'local_time_zone' at : // 'start' time point. : cctz::time_zone::absolute_lookup start_lookup = : context->impl()->state()->local_time_zone()->lookup(start); : const string& tz_name = (start_lookup.abbr != nullptr) ? start_lookup.abbr : : context->impl()->state()->local_time_zone()->name(); any chance that can throw an exception? I believe our IR code can't properly handle try/catch, so if this can indeed throw an exception and needs to be wrapped in try/catch, it may need to be refactored so that this code lives in the native code and we call out to it from the IR. (Just a heads up, this may not be a problem here). http://gerrit.cloudera.org:8080/#/c/9986/7/be/src/runtime/timestamp-value.inline.h File be/src/runtime/timestamp-value.inline.h: http://gerrit.cloudera.org:8080/#/c/9986/7/be/src/runtime/timestamp-value.inline.h@59 PS7, Line 59: .days() in the past we've had issues where the boost date library can throw exceptions. I don't remember the details off hand and it may be that you are okay here given you've already checked HasDateAndTime() and if we ensure date_ is within range, but just wanted to mention it. -- 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: 7 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 15 May 2018 17:34:57 +0000 Gerrit-HasComments: Yes
