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 12: (14 comments) http://gerrit.cloudera.org:8080/#/c/11183/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11183/12//COMMIT_MSG@43 PS12, Line 43: thw typo: the 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@932 PS11, Line 932: > I added the test originally to check if the rounding has added any regressi Thanks for the explanation, I'm okay with keeping it. 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? 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? 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"? http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc@939 PS12, Line 939: (old) "sec split (old)" ? http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/benchmarks/convert-timestamp-benchmark.cc@940 PS12, Line 940: (new) "day split (new)" ? 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& 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? 'expected' is set to 'from_millis' in L267 and has millisec precision. 'from_double's time member is set to milliseconds + sub-nanos (<1 nano). So when they are both converted to microseconds their diff should be 0 (or maybe 1), right? 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 (seconds*1000 + millis) calculated on L258 is the same as ((seconds + sign * offset)*1000 + (millis - 1000 * sign * offset)) which is always true. I would change TestFromSubSecondFunctionsInner() to return void and just call TestFromSubSecondFunctionsInner() here in a loop without EXPECT_EQ(). Calculations in L257-259 can be moved to L284 and 'result' can be passed to TestFromSubSecondFunctionsInner() as a parameter. 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. http://gerrit.cloudera.org:8080/#/c/11183/12/be/src/runtime/timestamp-test.cc@884 PS12, Line 884: // A few test with sub-millisec precision. : auto tz = TimezoneDatabase::GetUtcTimezone(); : EXPECT_EQ("1970-01-01 00:00:00.000001000", : TimestampValue::UtcFromUnixTimeMicros(1).ToString()); : EXPECT_EQ("1969-12-31 23:59:59.999999000", : TimestampValue::UtcFromUnixTimeMicros(-1).ToString()); : : EXPECT_EQ("1970-01-01 00:00:00.000001000", : TimestampValue::FromUnixTimeMicros(1, tz).ToString()); : EXPECT_EQ("1969-12-31 23:59:59.999999000", : TimestampValue::FromUnixTimeMicros(-1, tz).ToString()); : : EXPECT_EQ("1970-01-01 00:00:00.000000001", : TimestampValue::FromUnixTimeNanos(0, 1, tz).ToString()); : EXPECT_EQ("1969-12-31 23:59:59.999999999", : TimestampValue::FromUnixTimeNanos(0, -1, tz).ToString()); Thanks for adding these. 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. 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@190 PS11, Line 190: time.total_nanoseconds() < NANOS_PER_DAY > hours() is a confusing function in my opinion - I think that it should be c Ok, I'm convinced :) -- 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 10:05:44 +0000 Gerrit-HasComments: Yes
