Zoltan Borok-Nagy 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/benchmarks/convert-timestamp-benchmark.cc File be/src/benchmarks/convert-timestamp-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/benchmarks/convert-timestamp-benchmark.cc@150 PS2, Line 150: void AddTestDataDateTimes(vector<TimestampValue>& data, int n, const string& startstr) { Since it's in a benchmark it doesn't really matter, anyway, some nit comments: - output parameters should be listed last and they should be pointers - in this particular case I think it would be better to return vector<TimestampValue> http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/benchmarks/convert-timestamp-benchmark.cc@174 PS2, Line 174: > > Nit: since C++11 you don't need to put spaces between right angle brackets. http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/benchmarks/convert-timestamp-benchmark.cc@228 PS2, Line 228: const shared_ptr<vector<FROM> > data_ Nit: I think using 'const vector<FROM>&' would be simpler. I don't really see why we need shared_ptr here. Also, in 'const shared_ptr<T>' only the pointer is const, the pointed object is not. http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/benchmarks/convert-timestamp-benchmark.cc@319 PS2, Line 319: boost_throw_if_date_out_of_range(local_time.date()); Is this function call really needed here? Don't we trust boost that it validates the date correctly? http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/benchmarks/convert-timestamp-benchmark.cc@608 PS2, Line 608: if (cctz_utc_to_unix_data.get_result() != glibc_utc_to_unix_data.get_result()) { : cerr << "cctz/glibc utc_to_unix results do not match!" << endl; : return 1; : } : if (boost_utc_to_unix_data.get_result() != glibc_utc_to_unix_data.get_result()) { : cerr << "boost/glibc utc_to_unix results do not match!" << endl; : return 1; : } The other benchmarks don't need this validity check? It could be implemented in a helper function that takes a vector of TestData. http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timestamp-functions.cc File be/src/exprs/timestamp-functions.cc: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timestamp-functions.cc@122 PS2, Line 122: or Nit: of 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@21 PS2, Line 21: boost I think we should use the unordered_map class from the C++ STL. http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/exprs/timezone_db.h@56 PS2, Line 56: TZ_MAP TimezoneMap? I don't think a typename should be all capitals. -- 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: Thu, 19 Apr 2018 14:20:32 +0000 Gerrit-HasComments: Yes
