Zoram Thanga 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 7: (4 comments) Thanks for the comments. Please see patch set #8 http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions-ir.cc@441 PS7, Line 441: IS_CONSTANT > We can possibly get rid of this after IMPALA-6380 is fixed. Agree. Will revisit this aspect of the code. http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions-ir.cc@443 PS7, Line 443: chars_to_trim.is_null > As discussed offline, we cannot make any assumption about the length of Str As discussed, this seems like a bad DCHECK anyway - because '' and NULL are legit values of chars_to_trim, even when the arguments come from column elements. Removing the DCHECK, and instead check for zero-length or NULL chars_to_trim, as these do not modify the target string. Have added code to check for NULL chars_to_trim in TrimPrepare(). http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions.h@87 PS7, Line 87: static void TrimPrepare(FunctionContext*, FunctionContext::FunctionStateScope); > A quick comment on what this Prepare() function do would be useful. Done http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions.h@149 PS7, Line 149: template <TrimPosition D, bool IS_CONSTANT> > Please add a comment on what these template parameters stand for. Done -- 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: 7 Gerrit-Owner: Zoram Thanga <zo...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Fri, 12 Jan 2018 00:14:12 +0000 Gerrit-HasComments: Yes
