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 1:

(7 comments)

Spent only a limited amount of time on this review. Will continue next week.

http://gerrit.cloudera.org:8080/#/c/9986/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9986/1//COMMIT_MSG@7
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? :)


http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/benchmarks/convert-timestamp-benchmark.cc
File be/src/benchmarks/convert-timestamp-benchmark.cc:

http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/benchmarks/convert-timestamp-benchmark.cc@1
PS1, Line 1: #include <chrono>
Missing Apache header


http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/benchmarks/convert-timestamp-benchmark.cc@38
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?


http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/benchmarks/convert-timestamp-benchmark.cc@136
PS1, Line 136:     ss << to_simple_string(start);
to_simple_string() return a string already, no need to use a stringstream for 
this purpose.


http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/exprs/decimal-operators.h
File be/src/exprs/decimal-operators.h:

http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/exprs/decimal-operators.h@168
PS1, Line 168:   /// instead of truncating if 'round' is true.
Should we mention the new parameter in the comment?


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

http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/runtime/runtime-state.h@321
PS1, Line 321: global local
I see the intent but "global local timezone" sounds strange :)


http://gerrit.cloudera.org:8080/#/c/9986/1/be/src/runtime/runtime-state.h@321
PS1, Line 321: -
nit: typo



--
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: 1
Gerrit-Owner: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 12:20:12 +0000
Gerrit-HasComments: Yes

Reply via email to