Alex Behm has posted comments on this change. Change subject: IMPALA-3905: Add single-threaded scan node. ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/4113/3/be/src/exec/hdfs-avro-scanner.cc File be/src/exec/hdfs-avro-scanner.cc: Line 768: Function* HdfsAvroScanner::CodegenMaterializeTuple( > It looks like you rebased onto an older commit somehow, I'm seeing a lot of I rebased to a newer commit that includes the Parquet scanner codegen. Thought it would be better for you to see all changes. I might have to rebase again, looks like there are more conflicts :( http://gerrit.cloudera.org:8080/#/c/4113/3/be/src/exec/hdfs-scan-node-mt.cc File be/src/exec/hdfs-scan-node-mt.cc: PS3, Line 48: !files.second.empty() > Do we have test coverage for this case? We currently don't have any automated test coverage for the new scan node. Let's discuss with Marcel what to do about that. Line 72: if (scan_range_ == NULL || scanner_->eos()) { > Is the invariant that scan_range_ != NULL implies scanner_ != NULL? I think Correct, Added a DCHECK. When eos is returned both scan_range_ and scanner_ should be NULL, so the invariant is still true after returning eos. PS3, Line 73: .get() > the .get() isn't necessary (NULL comparisons against scoped_ptr work) Done http://gerrit.cloudera.org:8080/#/c/4113/3/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 196: SCOPED_TIMER(runtime_profile_->total_time_counter()); > I think it would be better to consistently follow the rule that the bottom- Done http://gerrit.cloudera.org:8080/#/c/4113/3/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: Line 131: /// TODO: The code at callers could be simplified if eos was instead a member > Stale comment? Yes. Removed. http://gerrit.cloudera.org:8080/#/c/4113/3/be/src/exec/scanner-context.h File be/src/exec/scanner-context.h: PS3, Line 298: .. > extra . Done -- To view, visit http://gerrit.cloudera.org:8080/4113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I98cc7f970e1575dd83875609985e1877ada3d5e0 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
