impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Ivanfi (Code Review)" <ger...@cloudera.org>
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:

(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 <zi@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message