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 HdfsScanner::GetNext() interface and implementation for Parquet.
Date Thu, 28 Jul 2016 18:46:56 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-3905: Add HdfsScanner::GetNext() interface and implementation for Parquet.
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3801/1/be/src/exec/hdfs-scanner.h
File be/src/exec/hdfs-scanner.h:

Line 119:   /// the batch is reset or destroyed, or a batch returned from a subsequent GetNext()
> more succinctly: or this or any subsequently returned batch is reset or des
Done


Line 137:   virtual void Close(RowBatch* row_batch, bool add_to_queue);
> on second thought, add_to_queue is a bit odd (what if you never called Proc
In that case, I think it's cleaner to declare the intent in the c'tor because batch() and
Close() can be called before even calling ProcessSplit(), e.g., if Open() fails.

Added the c'tor param.


Line 139:   RowBatch* batch() const { return batch_; }
> is this valid in GetNext mode? if not, add the above mentioned flags.
Added comment and DCHECK.


http://gerrit.cloudera.org:8080/#/c/3801/1/be/src/exec/hdfs-sequence-scanner.h
File be/src/exec/hdfs-sequence-scanner.h:

Line 165:   
> fix all trailing spaces
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iab50770bac05afcda4d3404fb4f53a0104931eb0
Gerrit-PatchSet: 1
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: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message