Tim Armstrong has posted comments on this change. Change subject: IMPALA-4539: fix bug when scratch batch references I/O buffers ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/5406/2/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: PS2, Line 245: > What do you mean by wrong point ? I just meant that there were certain states where it could trigger the bug - specifically if GetNextInternal() returns before the whole scratch batch is empty. That phrase is removed anyway. http://gerrit.cloudera.org:8080/#/c/5406/3/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: PS3, Line 601: (dst_batch->AtCapacity() || context_->num_completed_io_buffers() > 0) > Do we still need these conditions ? I suppose FlushRowGroupResources() will I don't believe they're necessary for correctness, but I'd have to think it through more. I think it's fine to attach the resources at any point once we've copied the tuples. It can save us iterating over all the streams to check them, but it's unclear how important that is. -- To view, visit http://gerrit.cloudera.org:8080/5406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic27e7251e0f633cb694b506f6eb62beed6e66ad9 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
