impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Youwei Wang (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Date Thu, 23 Jun 2016 01:46:31 GMT
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 <youwei.a.wang@intel.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Youwei Wang <youwei.a.wang@intel.com>
Gerrit-HasComments: Yes

Mime
View raw message