Lars Volker has posted comments on this change. Change subject: IMPALA-3973: add position and occurrence to instr() ......................................................................
Patch Set 9: Code-Review+1 (6 comments) I only added some minor comments. I'm not sure whether you should adapt all of string-search.h to our style guide, now that a significant part of the file changed. I'd be in favor of it, but I don't feel strongly about it. http://gerrit.cloudera.org:8080/#/c/4094/9/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS9, Line 329: IntVal StringFunctions::Instr(FunctionContext* context, const StringVal& str, : const StringVal& substr, const BigIntVal& start_position) { : return Instr(context, str, substr, start_position, BigIntVal(1)); : } : : IntVal StringFunctions::Instr( : FunctionContext* context, const StringVal& str, const StringVal& substr) { : return Instr(context, str, substr, BigIntVal(1), BigIntVal(1)); : } nit: Wrap arguments uniformly for both methods, either with clang-format or according to the surrounding code. http://gerrit.cloudera.org:8080/#/c/4094/9/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: Line 63: static IntVal Instr(FunctionContext*, const StringVal& str, const StringVal& substr); nit: have the same order as in the implementation file. http://gerrit.cloudera.org:8080/#/c/4094/9/be/src/runtime/string-search-test.cc File be/src/runtime/string-search-test.cc: Line 30: // If the length is -1, use the full string length nit: punctuation Line 43: // If the length is -1, use the full string length nit: punctuation http://gerrit.cloudera.org:8080/#/c/4094/9/be/src/runtime/string-search.h File be/src/runtime/string-search.h: Line 101: skip_ = rskip_ = mlast - 1; I haven't seen multiple assignments per line elsewhere, for readability reasons I'd personally favor having one per line. However I don't feel strongly about this Line 105: if (pattern_->ptr[i] == pattern_->ptr[mlast] && i != mlast) { nit: single 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: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zoltan Ivanfi <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Zoltan Ivanfi <[email protected]> Gerrit-HasComments: Yes
