Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201608291352.u7TDqvWe023514@ip-10-146-233-104.ec2.internal>
Date: Mon, 29 Aug 2016 13:52:57 +0000
From: "Zoltan Ivanfi (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, dev@impala.incubator.apache.org
CC: Lars Volker <lv@cloudera.com>
Reply-To: zi@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3973=3A_add_position_and_occurrence_to_instr=28=29=0A?=
In-Reply-To: <gerrit.1471974771000.Ie9648de458d243306fa14adc5e7f7002bf6f67fd@gerrit.cloudera.org>
References: <gerrit.1471974771000.Ie9648de458d243306fa14adc5e7f7002bf6f67fd@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 29 Aug 2016 13:53:02 -0000

Zoltan Ivanfi has posted comments on this change.

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


Patch Set 1:

(6 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 num
Done


PS1, Line 2077: 1
> The Oracle docs say this value must be >0, please add failure tests for 0 a
Done


http://gerrit.cloudera.org:8080/#/c/4094/1/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

Line 305:       // When searching backwards, position is counted from the end.
> For readability, [lease don't interleave comments and ternary expressions. 
Done


Line 312:   std::basic_string<uint8_t> haystack(str.ptr);
> Isn't this equivalent to std::string?
No, std::string is signed, therefore I would need to apply a reinterpret_cast on str.ptr, which is far less elegant and more error-prone than using std::basic_string<uint8_t>.


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 pre
I wrapped the parameters this way intentionally, as the two string parameters make one logical group of the arguments and the two bigint parameters can be considered another logical group. Wrapping them accordingly makes the function signature easier to read in my opinion.

Regarding the line length limit, the Impala wiki (https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala) links to the Google C++ style guide, which specifies 80 chars as the line length. If we use 90 chars instead, we should mention this as an exception on our wiki page.


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
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: 1
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: Zoltan Ivanfi <zi@cloudera.com>
Gerrit-HasComments: Yes