Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14885 )
Change subject: IMPALA-8891 concat_ws() null handling is non-standard This patch is for IMPALA-8891,in the original implementation of function concat_ws, any null string element in array argument strs will result in null result,as it is shown below: -- ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/14885/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14885/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-8891 concat_ws() null handling is non-standard about commit message formatting: - the common format of first line is JIRA: text - we usually wrap at 72 chars. This can be ignored in copy-pasted outputs http://gerrit.cloudera.org:8080/#/c/14885/1/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/14885/1/be/src/exprs/string-functions-ir.cc@864 PS1, Line 864: if (strs[0].is_null) return StringVal::null(); : int32_t total_size = strs[0].len; This could be merged to the loop by starting at i=0 http://gerrit.cloudera.org:8080/#/c/14885/1/be/src/exprs/string-functions-ir.cc@877 PS1, Line 877: Ubsan::MemCpy(ptr, strs[0].ptr, strs[0].len); : ptr += strs[0].len; Why not merge to the loop? http://gerrit.cloudera.org:8080/#/c/14885/1/be/src/exprs/string-functions-ir.cc@892 PS1, Line 892: // nit: empty comment http://gerrit.cloudera.org:8080/#/c/14885/1/be/src/exprs/string-functions-ir.cc@910 PS1, Line 910: // Pass through if there's only one argument nit: We generally try to be consistent with comment, for example if they start with a capital latter then also put a dot at the end. http://gerrit.cloudera.org:8080/#/c/14885/1/be/src/exprs/string-functions-ir.cc@915 PS1, Line 915: // Iterate over all valid values to compute the final size and reserve space. : for (int32_t i = (valid_start_index + 1); i < num_children; ++i) { : if (strs[i].is_null) continue; : total_size += sep.len + strs[i].len; : } optional: I would prefer to merge this to the previous loop. sep.len could be added an else branch for if(valid_start_index == -1). -- To view, visit http://gerrit.cloudera.org:8080/14885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I64cd3bfbb952e431a0cf52a5835ac05d2513d29b Gerrit-Change-Number: 14885 Gerrit-PatchSet: 1 Gerrit-Owner: jichen <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 11 Dec 2019 16:07:44 +0000 Gerrit-HasComments: Yes
