Csaba Ringhofer 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 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc File be/src/benchmarks/convert-timestamp-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc@875 PS12, Line 875: Note that the test data contains only whole seconds. > Why not add sub-second microsec test values? Done http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc@899 PS12, Line 899: Benchmark FromUnixTimeNanos improvement in IMPALA-7417. : // Note that the test data contains only whole seconds. > Why not add sub-second nanosec test values? Done http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc@917 PS12, Line 917: sec split > Shouldn't this be "day split"? The first division in nano conversion is splitting nano to second and (sub-second) nanosecond parts. This is why I still consider it do be sec split. The majority of the speedup in nano + double functions comes from replacing "temp += boost::posix_time::nanoseconds(...);" with "result.time_ += boost::posix_time::nanoseconds(...);". The former has to care about overflowing to another day, while the later does not. http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc@939 PS12, Line 939: (old) > "sec split (old)" ? see line 917 http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc@940 PS12, Line 940: (new) > "day split (new)" ? see line 917 http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-test.cc@238 PS12, Line 238: TimestampValue > const TimestampValue& Done http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-test.cc@249 PS12, Line 249: 10 > I could be mistaken, but shouldn't this diff be 0 or 1? It can be more than a microsec due to double's lower precision at large numbers. The biggest one I saw was 7, so I have set the limit to 8 + added a comment about this. http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-test.cc@290 PS12, Line 290: EXPECT_EQ(result, TestFromSubSecondFunctionsInner( : seconds + sign * offset, millis - 1000 * sign * offset)); > EXPECT_EQ() is not really necessary here. It checks that I have removed TestFromSubSecondFunctionsInner, and call FromUnixTimeNanos directly in this loop, as that is the only functions where this shifting matters. http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-test.cc@295 PS12, Line 295: if (expected == nullptr) EXPECT_FALSE(result.HasDate()); : else EXPECT_EQ(expected, result.ToString()); > I would also move these lines after L284 for readability. Good idea. http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-value.h@315 PS12, Line 315: GRANURALITY > Typo: GRANULARITY. Done -- 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: 12 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 31 Aug 2018 15:48:36 +0000 Gerrit-HasComments: Yes
