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 11: (18 comments) http://gerrit.cloudera.org:8080/#/c/11183/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11183/9//COMMIT_MSG@35 PS9, Line 35: This function was : only used in tests until > Why? Is it used for testing only? If so, please add a sentence explaining Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/benchmarks/convert-timestamp-benchmark.cc File be/src/benchmarks/convert-timestamp-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/benchmarks/convert-timestamp-benchmark.cc@258 PS9, Line 258: < > nit: space is missing before '<<' operator Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/benchmarks/convert-timestamp-benchmark.cc@561 PS9, Line 561: GRANULARITY > typo: GRANULARITY. Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/benchmarks/convert-timestamp-benchmark.cc@574 PS9, Line 574: Sp > nit: remove extra spaces. Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/exec/hdfs-orc-scanner.cc@27 PS9, Line 27: #include "runtime/timestamp-value.inline.h" : #include "runtime/tuple-row.h" > nit: order alphabetically. Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@229 PS9, Line 229: UtcToLoc > You should validate the time value here (call Validate() or whatever functi Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@295 PS9, Line 295: > Fix the comment or move the time-validation logic below to a separate funct Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@298 PS9, Line 298: ti > else if Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@303 PS9, Line 303: Time's validity is only checked in debug builds. : /// TODO: This could be also checked in release, but I a > Maybe instead of DCHECK calls, you should call SetToInvalidDateTime() here? I am unsure about the future of Validate() - with the exception of cases when the timestamp is created with memcpy from an unchecked source (e.g. Parquet reading), it may be better to use a DCHECK instead of SetToInvalidDateTime() - generally the caller logic should ensure that only valid arguments are used. I have moved the check to a separate function. http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@314 PS9, Line 314: > typo: GRANULARITY Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@327 PS9, Line 327: > I think, this should be called 'unix_time_ticks' Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@327 PS9, Line 327: > Maybe this should be called UtcFromUnixTimeTicks(). Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.cc@153 PS9, Line 153: TimestampValue TimestampValue::UnixTimeToLocal( : time_t unix_time, const Timezone& local_tz) { > Fix comment. I have fixed the comment in .h and removed the comment from .cc. http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.cc@168 PS9, Line 168: T > nit: insert empty line after } Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.cc@169 PS9, Line 169: > nit: missing space between ) and { Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.inline.h File be/src/runtime/timestamp-value.inline.h: http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.inline.h@34 PS9, Line 34: 64_t unix > should be called 'unix_time_ticks' Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.inline.h@34 PS9, Line 34: UtcFromUnixTime > should be called UtcFromUnixTimeTicks(). Done http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.inline.h@61 PS9, Line 61: unix_ > Is calling round() here and below intentional? The previous version of From I have removed the rounds because it lead to slightly (+-1 nanosec) different results in a test. I think that using round() leads more to precise results but I am trying to avoid adding functional changes in this commit. (It would be probably the best to remove direct double<->timestamp conversion and replace it with decimal<->timestamp, see IMPALA-7472. -- 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 <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 23 Aug 2018 20:55:43 +0000 Gerrit-HasComments: Yes
