impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Ivanfi (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3973: add position and occurrence to instr()
Date Tue, 06 Sep 2016 13:25:55 GMT
Zoltan Ivanfi has posted comments on this change.

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

Patch Set 9:


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.
File be/src/exprs/

PS9, Line 329: IntVal StringFunctions::Instr(FunctionContext* context, const StringVal&
             :     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.
File be/src/exprs/string-functions.h:

Line 63:   static IntVal Instr(FunctionContext*, const StringVal& str, const StringVal&
> nit: have the same order as in the implementation file.
File be/src/runtime/

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
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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9648de458d243306fa14adc5e7f7002bf6f67fd
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoltan Ivanfi <>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Zoltan Ivanfi <>
Gerrit-HasComments: Yes

View raw message