impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3905: Add single-threaded scan node.
Date Tue, 30 Aug 2016 22:27:40 GMT
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.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message