Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2736: Basic column-wise slot materialization in Parquet scanner. ......................................................................
Patch Set 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/2779/8/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 329: uint8_t* tuple_mem, int* num_values) { describe role of tuple_mem Line 369: bool ReadValueBatch(MemPool* pool, int max_values, int tuple_size, instead of having this be templated, why not inline the code at the call sites and remove the template param. there are only 8 lines of shared code, which isn't worth the extra indirection and complication through the template parameter. also, the function call overhead should be negligible in this case, so you can move this to the .cc file. Line 708: virtual bool ReadValue(MemPool* pool, Tuple* tuple) { do we still need this? Line 723: return ReadValueBatch<false>(pool, max_values, tuple_size, tuple_mem, num_values); same comment about inlining the call of the templated function applies here Line 755: template<bool IN_COLLECTION> since we're now dealing with batches, it would be nice to get rid of the template parameters. Line 1722: // This function most not be called when the output batch is already full. As long as must Line 1797: ++stats->total_possible; todo: do these stats updates outside the loop if possible, as in stats->total_possible += num_scratch_tuples Line 1801: // producing an output batch. good idea Line 1874: continue_execution = col_reader->ReadNonRepeatedValueBatch( is there a more graceful name? ReadScalarValueBatch? TopLevelValueBatch? http://gerrit.cloudera.org:8080/#/c/2779/8/be/src/exec/hdfs-parquet-scanner.h File be/src/exec/hdfs-parquet-scanner.h: Line 484: template <bool IN_COLLECTION> the comment says top-level tuples, how can in_collection be true? (or: what does in_collection mean?) -- 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: 8 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
