[Impala-CR](cdh5-trunk) Optimized ReadValueBatch() for Parquet scalar column readers.

["Alex Behm (Code Review)" <ge...@cloudera.org>]

Alex Behm has posted comments on this change.

Change subject: Optimized ReadValueBatch() for Parquet scalar column readers.
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/2843/2/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

Line 276:   /// Decodes and caches the next batch of levels. Resets members associated with the cache.
> Is it valid to call this when the prev batch hasn't been totally consumed?
It's valid, but doesn't really make sense.


Line 301:   uint8_t* cached_levels_;
> Would it make sense to just have this be a constant-sized array with e.g. 1
Good question. Had thought about it and opted for the safer solution. For large scans the amount of untracked memory could be in the 10s of MBs (per query). Do you think it's acceptable/preferable to leave that untracked?


Line 854:   /// It assumes a data page with remaining values is available, and that the def/rep
> Can we assert any of these preconditions with DCHECKs?
Good point. Done.


Line 1872:       if (col_reader->IsCollectionReader() || col_reader->IsBoolColumnReader()) {
> Maybe should have a NeedsSeeding() method?
I added one, but not sure if it's clearer/better.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I21fa9b050a45f2dd45cc0091ea5b008d3c0a3f30
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes