Dan Hecht has posted comments on this change. Change subject: IMPALA-3286: prefetching for PartitionedAggregationNode ......................................................................
Patch Set 5: (8 comments) Please have Mostafa review the perf results (could do that here in the code review to keep the discussion in one place). http://gerrit.cloudera.org:8080/#/c/3070/5/be/src/exec/partitioned-aggregation-node-ir.cc File be/src/exec/partitioned-aggregation-node-ir.cc: Line 50: std::min<int>(batch->num_rows() doesn't the FOREACH_ROW_LIMIT take care of this? besides, it doesn't really make sense if cache capacity is not a multiple of batch num_rows (we need to limit the iteration on the last group anyway). Line 50: batch_size cache_size Line 64: Batch Since we're not doing the whole batch, maybe rename this to something like EvalAndHashPrefetchGroup()? I think that's what the join patch is callnig it. Line 68: batch_size cache_size Line 79: // Hoist lookups out of non-null branch to speed up non-null case. why does this speed it up? Line 182: const int batch_size = std::min<int>(num_rows, expr_vals_cache->capacity()); same comments http://gerrit.cloudera.org:8080/#/c/3070/5/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: Line 490: free space available capacity of (i.e. the cache always starts empty). Line 558: /// that 'prefetch_mode' will be substituted with constants during codegen time. maybe move after needs_serialize to keep in order -- 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: 5 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
