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 15: (2 comments) Just one more thing about the DCHECK. lmk what you think. http://gerrit.cloudera.org:8080/#/c/2203/15/be/src/exec/base-sequence-scanner.cc File be/src/exec/base-sequence-scanner.cc: Line 98: Can happen if Prepare() failed. one way to interpret this comment is that a resource leak can happen if Prepare() fails. But I think you mean to say if prepare fails, then batch_ can be NULL. How about: If batch_ == NULL (can happen if Prepare() failed), then verify there are no resources that need to be transferred. But actually, wouldn't it be correct to have these DCHECKs be after the if-stmt (i.e. run them regardless of whether batch_ was null or not, after the transfer is done)? that would verify that either there were no resources to transfer, or that we had a row-batch to transfer them to and the transfer worked as expected. Then, the comment would simply be: // Verify resources have been transferred. http://gerrit.cloudera.org:8080/#/c/2203/15/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 165: static_cast<int64_t> this cast shouldn't be needed - converting from smaller to larger type just use implicit conversion. or are you seeing warnings? -- 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: 15 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
