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 <[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, 13 Apr 2018 12:20:12 +0000 Gerrit-HasComments: Yes
