Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18958 )
Change subject: IMPALA-11563: Optimized /etc/sysconfig/clock to find the time zone ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/18958/4/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: http://gerrit.cloudera.org:8080/#/c/18958/4/be/src/exprs/timezone_db.cc@184 PS4, Line 184: "ZONE=\"" You could extract this string to a constant (or constexpr) so we wouldn't have to repeat it on L187. http://gerrit.cloudera.org:8080/#/c/18958/4/be/src/exprs/timezone_db.cc@187 PS4, Line 187: header Something like 'header_len' would be clearer. http://gerrit.cloudera.org:8080/#/c/18958/4/be/src/exprs/timezone_db.cc@188 PS4, Line 188: p_end - p_start - header We could extract this to a variable, for example 'result_len'. http://gerrit.cloudera.org:8080/#/c/18958/4/be/src/exprs/timezone_db.cc@188 PS4, Line 188: substr Why do you prefer substr() instead of erase()? Because of the assignment we end up modifying 'result' in this case too. -- To view, visit http://gerrit.cloudera.org:8080/18958 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f80fd1817d072f8dadf288025cb9534191ca458 Gerrit-Change-Number: 18958 Gerrit-PatchSet: 4 Gerrit-Owner: Baike Xia <xiaba...@163.com> Gerrit-Reviewer: Baike Xia <xiaba...@163.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jian Zhang <zjsar...@gmail.com> Gerrit-Reviewer: Xiang Yang <yx91...@126.com> Gerrit-Comment-Date: Tue, 17 Jan 2023 14:26:46 +0000 Gerrit-HasComments: Yes