Tim Armstrong has posted comments on this change. Change subject: IMPALA-3286: prefetching for PartitionedAggregationNode ......................................................................
Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/3070/8/be/src/exec/partitioned-aggregation-node-ir.cc File be/src/exec/partitioned-aggregation-node-ir.cc: Line 73: HashTable* hash_tbl = hash_tbls_[partition_idx]; > Maybe add the DCHECK here that verifies hash_tbls_ is in sync. You could ev Done. Added GetHashTable. Line 92: if (expr_vals_cache->IsRowNull()) return Status::OK(); > Is it intentional to put this if-stmt after fetching the hash value in the Yes, same reason as in the other place - to speed up the non-null branch. Line 182: !expr_vals_cache->IsRowNull()) { : if (!TryAddToHashTable(ht_ > Can you please combine these two if-stmts ? Done Line 186: RETURN_IF_ERROR(process_batch_status_) > So, if we succeed in adding to the hash table, process_batch_status_->ok() Done Line 214: Partition* __restrict__ partition, : HashTable* __restrict__ hash_tbl, > Do we need to pass these two parameters ? Can they be local to this functio We could alternatively pass in the partition index, which might save an argument, but it seems cleaner to me if the partitioning is handled by the caller. http://gerrit.cloudera.org:8080/#/c/3070/8/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 282: Function* codegen_process_batch_streaming_fn; : codegen_status = CodegenProcessBatchStreaming(&codegen_process_batch_streaming_fn); : if (codegen_status.ok()) { : codegen->AddFunctionToJit(codegen_process_batch_streaming_fn, : reinterpret_cast<void**>(&process_batch_streaming_fn_)); : codegen_enabled = true; > Why cannot these be placed in CodegenProcessBatchStreaming() similar to how Done. Makes sense, I'd never looked at restructuring it. Line 293: void **codegened_fn_ptr = grouping_expr_ctxs_.empty() ? : reinterpret_cast<void**>(&process_batch_no_grouping_fn_) : : reinterpret_cast<void**>(&process_batch_fn_); : codegen->AddFunctionToJit(codegen_process_batch_fn, codegened_fn_ptr); > Why cannot these be placed in CodegenProcessBatch() similar to how we handl Done http://gerrit.cloudera.org:8080/#/c/3070/8/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: Line 494: 'ht_ctx'->expr_value_buffer_array_' > This probably needs to be updated after rebase. 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: 8 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
