Lars Volker has posted comments on this change. Change subject: IMPALA-3973: add position and occurrence to instr() ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/4094/5/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS5, Line 289: if (str.is_null || substr.is_null) return IntVal::null(); : StringValue str_sv = StringValue::FromStringVal(str); : StringValue substr_sv = StringValue::FromStringVal(substr); : StringSearch search(&substr_sv); : // Hive returns positions starting from 1. : return IntVal(search.Search(&str_sv) + 1); > I would expect a minor, probably insignificant performance impact. (On the I think it makes sense to unify this by using the new method. If there's bugs we should be able to find them. PS5, Line 329: allowed match > When we are searching from the right, there are two "current" positions. On I think we could just remove the comment. The next line by itself explains where the search starts. http://gerrit.cloudera.org:8080/#/c/4094/5/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: PS5, Line 64: context nit: remove to match the surrounding code. http://gerrit.cloudera.org:8080/#/c/4094/5/be/src/runtime/string-search-test.cc File be/src/runtime/string-search-test.cc: Line 28: if (len == -1) { > This is matter of taste, the style guide allows both variant. I like this o You're right. Unfortunately the style guide wasn't clear in this regard. What we do is put single statements in the same line as the "if", if they fit within the 90 character limit. I'm not a fan of this myself, but that's the predominant formatting. I updated the style guide on this occasion to be more specific. The style guide will probably also change a bit in the near future, but I'm confident that this single-line-if-possible rule will not change. PS5, Line 69: Haystack is not full string > I didn't write the test cases, they are almost verbatim copies from https:/ I see, let's fix it here then at least. Please rephrase to "Test searching through part of haystack" or similar. PS5, Line 70: 5 > As I noted in the other comment, I don't know why each individual test case Ok, makes sense to me. Can you add this as a comment to the factory methods above? Line 81: // Test last bit, this is the interesting edge case > I guess last bit means last part. I'm not sure why it's interesting, it was How about "Tests with needle at the end of the haystack." then? Doesn't make sense to me to keep a vague comment if neither of us can fiigure out why it's there. -- 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: 5 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: Zoltan Ivanfi <[email protected]> Gerrit-HasComments: Yes
