Gabor Kaszab 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: Code-Review+1

(4 comments)

Thanks Attila for addressing my comments! I'm fine with the change.

http://gerrit.cloudera.org:8080/#/c/9986/7/be/src/exprs/timezone_db-test.cc
File be/src/exprs/timezone_db-test.cc:

http://gerrit.cloudera.org:8080/#/c/9986/7/be/src/exprs/timezone_db-test.cc@119
PS7, Line 119:   TestInvalidTimezoneAbbrevName("pST");
Four of these are already tested by TimezoneDbNamesTest. No need to test the 
here as well, I think.


http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/exprs/timezone_db.h
File be/src/exprs/timezone_db.h:

http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/exprs/timezone_db.h@1
PS4, Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> Not sure what happened there. I probably inadvertently executed some myster
:D


http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/exprs/timezone_db.cc
File be/src/exprs/timezone_db.cc:

http://gerrit.cloudera.org:8080/#/c/9986/4/be/src/exprs/timezone_db.cc@365
PS4, Line 365:   RETURN_IF_ERROR(
> LoadZoneInfoFromHdfs() uses cctz::load_time_zone() to load time-zone files
Thx!


http://gerrit.cloudera.org:8080/#/c/9986/5/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

http://gerrit.cloudera.org:8080/#/c/9986/5/be/src/runtime/runtime-state.cc@136
PS5, Line 136:     local_time_zone_ = &TimezoneDatabase::GetUtcTimezone();
> True, but I wanted to be more explicit here.
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: 7
Gerrit-Owner: Attila Jeges <[email protected]>
Gerrit-Reviewer: Attila Jeges <[email protected]>
Gerrit-Reviewer: Csaba Ringhofer <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: Gabor Kaszab <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Wed, 16 May 2018 14:43:24 +0000
Gerrit-HasComments: Yes

Reply via email to