Michael Ho has posted comments on this change. Change subject: IMPALA-4729: Implement REPLACE() ......................................................................
Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/5776/5/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS5, Line 232: if (UNLIKELY(out_max > 1ULL << 18)) { > I dropped this down to 64k. Seems like the benefit for not re-allocating i Did you measure the performance gain for that ? Any hardcoded bound still looks a bit arbitrary unless there is justification for choosing it. It'd be nice to keep the decision simple while still getting reasonable speedup over regex_replace(). Starting with the next power of 2 of the current string length will work well with how FreePool allocates memory internally. PS5, Line 236: haystack.len + replace.len - pattern.len > should not be able to overflow. preferred indent is Wouldn't this be negative if haystack.len == (1 << 31) - 4 and replace.len = 6 and pattern.len = 2 ? Correct. Indentation is 4 for line continuation. -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <zams...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
