Henry Robinson has posted comments on this change. Change subject: IMPALA-3007: Adjust Bloom Filter size according to NDV estimate ......................................................................
Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/2812/1/be/src/exec/hash-join-node.cc File be/src/exec/hash-join-node.cc: Line 230: hash_tbl_->AddBloomFilters(); > huh? Could you be a bit more descriptive? :) All the logic for checking FP rates has gone into AddBloomFilters(). http://gerrit.cloudera.org:8080/#/c/2812/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 156: uint32_t log_space = state->filter_bank()->GetLogSpaceForNdv(filter.ndv_estimate); > why not have a GetFilterByteSize() or something like that. the scan node sh Done. http://gerrit.cloudera.org:8080/#/c/2812/1/be/src/exec/old-hash-table.cc File be/src/exec/old-hash-table.cc: Line 148: filters_[i]->filter_desc().filter_id); > this takes the capacity, not an id. Done http://gerrit.cloudera.org:8080/#/c/2812/1/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 500: state->filter_bank()->FpRateTooHigh(ndv_estimate, total_build_rows); > just as an aside: instead of looking at build rows, which is indirect, why Done http://gerrit.cloudera.org:8080/#/c/2812/1/be/src/runtime/runtime-filter.cc File be/src/runtime/runtime-filter.cc: Line 171: uint64_t required_space = > let's not use unsigned ints Done http://gerrit.cloudera.org:8080/#/c/2812/1/be/src/runtime/runtime-filter.h File be/src/runtime/runtime-filter.h: Line 77: /// expected false-positive rate would be larger than allowed by > "a filter's expected false-positive rate would exceed flags_max_filter_erro Done Line 79: bool FpRateTooHigh(uint64_t expected_ndv, uint64_t observed_ndv); > role of expected_ndv unclear Done Line 94: BloomFilter* AllocateScratchBloomFilter(int64_t ndv_estimate); > instead of continuing to talk about ndv and estimates, which are fe concept Why do you feel the NDV is a FE-only concept? In my opinion it's the key parameter to determining the size of the BF. http://gerrit.cloudera.org:8080/#/c/2812/1/fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java File fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java: Line 415: filter.computeNdvEstimate(); > this also needs to happen for repartitioning joins Done (as a result of the previous patch which refactored this method). http://gerrit.cloudera.org:8080/#/c/2812/1/fe/src/main/java/com/cloudera/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/com/cloudera/impala/planner/RuntimeFilterGenerator.java: Line 113: // Estimate of the number of distinct values that will be inserted into this filter. > explain meaning of -1 Done. Even for repartitioning joins we want the total NDV across all instances, since the filters will be merged. http://gerrit.cloudera.org:8080/#/c/2812/1/testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test File testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test: Line 253: # Test case 11: filters with high expected FP rate get disabled. > what does "expected" mean here? That the rate of false-positives when probing the filter is expected to be high. The actual FP-rate is irrelevant. http://gerrit.cloudera.org:8080/#/c/2812/1/testdata/workloads/functional-query/queries/QueryTest/runtime_filters_wait.test File testdata/workloads/functional-query/queries/QueryTest/runtime_filters_wait.test: Line 37: row_regex: .*0 of 1 Runtime Filters Produced.* > i'm not sure about this error message, it makes it sound like something wen What would you prefer? In a certain sense, something did go wrong (we guessed the right size for a filter, and it was enough of an underestimate that the filter was disabled). -- To view, visit http://gerrit.cloudera.org:8080/2812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1fe37b8d4cfb3c52bb8e8cf0ca55e92665b87803 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-HasComments: Yes
