Tim Armstrong has posted comments on this change. Change subject: IMPALA-3077: Enable runtime filters when PHJ spills ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/2783/1/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 277: BOOST_FOREACH Nit: Could use c++11 for loop syntax. Line 279: void* e = ctx.expr->GetValue(build_row); We should make sure we unroll this loop and replace types at some point - maybe file a JIRA if there isn't already one since it'll be easy to forget about? Line 280: h Nit: Maybe get rid of single letter var names carried over from the existing code. http://gerrit.cloudera.org:8080/#/c/2783/1/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 484: DCHECK(null_aware_partition_ == NULL) I think it'd be clearer to check for NALAJ directly if that's the invariant. Line 650: if (process_build_batch_fn_ == NULL) { Oh wow, how did we miss that - nice catch. Line 1602: - _ http://gerrit.cloudera.org:8080/#/c/2783/1/be/src/exec/partitioned-hash-join-node.h File be/src/exec/partitioned-hash-join-node.h: Line 166: ProcessBuildBatchWithFilters Is the rename necessary? I think ProcessBuildBatch is fine and shorter if there's no need to disambiguate. http://gerrit.cloudera.org:8080/#/c/2783/1/testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test File testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test: Line 259: 15000 Is this enough e.g. for ASAN and code coverage? Line 269: row_regex: .*SpilledPartitions: 176 .* I think it would be best to just check that # of spilled partitions is non-zero so that we don't silently lose coverage. Exact # of spilled partitions will depend on a lot of factors. -- To view, visit http://gerrit.cloudera.org:8080/2783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59a2d9ee03ccea6b674392584e4c7f272233571e Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
