Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3973: add position and occurrence to instr() ......................................................................
Patch Set 11: (9 comments) just a few more small things... thanks! Have you tested your test cases against Oracle or another system that supports this? http://gerrit.cloudera.org:8080/#/c/4094/11/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: PS11, Line 2088: TestValue("instr('corporate floor', 'or', 3, 3)", TYPE_INT, 0); : TestValue("instr('corporate floor', 'or', -3, 3)", TYPE_INT, 0); I think we need a few more cases with empty search strings, e.g. TestValue("instr('corporate floor', '', 3, 3)", TYPE_INT, 0); TestValue("instr('corporate floor', '', -3, 3)", TYPE_INT, 0); Line 2102: add a few more null checks: instr('corporate floor', '', -15, 1) is NULL instr('corporate floor', '', 15, 1) is NULL http://gerrit.cloudera.org:8080/#/c/4094/11/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS11, Line 292: if (occurrence.val <= 0 || start_position.val == 0) { : return IntVal(0); : } 1 line PS11, Line 306: == -1 nit: < 0 PS11, Line 322: == -1 < 0 PS11, Line 324: DCHECK(search_start_pos < haystack.len); this check doesn't seem useful here as it did above. The point there was to guard the check to the next Substring() call, so I think in this case we'd want to make sure search_start_pos + needle.len > 0 to make sure the Substring call is valid. http://gerrit.cloudera.org:8080/#/c/4094/11/be/src/runtime/string-search.h File be/src/runtime/string-search.h: Line 104: add a brief comment about how this is transformed from the python code. e.g. they compute the bloom & skip_ each call, we do it all here for reuse. Also why we call BloomAdd in this way. Basically enough for people to look at the python code and see that this maps to that code. PS11, Line 107: if (pattern_->ptr[i] == pattern_->ptr[mlast]) : skip_ = mlast - i - 1; nit: 1 line PS11, Line 113: if (pattern_->ptr[i] == pattern_->ptr[0]) { : rskip_ = i - 1; : } 1 line -- 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: 11 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: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Zoltan Ivanfi <[email protected]> Gerrit-HasComments: Yes
