wesleydeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/16719 )
Change subject: IMPALA-10323: use gutil to cast int to string ...................................................................... Patch Set 7: (3 comments) > wesleydeng, can you publish the change as non-draft? The later > patches are drafts, which is why the Jenkins job didn't merge it. done http://gerrit.cloudera.org:8080/#/c/16719/3/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/16719/3/be/src/exprs/cast-functions-ir.cc@134 PS3, Line 134: > I'd prefer if you used a constant here, i.e. MAX_FLOAT_CHARS and maybe add Actually max_char_size is a constant here which will be replace by MAX_BIGINT_CHARS. But to defend others to reuse this macro by passing a variable, I will make this modification as you suggest. I has some problem to connect gerrit now, I will push new patch on Monday. http://gerrit.cloudera.org:8080/#/c/16719/3/be/src/exprs/cast-functions-ir.cc@140 PS3, Line 140: char char_buffer[MAX_EXACT_NUMERIC_CHARS];\ : char* end_position = to_string_method(val.val, char_buffer); \ : size_t len = end_position - char_buffer; \ : DCH > I wonder if we can skip this check, as a StringVal object with not sufficie I keep this check because I'm not sure. Other sugguestion are applied to new patch. http://gerrit.cloudera.org:8080/#/c/16719/4/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/16719/4/be/src/exprs/cast-functions-ir.cc@27 PS4, Line 27: #include <gutil/strings/substitute.h> > nit: we generally include headers sorted alphabetically, so numbers.h comes fixed -- 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: 7 Gerrit-Owner: wesleydeng <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: abeltian <[email protected]> Gerrit-Reviewer: wesleydeng <[email protected]> Gerrit-Comment-Date: Tue, 24 Nov 2020 11:45:42 +0000 Gerrit-HasComments: Yes
