Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16719 )
Change subject: IMPALA-10323: use snprintf instead of lexical_cast to cast to string ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/16719/1/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/16719/1/be/src/exprs/cast-functions-ir.cc@139 PS1, Line 139: max_char_size +1 I side effect of this change is that the strings created will take more memory - instead of the actual size needed they will always need max_char_size+1. (the real memory consumption is more complicated due to enforcing an alignment of 8 in https://github.com/apache/impala/blob/master/be/src/runtime/mem-pool.h#L111 ) To ensure that this can not lead to regression on queries that store a lot of such strings in memory, I would prefer to do the snprintf in a buffer on stack first, then copy the number of bytes returned snprintf to a string with StringVal::CopyFrom(FunctionContext* ctx, const uint8_t* buf, size_t len). This will add an extra memcpy to your solution, but my guess is that the difference will be minimal. The bulk of the improvement was probably due to lexical_cast's use of heap to construct the temporary string, which is avoided by both improved solutions. -- To view, visit http://gerrit.cloudera.org:8080/16719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief1d6b2c2cb5961c1d6cee1b0eac86ab61509768 Gerrit-Change-Number: 16719 Gerrit-PatchSet: 1 Gerrit-Owner: wesleydeng <wesleyd...@tencent.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: abeltian <abelt...@tencent.com> Gerrit-Comment-Date: Mon, 16 Nov 2020 17:35:54 +0000 Gerrit-HasComments: Yes
