Michael Ho has posted comments on this change. Change subject: IMPALA-4729: Implement REPLACE() ......................................................................
Patch Set 5: (6 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)) { > This is why I was asking earlier about what is considered a small / large m Sounds good. PS5, Line 236: haystack.len + replace.len - pattern.len > max string size is 1 << 30 Oops. Bad example. I suppose line 244 below will catch the case in which the buffer space exceeds StringVal::MAX_LENGTH. PS5, Line 266: UNLIKELY(check_space)) Not sure if UNLIKELY() is really unlikely here. Isn't it dependent on the inputs ? PS5, Line 272: buffer_space <<= 1; Please see comments in StringVal::Resize(). I think this can potentially go to 1 << 31 but it should be fine as long as StringVal::Resize() catches it. May want to DCHECK_LE(buffer_space, 1ULL << 30); PS5, Line 273: DCHECK( DCHECK_EQ(). http://gerrit.cloudera.org:8080/#/c/5776/5/be/src/udf/udf.cc File be/src/udf/udf.cc: PS5, Line 519: auto* new_ptr = ctx->impl()->ReallocateLocal(ptr, new_len); This should return StringVal::null() if new_len exceeds StringVal::MAX_LENGTH. -- 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 <[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
