[Impala-ASF-CR] IMPALA-3973: add position and occurrence to instr()

["Zoltan Ivanfi (Code Review)" <ge...@cloudera.org>]

Zoltan Ivanfi has posted comments on this change.

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


Patch Set 13:

(5 comments)

We should also update the documentation for instr(). Where can I do that?

 > FYI we should try to get query generator support for this, I filed:
 > https://issues.cloudera.org/browse/IMPALA-4115

http://gerrit.cloudera.org:8080/#/c/4094/13/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 2064:   TestValue("instr('', '')", TYPE_INT, 0);
> would probably be good to add this test case for backwards search.
Done


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

Line 292:   if (occurrence.val <= 0 || start_position.val == 0) return IntVal(0);
> is this what other systems do? i.e. do they silently return 0 or do they ra
Thanks for spotting this. An invalid value for start position is handled by silently returning a 0, therefore I assumed that an invalid occurrence value is handled in the same way. But in reality, the latter gives an error:

[Oracle][ODBC][Ora]ORA-01428: argument '0' is out of range (SQL-HY000)


PS13, Line 302: DCHECK
> DCHECK_LT.
Done


PS13, Line 317: DCHECK
> DCHECK_GT. but what is the meaning of this DCHECK?  search_start_pos is the
Done


PS13, Line 319: std::min
> This is pretty confusing. I think this would be clearer if we did the min()
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-HasComments: Yes