impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
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:

File be/src/exec/

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 :(
File be/src/exec/

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)
File be/src/exec/

Line 196:   SCOPED_TIMER(runtime_profile_->total_time_counter());
> I think it would be better to consistently follow the rule that the bottom-
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.
File be/src/exec/scanner-context.h:

PS3, Line 298: ..
> extra .

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I98cc7f970e1575dd83875609985e1877ada3d5e0
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message