impala-reviews 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-4285/IMPALA-4286: Fixes for Parquet scanner with MT DOP > 0.
Date Fri, 21 Oct 2016 14:40:28 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4285/IMPALA-4286: Fixes for Parquet scanner with MT_DOP > 0.
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4767/4//COMMIT_MSG
Commit Message:

Line 17: HdfsScanNodeMt::GetNext().
> why do we have this check in HdfsScanNodeBase::Open() in the first place?
Removed the short-circuit in Open() instead.


http://gerrit.cloudera.org:8080/#/c/4767/4/be/src/exec/hdfs-scan-node-mt.cc
File be/src/exec/hdfs-scan-node-mt.cc:

PS4, Line 71: ,
> did you mean to say more or make it a period?
I removed the short-circuit in Open() instead.


Line 72:   if (file_descs_.empty()) {
> this is okay, but why can't we just delete the equivalent check in HdfsScan
No, removed it.


http://gerrit.cloudera.org:8080/#/c/4767/4/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

PS4, Line 230: row
> "row" seems misleading
Not relevant anymore, I also removed the conjunct eval. Tests passed.


PS4, Line 230: an
> and
Done


Line 236:   // Set remaining tuples in row.
> misleading comment
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4767
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79c0f6fd2aeb4bc6fa5f87219a485194fef2db1b
Gerrit-PatchSet: 4
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: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message