Jim Apple has posted comments on this change. Change subject: IMPALA-889 Add support for ISO-SQL trim() ......................................................................
Patch Set 2: (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: TestStringValue("btrim('abcdefg', 'aaaaabbbbbccccccccccffffffffffg', 'right')", "abcde"); long line Line 1921: TestStringValue("btrim('', 'abc', 'left')", ""); Can you add a test with the second argument being ''? Can you add one where only the fist argument is NULL? 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: r long line PS2, Line 764: bitset<256>* unique_chars = reinterpret_cast<bitset<256>*>( : ctx->GetFunctionState(FunctionContext::THREAD_LOCAL)); : // 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. : if (!ctx->IsArgConstant(1)) { : unique_chars->reset(); : DCHECK(chars_to_trim.len != 0 || chars_to_trim.is_null); : for (int32_t i = 0; i < chars_to_trim.len; ++i) { : unique_chars->set(static_cast<int>(chars_to_trim.ptr[i]), true); : } : } Can you please refactor this copied code into a function? Line 777: int option_; Please use C++11 (aka 'scoped') enum Line 778: if (strncmp(reinterpret_cast<const char*>(option.ptr), Maybe memcmp? I'm not sure. Line 779: reinterpret_cast<const char*>("left"), 4) == 0) { Do you need this cast? Line 785: // both Please check that it actually does equal "both", unless ISO SQL says that non-"both" arguments that are also not "left" or "right" must be interpreted as "both". Line 791: if (option_ == 0 || option_ == 2) { Please refactor these two blocks, too, to avoid the shared code with the other btrim http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: Line 103: static StringVal BTrimStringWithOption(FunctionContext* ctx, const StringVal& str, This deserves a function comment -- 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: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
