Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21735 )
Change subject: IMPALA-7086: Cache timezone name look ups ...................................................................... Patch Set 1: (7 comments) Looks good! Few minor comments. http://gerrit.cloudera.org:8080/#/c/21735/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21735/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-7086: Cache timezone name look ups the name of the functions could be mentioned, e.g. "Cache timezone in from/to_ut_timestamp(): http://gerrit.cloudera.org:8080/#/c/21735/1//COMMIT_MSG@8 PS1, Line 8: A few more info would be nice: - which functions are modified? (if not mentioned in title) - only the constant timezone name case is affected - some benchmark results could be added http://gerrit.cloudera.org:8080/#/c/21735/1/be/src/exprs/timestamp-functions.h File be/src/exprs/timestamp-functions.h: http://gerrit.cloudera.org:8080/#/c/21735/1/be/src/exprs/timestamp-functions.h@156 PS1, Line 156: // /// Find and initialize timezone if it is a constant. Extra // http://gerrit.cloudera.org:8080/#/c/21735/1/be/src/exprs/timestamp-functions.h@156 PS1, Line 156: it "the timezone name" would be clearer IMO http://gerrit.cloudera.org:8080/#/c/21735/1/be/src/exprs/timestamp-functions.cc File be/src/exprs/timestamp-functions.cc: http://gerrit.cloudera.org:8080/#/c/21735/1/be/src/exprs/timestamp-functions.cc@95 PS1, Line 95: string(tz_string_value.Ptr(), tz_string_value.Len())); nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/21735/1/be/src/exprs/timestamp-functions.cc@109 PS1, Line 109: THREAD_LOCAL It doesn't matter much, but FRAGMENT_LOCAL makes more sense in this case, as the same object is returned anyway, so different threads will access the same Timezone object. http://gerrit.cloudera.org:8080/#/c/21735/1/be/src/exprs/timestamp-functions.cc@147 PS1, Line 147: if (context->IsArgConstant(1)) { optional: I think that it would be slightly faster to skip checking this and get the state directly. If it is nullptr, then we could do the lookup. A DCHECK could be added to verify that in case the state in not nullptr, then context->IsArgConstant(1) is true. -- To view, visit http://gerrit.cloudera.org:8080/21735 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icdf5ff82c5d0554333aef1bc3bba034a4cf48230 Gerrit-Change-Number: 21735 Gerrit-PatchSet: 1 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 29 Aug 2024 13:25:54 +0000 Gerrit-HasComments: Yes
