Henry Robinson has posted comments on this change. Change subject: IMPALA-3077: Enable runtime filters when PHJ spills ......................................................................
Patch Set 1: (8 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. huh, I didn't think we could use C++11 in cross-compiled code. But it works! Looks like LLVM had range-based for from v3.0, and it doesn't require -std=c++11 to enable. Line 279: void* e = ctx.expr->GetValue(build_row); > We should make sure we unroll this loop and replace types at some point - m Good idea. IMPALA-3360. Line 280: h > Nit: Maybe get rid of single letter var names carried over from the existin Done 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 Done. I added a DCHECK to confirm that only n_a_p_ is only set if the type is NALAJ. Line 1602: - > _ Done 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 t Done 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? Can't hurt to increase it. Line 269: row_regex: .*SpilledPartitions: 176 .* > I think it would be best to just check that # of spilled partitions is non- Done -- 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: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
