Zoltan Ivanfi has posted comments on this change. Change subject: IMPALA-3973: add position and occurrence to instr() ......................................................................
Patch Set 9: (6 comments) I would suggest clang-formatting this file in a follow-up change. This way we separate the formatting changes cleanly in the sense that every part of this diff clearly means to a functional change, while every part of the follow-up change will purely reflect a formatting change. > 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 Hmmm, this was wrapped by clang-format, I have no idea why the wrapping is different. Probably clang-format noticed that all arguments fit a single line in line 335, that's why it didn't split it. I think we should accept clang-format's output, otherwise we can spend a lot of time on fixing its style, which would make using clang-format pointless. 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. Done 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 Done Line 43: // If the length is -1, use the full string length > nit: punctuation Done 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 rea Done, although in my opinion assigning two vars the same value in a single-line is cleaner. Line 105: if (pattern_->ptr[i] == pattern_->ptr[mlast] && i != mlast) { > nit: single 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: 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
