Tim Armstrong has posted comments on this change. Change subject: IMPALA-3286: prefetching for PartitionedAggregationNode ......................................................................
Patch Set 11: (14 comments) http://gerrit.cloudera.org:8080/#/c/3070/11/be/src/exec/partitioned-aggregation-node-ir.cc File be/src/exec/partitioned-aggregation-node-ir.cc: Line 46 I made a mistake here caught by running the full test suite: we need to call this from both callsites of ProcessBatch(). I don't think this change was especially important, so I reverted it. Line 48: } > DCHECK(expr_vals_cache->AtEnd()) ? Done Line 200: } > DCHECK(expr_vals_cache->AtEnd()) ? Done http://gerrit.cloudera.org:8080/#/c/3070/11/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 278: LlvmCodeGen* codegen; : RETURN_IF_ERROR(state->GetCodegen(&codegen)); > Not needed in this function anymore ? Good point. Line 282: codegen_enabled = codegen_status.ok(); > This line and line 285 can be probably be merged. Done Line 343: // There is grouping, so we will do partitioned aggregation. > This line can be moved to line 339 instead as this should not be specific t Done Line 1412: hash_partitions_.clear(); > May be we should reset hash_tbls_[] here too ? Done. I think the consistency DCHECKS are sufficient to catch any bugs with hash_tbls_ being out of sync with hash_partitions_, but this seems cleaner. Line 1830: ConstantInt::get(Type::getInt32Ty(codegen->context()), prefetch_mode)); > not this change, but this is probably a common enough pattern that it's wor Probably a good idea. http://gerrit.cloudera.org:8080/#/c/3070/11/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: Line 232: > nit: extra space. Same below. Done Line 400: hash_tbls_ > nit: 'hash_tbls_' Done Line 499: stores the results > Can you please mention the null bool and hash values are also cached ? Explaining that here seems redundant, given that those are always cached in the ExprValueCache as part of evaluation. The rewording might make it a bit clearer that it's using the ExprValuesCache. Line 500: 'ht_ctx'. > the expression values cache in 'ht_ctx'. Done Line 501: > nit: extra space. Done Line 581: HashTable* hash_tbl > Can you please add a comment about this new parameter ? Done -- To view, visit http://gerrit.cloudera.org:8080/3070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7726454efb416d61080c4e11db0ee7ada18c149b Gerrit-PatchSet: 11 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
