Michael Ho has posted comments on this change. Change subject: IMPALA-4729: Implement REPLACE() ......................................................................
Patch Set 14: (8 comments) Looking good. Just some more minor comments. http://gerrit.cloudera.org:8080/#/c/5776/13/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS13, Line 238: (context->GetFunctionState(FunctionContext::FRAGMENT_LOCAL)); nit: line continuation uses indent 4. http://gerrit.cloudera.org:8080/#/c/5776/14/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: PS14, Line 238: (context->GetFunctionState(FunctionContext::FRAGMENT_LOCAL)); nit: indent 4. PS14, Line 278: was nit: is PS14, Line 289: == Should this be <= ? It won't overflow if it's StringVal::MAX_LENGTH - 1 + StringVal::MAX_LENGTH <= std::numeric_limits<decltype(buffer_space)>::max(), right ? PS14, Line 352: UNLIKELY(resized == false) nit: !resized. May be easier to just do: if (!result.Resize(...)) return StringVal::null(); http://gerrit.cloudera.org:8080/#/c/5776/14/be/src/udf/udf.h File be/src/udf/udf.h: Line 583: /// Reallocate a StringVal so that it as at least as large as len. May shrink or May help to clarify that the string being resized needs to be allocated from StringVal(FunctionContext* context, int len); Memory allocated from that interface is considered memory managed by Impala instead of the UDF/UDA (aka local allocations). In other words, one shouldn't call this function on a string allocated via FunctionContext::Allocate() at the init() of a UDA. This may result in double free. Sorry, our UDF memory management story is rather messy. Line 587: /// If the resize fails, the original StringVal remains in place. and returns false. Returns true otherwise. http://gerrit.cloudera.org:8080/#/c/5776/14/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: PS14, Line 1942: select sum(length(replace(y, x, 'bbbbbbbbbb'))) from (select cast(round( : float_col) AS STRING) as x, string_col as y from functional.alltypes) v; This seems rather similar to the previous query ? How about we test the case in which the replace string is a the string_col instead ? select sum(length(replace(y, '0', x))) from (select cast(round( float_col) AS STRING) as x, string_col as y from functional.alltypes) v; -- 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: 14 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
