Lars Volker has posted comments on this change. Change subject: IMPALA-3973: add position and occurrence to instr() ......................................................................
Patch Set 1: (7 comments) Thank you for the change. Please see my 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 number typed parameters. Can you add one such test, too? PS1, Line 2077: 1 The Oracle docs say this value must be >0, please add failure tests for 0 and <0, too. http://gerrit.cloudera.org:8080/#/c/4094/1/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: Line 297: IntVal StringFunctions::Instr(FunctionContext* context, Line wrapping, see .h file. Line 305: // When searching backwards, position is counted from the end. For readability, [lease don't interleave comments and ternary expressions. Use an if-else block instead or move the comment above the line. Line 312: std::basic_string<uint8_t> haystack(str.ptr); Isn't this equivalent to std::string? 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 previous line. Please check the other args, too. 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 -- 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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
