Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11183 )
Change subject: IMPALA-7521: Speed up sub-second unix time->TimestampValue conversions ...................................................................... Patch Set 16: (4 comments) http://gerrit.cloudera.org:8080/#/c/11183/16/be/src/benchmarks/convert-timestamp-benchmark.cc File be/src/benchmarks/convert-timestamp-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/11183/16/be/src/benchmarks/convert-timestamp-benchmark.cc@592 PS16, Line 592: (uint64_t)MICROS_PER_SEC > Is there a reason to use static_cast for primitive numeric types? IMO C sty AFAIK we are following the google style guide here: https://google.github.io/styleguide/cppguide.html#Casting The problem with C-style casting is that it is too strong, e.g. it can cast pointers to integers. However, the google style guide allows brace-initialization, which is as brief as a C-style cast and only does conversion, not casting. http://gerrit.cloudera.org:8080/#/c/11183/16/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: http://gerrit.cloudera.org:8080/#/c/11183/16/be/src/runtime/timestamp-test.cc@241 PS16, Line 241: 1000 > Attila mentioned this too. I think that 1000 is easier to read than MILLIS_ It's usually better to use named constants because they express the intent better. Here it is trivial why we need 1000 so I don't feel strong about it. http://gerrit.cloudera.org:8080/#/c/11183/16/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/11183/16/be/src/runtime/timestamp-value.h@188 PS16, Line 188: (int64_t)1000*1000*1000 > I was not happy to write this here, but walltime.h constants are not availa nit: I see, btw you could use C++14 digit separators: 1'000'000'000 http://gerrit.cloudera.org:8080/#/c/11183/16/be/src/runtime/timestamp-value.h@318 PS16, Line 318: int64_t SplitTime(int64_t* ticks) > I see the point, and I trust the compiler that it will eliminate the variab Using different variables lets you use more names to express the calculation better, i.e. to make the code easier to read. E.g. in this example 'nanos' means something different before and after the function call. That's my view, but I'm OK if you don't agree with it and keep it as it is. -- 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: 16 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 06 Sep 2018 12:29:52 +0000 Gerrit-HasComments: Yes
