Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/11183 )
Change subject: IMPALA-7417: Speed up sub-second unix time->TimestampValue conversions ...................................................................... Patch Set 11: (15 comments) Thanks! Another batch of comments: http://gerrit.cloudera.org:8080/#/c/11183/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11183/11//COMMIT_MSG@10 PS11, Line 10: (which is split do date_ and time_ similarly to boost::posix_time::ptime) nit: please wrap at 70 characters, here and below. http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/benchmarks/convert-timestamp-benchmark.cc File be/src/benchmarks/convert-timestamp-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/benchmarks/convert-timestamp-benchmark.cc@578 PS11, Line 578: boost::posix_time::seconds(time) It probably wouldn't make a difference but maybe you should call boost::posix_time::nanoseconds(time*NANOS_PER_SEC) here to be consistent with TimestampValue::UtcFromUnixTimeTicks(). http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/benchmarks/convert-timestamp-benchmark.cc@631 PS11, Line 631: temp += boost::posix_time::nanoseconds((unix_time - unix_time_whole) * NANOS_PER_SEC); The original implementation had: temp += boost::posix_time::nanoseconds((unix_time - unix_time_whole) / ONE_BILLIONTH); Any reason you changed it? http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/benchmarks/convert-timestamp-benchmark.cc@635 PS11, Line 635: TimestampValue from_subsecond_unix_time_new( : const double& unix_time) { : int64_t unix_time_whole = round(unix_time); : int64_t nanos = round((unix_time - unix_time_whole)*NANOS_PER_SEC); : return TimestampValue::FromUnixTimeNanos( : unix_time_whole, nanos, TimezoneDatabase::GetUtcTimezone()); : } The implementation of TimestampValue::FromSubsecondUnixTime() was changed in patch-set #11. Please change the implementation here too. http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/benchmarks/convert-timestamp-benchmark.cc@923 PS11, Line 923: // Note that the test data contains only whole seconds. The results could be slightly : // different for sub-second times due to the different rounding rules. Are the rounding rules still different? http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/benchmarks/convert-timestamp-benchmark.cc@932 PS11, Line 932: Benchmark from_subsecond_unix_time("FromSubsecondUnixTime"); This benchmark and the previous bm_utc_from_unix_time_nanos benchmark measures very similar code. Do we need both? http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-test.cc@235 PS11, Line 235: Double can express sub-nanoseconds You mean "'nanos' can express sub-nanoseconds"? I'm not really sure what is the role of 'nanos' in this test. Could you clarify in the comment? http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-test.cc@240 PS11, Line 240: 1000 Maybe use MILLIS_PER_SEC instead? http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-test.cc@282 PS11, Line 282: nit: extra space http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-test.cc@280 PS11, Line 280: // Try the same timestamp with opposite sign. : seconds += millis < 0 ? -1 : 1; : millis += millis < 0 ? 1000 : -1000; : EXPECT_EQ(result, TestFromSubSecondFunctionsInner(seconds, millis)); Why do we need this extra test? And how is this 'opposite sign'? Please clarify in the comment. http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-test.cc@284 PS11, Line 284: if nit: missing space after 'if' http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-test.cc@848 PS11, Line 848: Test subsecond unix time conversion for non edge cases. How about adding some extra tests for the micros/nanos unix time conversion functions to make sure that micros/nanos precision is handled correctly. http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-value.h@189 PS11, Line 189: time.total_nanoseconds() >= 0 Instead this, you could use !time.is_negative() http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-value.h@190 PS11, Line 190: time.total_nanoseconds() < NANOS_PER_DAY maybe use (time.hours() < 24) ? http://gerrit.cloudera.org:8080/#/c/11183/11/be/src/runtime/timestamp-value.h@328 PS11, Line 328: unix_time unix_time_ticks -- To view, visit http://gerrit.cloudera.org:8080/11183 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I572b5876b979ddae58165bd40d5b008ce9d7a4aa Gerrit-Change-Number: 11183 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Fri, 24 Aug 2018 15:25:29 +0000 Gerrit-HasComments: Yes
