Michael Ho has posted comments on this change. Change subject: IMPALA-3286: prefetching for PartitionedAggregationNode ......................................................................
Patch Set 8: (7 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 92: if (expr_vals_cache->IsRowNull()) return Status::OK(); Is it intentional to put this if-stmt after fetching the hash value in the line above ? Line 182: !expr_vals_cache->IsRowNull()) { : if (!TryAddToHashTable(ht_ Can you please combine these two if-stmts ? Line 186: RETURN_IF_ERROR(process_batch_status_) So, if we succeed in adding to the hash table, process_batch_status_->ok() should be TRUE, right ? If so, can we have a DCHECK for it (may be at line 200 below) ?! Line 214: Partition* __restrict__ partition, : HashTable* __restrict__ hash_tbl, Do we need to pass these two parameters ? Can they be local to this function ? 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 we handle codegen for PHJ ? 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 handle codegen for PHJ ? 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. -- 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
