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 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timezone_db.h File be/src/exprs/timezone_db.h: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timezone_db.h@31 PS2, Line 31: class TimezoneDatabase { My general feeling about this class is that a bit more transparency would add great value. What I mean is that some additional class level comments would really help e.g. what is the exact format of the inputs to this class, what sources it can have, in nutshell what transformation is done on that data, and how it uses the processed data later on. What do you think? http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timezone_db.cc@420 PS2, Line 420: /* = nullptr */ drop this comment http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timezone_db.cc@495 PS2, Line 495: ZONEINFO_DIR nit: ZONE_INFO_DIR? http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/runtime/timestamp-value.h@101 PS2, Line 101: static TimestampValue FromUnixTime(time_t unix_time, const cctz::time_zone* local_tz) { Do you think that mentioning the new param for these functions would add extra value? -- 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: 2 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: Fri, 20 Apr 2018 12:00:42 +0000 Gerrit-HasComments: Yes
