Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-2736: Basic column-wise slot materialization in Parquet scanner. ......................................................................
Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/2779/6/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 1164: pool->AcquireData(decompressed_data_pool_.get(), false); Personally, I think it's more clear if you change this to scratch_batch_->mem_pool() instead of plumbing pool around. When I initially read this, my immediate question was, "Which pool is this?" and then I had to trace this all the way back up to AssembleRows(). Going the other way, it's not obvious from the ReadValueBatch() signature what the is used for since it happens so far down the callstack (although you could say it in the comment I suppose). Your call. Line 1754: int num_null_tuples = min(batch_->capacity() - batch_->num_rows(), nit: I would just call this "num_tuples", they're technically NULL but that's kind of a bug. Line 1897: if (column_readers[0]->RowGroupAtEnd()) break; nit: I think this should go in the while loop because it describes the logic more clearly. If anything continue_execution seems like the exceptional case that could go in a break statement, e.g.: while not at end of row group: read some rows if (UNLIKELY(!continue_execution)) break; -- To view, visit http://gerrit.cloudera.org:8080/2779 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I72a613fa805c542e39df20588fb25c57b5f139aa Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
