Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3973: add position and occurrence to instr() ......................................................................
Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/4094/6/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS6, Line 322: search_start_pos > I don't think it's necessary. I check the starting position before the loop Ok, but since it requires a bit of thinking can you add a DCHECK for #2 to make it clear & validate? PS6, Line 336: ntVal(1), BigIntVal(1)); > Because the position of the match is the index of its leftmost char, but we Right, thanks http://gerrit.cloudera.org:8080/#/c/4094/10/be/src/runtime/string-search.h File be/src/runtime/string-search.h: PS10, Line 23: #include <cstring> : #include <vector> can you put the std includes above boost includes, w/ a space btwn? PS10, Line 104: for (int i = 0; i <= mlast; ++i) { : BloomAdd(pattern_->ptr[i]); : if (pattern_->ptr[i] == pattern_->ptr[mlast] && i != mlast) skip_ = mlast - i - 1; : if (pattern_->ptr[i] == pattern_->ptr[0] && i != 0) { : rskip_ = std::min(rskip_, i - 1); : } Given that this is subtle code (at least the algorithm is tricky), I think I'd feel more comfortable if you modeled this more clearly after the code in fastsearch.h, e.g. split this into different loops to create skip_ and rskip_, where the indexes and logic match that of fastsearch.h. You could create the bloom filter in a separate loop or using the loop that creates skip_ and then updating the bloom filter at the end for mlast as the code in fastsearch.h does. -- 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: 10 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
