Zoltan Ivanfi has posted comments on this change. Change subject: IMPALA-3973: add position and occurrence to instr() ......................................................................
Patch Set 11: (8 comments) > (9 comments) > > just a few more small things... thanks! > > Have you tested your test cases against Oracle or another system > that supports this? Yes, I ran these queries in Oracle and used those results as the reference values. 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. Done Line 2102: > add a few more null checks: These should not be NULL, because none of the parameters are NULL. These simply return 0 to indicate an error. 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 Done PS11, Line 306: == -1 > nit: < 0 Done PS11, Line 322: == -1 > < 0 Done 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 Done, but shouldn't we simply add a DCHECK to Substring itself (in a follow-up change)? http://gerrit.cloudera.org:8080/#/c/4094/11/be/src/runtime/string-search.h File be/src/runtime/string-search.h: PS11, Line 107: if (pattern_->ptr[i] == pattern_->ptr[mlast]) : skip_ = mlast - i - 1; > nit: 1 line This line was not modified compared to base and I wouldn't like to mix purely styleguide-driven changes with actual behavioral changes in a single commit. As I already suggested in an earlier comment, I am happy to run clang-format on the whole file and fix styleguide-related issues in a follow-up change. This way we will end up with a clean file history that allows easy identification of the reason of changes later on. PS11, Line 113: if (pattern_->ptr[i] == pattern_->ptr[0]) { : rskip_ = i - 1; : } > 1 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: 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
