Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8349 )
Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters. ...................................................................... Patch Set 8: (3 comments) Looking good. Close to completion. Please find some more comments below. http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions-ir.cc@410 PS8, Line 410: if (context->GetNumArgs() < 2) { Please DCHECK(context->GetNumArgs() == 1 or context->GetNumArgs() == 2); http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions-ir.cc@436 PS8, Line 436: if (str.len == 0 || chars_to_trim.len == 0 || chars_to_trim.is_null) return str; Seems that this line can be moved. If str.len == 0, the while loop will take care of it so no need to special case as we don't expect it to be common. "chars_to_trim" being empty or null doesn't seem to be common case and it doesn't warrant an extra if-check here as the common case is when "chars_to_trim" is constant. Instead, we may want to move if (chars_to_trim.is_null) to line 443 below for the case in which "char_to_trim" is not constant. http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions.h@153 PS8, Line 153: , flags whether or not the set of : /// characters to be trimmed is fixed, or column elements is true when the set of characters to trim is constant. -- To view, visit http://gerrit.cloudera.org:8080/8349 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79 Gerrit-Change-Number: 8349 Gerrit-PatchSet: 8 Gerrit-Owner: Zoram Thanga <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Zoram Thanga <[email protected]> Gerrit-Comment-Date: Fri, 12 Jan 2018 23:17:30 +0000 Gerrit-HasComments: Yes
