Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17980 )
Change subject: IMPALA-10984: Improve TimestampValue to String casting ...................................................................... Patch Set 3: (12 comments) Just have some very minor comments. Looks great to me and the improvement is impressive! http://gerrit.cloudera.org:8080/#/c/17980/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17980/3//COMMIT_MSG@17 PS3, Line 17: reimplement nit reimplements http://gerrit.cloudera.org:8080/#/c/17980/3//COMMIT_MSG@22 PS3, Line 22: passed to TimestampParser::Format along with date_ and time_ to be nit is passed http://gerrit.cloudera.org:8080/#/c/17980/3//COMMIT_MSG@32 PS3, Line 32: Machine Info: Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz nit. duplicated in before/after. Probably should be mentioned the para at line 26. http://gerrit.cloudera.org:8080/#/c/17980/3//COMMIT_MSG@33 PS3, Line 33: iters/ms nit. this column can be removed? http://gerrit.cloudera.org:8080/#/c/17980/3//COMMIT_MSG@38 PS3, Line 38: 5.94 nit. not aligned with the rest of the values in this column. http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/datetime-iso-sql-format-tokenizer.h File be/src/runtime/datetime-iso-sql-format-tokenizer.h: http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/datetime-iso-sql-format-tokenizer.h@111 PS3, Line 111: fmt_out_max_len nit. fmt_out_max_len_? http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/datetime-simple-date-format-parser.cc File be/src/runtime/datetime-simple-date-format-parser.cc: http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/datetime-simple-date-format-parser.cc@401 PS3, Line 401: const DateTimeFormatContext* SimpleDateFormatTokenizer::GetDefaultTimestampFormatContext( : const time_duration& time) { : return time.fractional_seconds() > 0 ? &DEFAULT_DATE_TIME_CTX[9] : : &DEFAULT_SHORT_DATE_TIME_CTX; : } inline? http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/timestamp-parse-util.h@128 PS3, Line 128: int max_length I wonder if we need this argument, since the source is 'str' which has a length attribute. http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/timestamp-value.cc@83 PS3, Line 83: (written < 0) UNLIKELY? http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/timestamp-value.cc@86 PS3, Line 86: dst.resize(written) I wonder why resize once dst is populated. http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/timestamp-value.cc@225 PS3, Line 225: (written < 0) unlikely? http://gerrit.cloudera.org:8080/#/c/17980/3/be/src/runtime/timestamp-value.cc@230 PS3, Line 230: StringVal TimestampValue::ToStringVal(FunctionContext* ctx) const { : const DateTimeFormatContext* dt_ctx = : SimpleDateFormatTokenizer::GetDefaultTimestampFormatContext(time_); : return ToStringVal(ctx, *dt_ctx); : } nit This method probably can be inlined. -- To view, visit http://gerrit.cloudera.org:8080/17980 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4fcb4545d9c9a3fdb38c4db58bb4b1321a429d61 Gerrit-Change-Number: 17980 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Fri, 29 Oct 2021 17:00:27 +0000 Gerrit-HasComments: Yes
