Tim Armstrong has posted comments on this change. Change subject: IMPALA-3905: Add HdfsScanner::GetNext() interface and implementation for Parquet. ......................................................................
Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/3732/2/be/src/exec/base-sequence-scanner.cc File be/src/exec/base-sequence-scanner.cc: Line 139: scan_node_->AddDiskIoRanges(desc); > Sorry we don't need the change in this patch. We'll eventually need it but I'm ok with making this change, I just didn't understand it :). Whichever way is easiest to stage it is ok by me. http://gerrit.cloudera.org:8080/#/c/3732/2/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 139: advance_row_group_(true), > This needs to start out as true to move to the very first row group. It's e I think that makes sense to me. Line 354: FlushFinal(row_batch); > I renamed the lengthy function to save some chars. Hopefully looks a little LGTM http://gerrit.cloudera.org:8080/#/c/3732/3/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 302: assemble_rows_timer_ = &assemble_rows_timer; I think we should avoid putting a pointer to the stack-allocated timer into a member like this, it's too easy to mess up the lifecycle in a hard-to-debug way. I'm ok with going back to the old approach since there wasn't an easy way to make it a scoped timer. http://gerrit.cloudera.org:8080/#/c/3732/3/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: PS3, Line 373: CreateAndPrepareScanner This should really be CreateAndOpenScanner() now. http://gerrit.cloudera.org:8080/#/c/3732/2/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: Line 114: > Correct, the guarantees we need to provide here are slightly different from Right, I think the only difference from the ExecNode is that scanners are not allowed to use MarkNeedToReturn(). If you don't use that in an ExecNode the assumption is that the resource is either attached to a trailing batch or valid until Close(). Anyway, I think the new wording is good. PS2, Line 115: anner's sp > I don't think it makes much difference in this patch. I'll keep it in mind Ok by me. http://gerrit.cloudera.org:8080/#/c/3732/3/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: Line 69: /// TODO(Alex): Change comments. ? -- To view, visit http://gerrit.cloudera.org:8080/3732 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iab50770bac05afcda4d3404fb4f53a0104931eb0 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
