Tim Armstrong has posted comments on this change. Change subject: IMPALA-3905: Add single-threaded scan node. ......................................................................
Patch Set 3: (10 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 unrelated changes in the diff. http://gerrit.cloudera.org:8080/#/c/4113/1/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: Line 69: > Not following the first part (space where?). I was thinking there should be a newline (comment wasn't clear) but it's no longer relevant. 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? Line 72: if (scan_range_ == NULL || scanner_->eos()) { Is the invariant that scan_range_ != NULL implies scanner_ != NULL? I think that's true before eos/error has been returned (and callers shouldn't be calling GetNext() after eos). Would be good to add a DCHECK to document the invariant. PS3, Line 73: .get() the .get() isn't necessary (NULL comparisons against scoped_ptr work) 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-most class in the hierarchy tracks the timing, rather than having a timer at multiple levels of the class hierarchy. We already do this, e.g. ExecNode::Open() doesn't have any timers. http://gerrit.cloudera.org:8080/#/c/4113/1/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: Line 208: /// Clones of the conjuncts ExprContexts in scan_node_->conjuncts_map(). Each scanner > Commented on lifetime. Yeah I think this may be the least of the evils at the moment- it doesn't seem like there's an easy simplification here. Line 270: > Added a TODO, hope that's ok for now. Yeah, definitely don't need to fix it now. 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? 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 . -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
