Dan Hecht has posted comments on this change. Change subject: IMPALA-2399: Check for mem limit in allocations in parquet scanner and decompressor ......................................................................
Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/2203/12/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 1569: RETURN_IF_ERROR(state_->CheckQueryState()); What is this check needed for? Is it just for UDF errors? The memtracker check is no longer necessary right? So, can we make this just GetQueryStatus() (which will eventually become GetUDFStatus())? Maybe we should even introdue GetUDFStatus() now to make it easier to keep track of which places have been dealt with (in terms of mem-trackers), and then once all calls to GetQueryState and GetQueryStatus are gone we can delete those functions. http://gerrit.cloudera.org:8080/#/c/2203/12/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 88: // most format specific scanners expect batch_ != NULL in their Close() functions. but this assumes StartNewRowBatch() will always set a batch_ even if returns error, which seems like a fragile assumption. is the batch_ needed just because of the AddFinalRowBatch() call? if so, maybe we should just turn that DCHECK into an if-stmt? Line 196: // TODO: move this to ProcessSplit() / ProcessRange(). This check eventually will be needed only for UDF status, right? So wouldn't it make sense to keep it together with FreeLocalAllocations()? (whether that's here or somewhere else). http://gerrit.cloudera.org:8080/#/c/2203/12/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 290: // cases where we use Status::CANCELLED to indicate that the limit was reached. I don't think we want to do this TODO, right? how about we delete it? Line 293: if (UNLIKELY(!query_status_.ok())) return GetQueryStatus(); should this if-stmt go in GetQueryStatus() so that direct callers also benefit? -- To view, visit http://gerrit.cloudera.org:8080/2203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic70400407b7662999332448f4d1bce2cc344ca89 Gerrit-PatchSet: 12 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
