Gabor Kaszab has posted comments on this change. ( )

Change subject: IMPALA-3307: Add support for IANA time-zone db

Patch Set 1:


Spent only a limited amount of time on this review. Will continue next week.
Commit Message:
PS1, Line 7: time-zone
Fun fact:
I was curious whether "timezone", "time zone" or "time-zone" is correct. 
Apparently all of them are, however I read that "time-zone" is a bit outdated.  
"time zone" is widely used and "timezone" is only in the US.
Can someone native confirm? :)
File be/src/benchmarks/
PS1, Line 1: #include <chrono>
Missing Apache header
PS1, Line 38: UtcToUnixTime:             Function  iters/ms   10%ile   50%ile   
90%ile     10%ile     50%ile     90%ile
Do you think we should force the 90col limit on the following comment as well?
PS1, Line 136:     ss << to_simple_string(start);
to_simple_string() return a string already, no need to use a stringstream for 
this purpose.
File be/src/exprs/decimal-operators.h:
PS1, Line 168:   /// instead of truncating if 'round' is true.
Should we mention the new parameter in the comment?
File be/src/runtime/runtime-state.h:
PS1, Line 321: global local
I see the intent but "global local timezone" sounds strange :)
PS1, Line 321: -
nit: typo

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I93c1fbffe81f067919706e30db0a34d0e58e7e77
Gerrit-Change-Number: 9986
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Jeges <>
Gerrit-Reviewer: Csaba Ringhofer <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Fri, 13 Apr 2018 12:20:12 +0000
Gerrit-HasComments: Yes

Reply via email to