Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17374 )
Change subject: IMPALA-10691: Fix multithreading related crash in CAST FORMAT ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/17374/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17374/1//COMMIT_MSG@12 PS1, Line 12: : > This also fixes the following query with MT_DOP=0: Rewritten the comment to avoid false information. http://gerrit.cloudera.org:8080/#/c/17374/1/be/src/exprs/cast-format-expr.cc File be/src/exprs/cast-format-expr.cc: http://gerrit.cloudera.org:8080/#/c/17374/1/be/src/exprs/cast-format-expr.cc@46 PS1, Line 46: return Status(TErrorCode::INTERNAL_ERROR, fn_ctx->error_msg()); > We can get a memory leak here. Done http://gerrit.cloudera.org:8080/#/c/17374/3/be/src/exprs/cast-format-expr.cc File be/src/exprs/cast-format-expr.cc: http://gerrit.cloudera.org:8080/#/c/17374/3/be/src/exprs/cast-format-expr.cc@38 PS3, Line 38: std::make_unique<DateTimeFormatContext>(format_.c_str(), format_.length()); > optional: You can even track the allocation in the function context if you I prefer to do simply on the heap, because in my "mental model" FunctionContext's Allocate belongs to per/row allocations. It would be also a bit misleading because the allocations inside DateTimeFormatContext would still occur untracked on the heap, while it would look like we do tell FunctionContext the amount of memory we consumed. In my opinion the best would be to add another functions to Expr that will be called only once in its lifetime, e.g. Prepare, and do this kind of processing there. The current model of FRAGMENT_LOCAL/THREAD_LOCAL is not intuitive since we are running multiple instences of fragments in parallel. -- To view, visit http://gerrit.cloudera.org:8080/17374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I501c8a184591b1c836b2ca4cada1f2117f9f5c99 Gerrit-Change-Number: 17374 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 30 Apr 2021 15:56:23 +0000 Gerrit-HasComments: Yes
