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 <z...@cloudera.com> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Zoltan Ivanfi <z...@cloudera.com> Gerrit-HasComments: Yes
