Zach Amsden has posted comments on this change. Change subject: IMPALA-4729: Implement REPLACE() ......................................................................
Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/5776/11/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS11, Line 227: replace = new ReplaceContext(pattern); > Sorry, I may have misunderstood your question about this. We need to call c Done PS11, Line 236: delete rptr; > Please use context->Free(). Done PS11, Line 274: 128 > Mind documenting how the number 128 is derived ? In other words, why not 25 In order to not burn too many cache lines. This makes short string data go really fast. I'll add a comment. PS11, Line 282: (replace.len - pattern.len) > Just use 'delta' for clarity. Done PS11, Line 288: haystack.len - pattern.len + replace.len > For clarity, can you please use haystack.len + delta ? Done PS11, Line 322: const int bytes_remaining = haystack.len - consumed; > This seems to overlap with the remaining_bytes in line 352. How about we ho Done PS11, Line 335: it's > nit: its Done PS11, Line 342: const auto ofs = ptr - result.ptr; > Isn't this the same as bytes_produced above ? yes PS11, Line 352: const int remaining_bytes = haystack.len - consumed; > If you take the suggestion above to hoist bytes_remaining out of the while Done -- To view, visit http://gerrit.cloudera.org:8080/5776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1780a7d8fee6d0db9dad148217fb6eb10f773329 Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
