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 9: (18 comments) Did a quick first-round review. I will look at the tests the next time. 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 should have : no visible side-effects. Why? Is it used for testing only? If so, please add a sentence explaining it. 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 http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/benchmarks/convert-timestamp-benchmark.cc@561 PS9, Line 561: GRANURALITY typo: GRANULARITY. http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/benchmarks/convert-timestamp-benchmark.cc@574 PS9, Line 574: nit: remove extra spaces. 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/tuple-row.h" : #include "runtime/timestamp-value.inline.h" nit: order alphabetically. 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: set_time You should validate the time value here (call Validate() or whatever function time-validation ends up in) http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@295 PS9, Line 295: /// Sets both date and time to invalid if date is outside the valid range. Fix the comment or move the time-validation logic below to a separate function. http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@298 PS9, Line 298: if else if http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@303 PS9, Line 303: DCHECK_GE(time_.total_nanoseconds(), 0); : DCHECK_LT(time_.total_nanoseconds(), NANOS_PER_DAY); Maybe instead of DCHECK calls, you should call SetToInvalidDateTime() here? Again, it might make sense to move time-validation code to a separate function. http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@314 PS9, Line 314: GRANURALITY typo: GRANULARITY http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@327 PS9, Line 327: unix_time I think, this should be called 'unix_time_ticks' http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.h@327 PS9, Line 327: UtcFromUnixTime Maybe this should be called UtcFromUnixTimeTicks(). 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: /// Return a ptime representation of the given Unix time (seconds since the Unix epoch). : /// The time zone of the resulting ptime is 'local_tz'. This is called by UnixTimeToPtime. Fix comment. http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.cc@168 PS9, Line 168: } nit: insert empty line after } http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.cc@169 PS9, Line 169: ){ nit: missing space between ) and { 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: UtcFromUnixTime should be called UtcFromUnixTimeTicks(). http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.inline.h@34 PS9, Line 34: unix_time should be called 'unix_time_ticks' http://gerrit.cloudera.org:8080/#/c/11183/9/be/src/runtime/timestamp-value.inline.h@61 PS9, Line 61: round Is calling round() here and below intentional? The previous version of FromSubsecondUnixTime() just casts unix_time to int64_t. Is this change addressed in the commit message? -- 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: 9 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: Wed, 22 Aug 2018 16:23:17 +0000 Gerrit-HasComments: Yes
