Alex Behm has posted comments on this change. Change subject: IMPALA-4729: Implement REPLACE() ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/5776/2//COMMIT_MSG Commit Message: Line 11: since function names act as bare identifiers. > I don't think std::replace would work here - the input isn't exactly easily Makes sense. Still a basic perf comparison against regex_replace() would be good. http://gerrit.cloudera.org:8080/#/c/5776/2/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: Line 210: if (pattern.len == 0) return str; > Yes, unclear what the proper behavior is. I elected to return the original When in doubt check Postgres (e.g. via sqlfiddle.com) Line 218: const int kMemoizedMatches = 16; > Will fix style - still learning this. I have no evidence to back up the need for more than 16. Intuitively 16 seems to be sufficient for most cases. My comment is more about documenting in the code the motivation behind this memo and the reason for its specific size. -- 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: 2 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
