impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Skye Wanderman-Milne (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-2736: Basic column-wise slot materialization in Parquet scanner.
Date Fri, 22 Apr 2016 22:49:38 GMT
Skye Wanderman-Milne has posted comments on this change.

Change subject: IMPALA-2736: Basic column-wise slot materialization in Parquet scanner.

Patch Set 6:

File be/src/exec/

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,

while not at end of row group:
  read some rows
  if (UNLIKELY(!continue_execution)) break;

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I72a613fa805c542e39df20588fb25c57b5f139aa
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Skye Wanderman-Milne <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message