[Impala-CR](cdh5-trunk) IMPALA-889 Add support for ISO-SQL trim()

["Jim Apple (Code Review)" <ge...@cloudera.org>]

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 <yo...@intel.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-HasComments: Yes