Youwei Wang has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() ......................................................................
Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 1919: > long line Done Line 1921: for (int i = 0; i < 3; i++) { > Can you add a test with the second argument being ''? Can you add one where Done http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS2, Line 758: > long line Done PS2, Line 764: MACRO_GET_UNIQUE_CHARS : // When 'chars_to_trim' is unique for each element (e.g. when 'chars_to_trim' : // is each element of a table column), we need to prepare a bitset of unique : // characters here instead of using the bitset from function context. : MACRO_TRIM_CHECK : // Find new starting position. : int32_t begin = 0; : MACRO_TRIM_LEFT_PART : // Find new ending position. : int32_t end = str.len - 1; : MACRO_TRIM_RIGHT_PART : r > Can you please refactor this copied code into a function? Done Line 777: > Please use C++11 (aka 'scoped') enum Done Line 778: StringVal StringFunctions::BTrimStringWithOption(FunctionContext* ctx, > Maybe memcmp? I'm not sure. Done Line 779: const StringVal& str, const StringVal& chars_to_trim, const StringVal& option) { > Do you need this cast? Done Line 785: MACRO_TRIM_CHECK > Please check that it actually does equal "both", unless ISO SQL says that n Done Line 791: { > Please refactor these two blocks, too, to avoid the shared code with the ot Done http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: Line 103: /// Similar to BTrimString with extra direction 'option', which has three choices: > This deserves a function comment Done -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Youwei Wang <[email protected]> Gerrit-HasComments: Yes
