impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Ivanfi (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3973: add position and occurrence to instr()
Date Mon, 29 Aug 2016 13:52:57 GMT
Zoltan Ivanfi has posted comments on this change.

Change subject: IMPALA-3973: add position and occurrence to instr()
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4094/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 2070: 
> I saw below that some tests use something like "cast(10 as bigint)" for num
Done


PS1, Line 2077: 1
> The Oracle docs say this value must be >0, please add failure tests for 0 a
Done


http://gerrit.cloudera.org:8080/#/c/4094/1/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

Line 305:       // When searching backwards, position is counted from the end.
> For readability, [lease don't interleave comments and ternary expressions. 
Done


Line 312:   std::basic_string<uint8_t> haystack(str.ptr);
> Isn't this equivalent to std::string?
No, std::string is signed, therefore I would need to apply a reinterpret_cast on str.ptr,
which is far less elegant and more error-prone than using std::basic_string<uint8_t>.


http://gerrit.cloudera.org:8080/#/c/4094/1/be/src/exprs/string-functions.h
File be/src/exprs/string-functions.h:

PS1, Line 65: str
> We usually wrap arguments greedily at 90 chars, so str would fit in the pre
I wrapped the parameters this way intentionally, as the two string parameters make one logical
group of the arguments and the two bigint parameters can be considered another logical group.
Wrapping them accordingly makes the function signature easier to read in my opinion.

Regarding the line length limit, the Impala wiki (https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala)
links to the Google C++ style guide, which specifies 80 chars as the line length. If we use
90 chars instead, we should mention this as an exception on our wiki page.


http://gerrit.cloudera.org:8080/#/c/4094/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

PS1, Line 435:   [['instr'], 'INT', ['STRING', 'STRING', 'BIGINT', 'BIGINT'], 'impala::StringFunctions::Instr'],
> long line
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4094
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9648de458d243306fa14adc5e7f7002bf6f67fd
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoltan Ivanfi <zi@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message