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 6: (4 comments) 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@475 PS4, Line 475: tionContext* context, > If we're going to skip on is_null, then I think we have to stick with passi Sure, we can keep passing StringVal(" ") for the no-arg case. It's not used anyway. That said, we still need to skip if chars_to_trim.is_null == true because this essentially means chars_to_trim is undefined. In fact, we should have a test case for it. http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc@451 PS6, Line 451: direction == LEADING || direction == BOTH To avoid a logic or, we can consider doing: if ((direction & LEADING) != 0) { } if ((direction & TRAILING) != 0) { } For the case of BOTH, you can pass direction as (LEADING | TRAILING). Of course, that requires redefining LEADING and TRAILING to bitmaps. http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc@464 PS6, Line 464: //return DoTrimStringImpl(str, unique_chars, direction); delete http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc@466 PS6, Line 466: : StringVal StringFunctions::Trim(FunctionContext* context, const StringVal& str) { : return DoTrimString(context, str, StringVal(" "), BOTH); : } : : StringVal StringFunctions::Ltrim(FunctionContext* context, const StringVal& str) { : return DoTrimString(context, str, StringVal(" "), LEADING); : } : : StringVal StringFunctions::Rtrim(FunctionContext* context, const StringVal& str) { : return DoTrimString(context, str, StringVal(" "), TRAILING); : } Can you please do a quick benchmark to make sure we didn't regress the perf of these expressions with this change ? It seems that we almost should templatize StringFunctions::DoTrimString(). -- 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: 6 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: Tue, 19 Dec 2017 22:59:29 +0000 Gerrit-HasComments: Yes
