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 4: (6 comments) Thanks for the comments. Uploading patch set #5. http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@406 PS4, Line 406: StringFunctions::Ltrim > nit: Can you please group these 3 functions close to *TrimString variant so Done http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@411 PS4, Line 411: static_cast<const StringVal&>(" ") > This seems unnecessary now. Can we just pass StringVal::null() in this case Can do this, but please see response to comment on line 475. Retaining this for now, albeit in a simpler form. http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@418 PS4, Line 418: new bitset<256>() > context->Allocate<>() ? Allocate<>() returns an uninitialized buffer. Can't use this. Else I'l have to explicitly call bitset.reset() on the returned unique_chars. Seems to me it's a lot simpler to just use the existing new/delete combination and let the bitset constructor do the initialization. http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@439 PS4, Line 439: delete unique_chars; > context->Free(); Please see response to previous comment. http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@443 PS4, Line 443: DoTrimStringImpl > Seems that this function can be merged with DoTrimString(). Done http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@475 PS4, Line 475: chars_to_trim.is_null > Shouldn't we skip if chars_to_trim.is_null ? We cannot make assumption abou If we're going to skip on is_null, then I think we have to stick with passing StringVal(" ") from the no-args trim functions. Otherwise it's going to be hard to make a distinction on whether we want to trim spaces, or the chars_to_trim is null, so there's no work to be 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: 4 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, 15 Dec 2017 20:35:32 +0000 Gerrit-HasComments: Yes
