Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19875 )
Change subject: IMPALA-12134: Optimize row materialization time ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/19875/1/be/src/benchmarks/date-benchmark.cc File be/src/benchmarks/date-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/19875/1/be/src/benchmarks/date-benchmark.cc@84 PS1, Line 84: to_string_old_result_.resize(date_.size()); > nit: AFAIK we try to limit to 90 columns in benchmark files too (the style nit: can you fix the long lines? http://gerrit.cloudera.org:8080/#/c/19875/3/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/19875/3/be/src/runtime/timestamp-value.cc@187 PS3, Line 187: timestamp_value.time(), dst) >= 0)) { : dst[len] = '\0'; This looks a bit weird to me, as we accept any >= 0 return value, but always put the null termination at len. I think that it would be clearer if we would simply return early if the time/date are "special". In the current code GetDefaultTimestampFormatContext also calls time.fractional_seconds() without checking whether it is a valid time, potentially leading to exception. -- To view, visit http://gerrit.cloudera.org:8080/19875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1ef5e4137fa6c2d0a5f08b430e01e3fb7de86330 Gerrit-Change-Number: 19875 Gerrit-PatchSet: 3 Gerrit-Owner: Kurt Deschler <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Comment-Date: Tue, 16 May 2023 14:57:05 +0000 Gerrit-HasComments: Yes
