[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Reviewed-on: http://gerrit.cloudera.org:8080/8971 Reviewed-by: Bikramjeet VigTested-by: Impala Public Jenkins --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 902 insertions(+), 550 deletions(-) Approvals: Bikramjeet Vig: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 14 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 13: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 13 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 13 Feb 2018 08:29:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#13). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 902 insertions(+), 550 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/13 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 13 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 12: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1921/ -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 12 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 13 Feb 2018 01:04:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 12: Code-Review+2 rebased and fixed conflicts. Carrying over Tim's and Dimitris's +2 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 12 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 08 Feb 2018 00:47:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#12). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 900 insertions(+), 550 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/12 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 12 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 11 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 07 Feb 2018 22:11:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@262 PS10, Line 262: .build() > Shouldn't build be called last? i need to do build here because the sum() method is a part of ResourceProfile class and not the builder. The builder class does not have a way of adding another ResourceProfile obj. http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@125 PS10, Line 125: ong maxLimit = BitUtil.roundUpToPowerOf2(tQueryOptions : .getRuntime_filter_max_size()); : long minBufferSize = BitUtil.roundUpToPowerOf2(BackendConfig.INSTANCE : .getMinBufferSize()); > You're calling roundUpToPoweOf2 multiple times while you only need to do it Done http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@136 PS10, Line 136: > // Make sure minVal <= defaultVal <= maxVal Done -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 10 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 07 Feb 2018 22:09:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#11). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 901 insertions(+), 549 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/11 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 11 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@262 PS10, Line 262: .build() Shouldn't build be called last? http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@125 PS10, Line 125: ong maxLimit = BitUtil.roundUpToPowerOf2(tQueryOptions : .getRuntime_filter_max_size()); : long minBufferSize = BitUtil.roundUpToPowerOf2(BackendConfig.INSTANCE : .getMinBufferSize()); You're calling roundUpToPoweOf2 multiple times while you only need to do it once. I think it may be simpler to do: maxVal = BitUtil.roundUpToPowerOf2(Math.max(maxLimit, minBufferSize)); http://gerrit.cloudera.org:8080/#/c/8971/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@136 PS10, Line 136: // Make sure minVal <= defaultVal <= maxVal -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 10 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 07 Feb 2018 19:20:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 9: (8 comments) http://gerrit.cloudera.org:8080/#/c/8971/9/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8971/9/be/src/service/query-options.cc@380 PS9, Line 380:"buffer size that can be allocated by the buffer pool", > nit: indentation Done http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@75 PS9, Line 75: max > Shouldn't this be the "min buffer size"? Done http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@108 PS9, Line 108: private class FilterSizeLimits { > Add a small class comment Done http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@112 PS9, Line 112: BitUtil.roundUpToPowerOf2( > Why do you need to do it here and not when the final 'max' value is compute I did that just make the final assignments look cleaner in code, otherwise it would look something like this: max = BitUtil.roundUpToPowerOf2(Math.max((tQueryOptions.getRuntime_filter_max_size(), BackendConfig.INSTANCE.getMinBufferSize())); or I could bring it down to : max = BitUtil.roundUpToPowerOf2(Math.max(maxLimit, minBufferSize)); http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@116 PS9, Line 116: // TODO: what happens if minbuffersize is greater than the MAX_BLOOM_FILTER_SIZE? > What do we know about this TODO? Can you check with Tim? oops forgot to remove that one. already talked to tim about that http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@123 PS9, Line 123: Preconditions.checkArgument(minLimit >= MIN_BLOOM_FILTER_SIZE); : Preconditions.checkArgument(minLimit <= MAX_BLOOM_FILTER_SIZE); > Hm, I know I asked you to add these but they do seem kind of redundant if w Done http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@134 PS9, Line 134: > nit: extra white space Done http://gerrit.cloudera.org:8080/#/c/8971/9/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@135 PS9, Line 135: // Maximum filter size, in bytes, rounded up to a power of two. : public final long max; : : // Minimum filter size, in bytes, rounded up to a power of two. : public final long min; : : // Pre-computed default filter size, in bytes, rounded up to a power of two. : public final long defaultVal; > nit: move them to the beginning of the class (Java style :)). Also, you cou I was thinking the same (to rename to maxSize). Lemme go ahead and do that -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 9 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 06 Feb 2018 23:05:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/8971/9/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8971/9/be/src/service/query-options.cc@376 PS9, Line 376: // last condition is to unblock the highly improbable case where the : // min_buffer_size is greater than RuntimeFilterBank::MAX_BLOOM_FILTER_SIZE. : && FLAGS_min_buffer_size <= RuntimeFilterBank::MAX_BLOOM_FILTER_SIZE pretty ugly workaround, but had to put this so it doesn't block the query for this highly improbable case. Any comments? -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 9 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 03 Feb 2018 00:21:22 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 8: (5 comments) > (5 comments) > > Did you look at the tests that Alex mentioned for creating a test > table with different cardinalities and nvds? initially we discussed including those tests to check if the limits are being enforced in case the size calculated from ndvs is outside those limits. It turns out that we infact are already testing that code path when we check if the query limits are enforced (in bloom_filters.test). Also, since we are taking care of enforcing the hard limits({MIN,MAX}_BLOOM_FILTER_SIZE) on the query options itself, we dont need to write any tests with mock table stats anymore. http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@95 PS8, Line 95: private static final long MIN_BLOOM_FILTER_SIZE = 4 * 1024; : private static final long MAX_BLOOM_FILTER_SIZE = 512 * 1024 * 1024; > You may remove them and add a check instead. Done http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@109 PS8, Line 109: _ > nit: no need for '_' when the fields are public Done http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@125 PS8, Line 125: bloomFilterSizeLimits_.max_ = tQueryOptions.getRuntime_filter_max_size(); : bloomFilterSizeLimits_.max_ = Math.max(bloomFilterSizeLimits_.max_, : MIN_BLOOM_FILTER_SIZE); : bloomFilterSizeLimits_.max_ = Math.max(bloomFilterSizeLimits_.max_, : BackendConfig.INSTANCE.getMinBufferSize()); : bloomFilterSizeLimits_.max_ = Math.min( : BitUtil.roundUpToPowerOf2(bloomFilterSizeLimits_.max_), MAX_BLOOM_FILTER_SIZE); > You may want to see if it makes sense to add this logic to FilterSizeLimits I moved this whole part into the FilterSizeLimits ctor, since i need to retain the order in which these are set. http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@482 PS8, Line 482: sizeLimits > 'filterSizeLimits' Done http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@494 PS8, Line 494: filterSizeBytes_ = Math.min(filterSizeBytes_, filterSizeLimits.max_); > There is always the case that the size needed to achieve the desired fp rat I would not recommend logging it here because although this is a valid case, it still does not guarantee that the filter will be disabled, as the disabling logic uses the actual ndv to check for fpp during query execution. Also, there might be a lot of runtime filters generated here but might not end up in the plan eventually, so the log might be filled with extra messages. -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 03 Feb 2018 00:18:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#9). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 906 insertions(+), 549 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/9 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 9 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 8: (5 comments) Did you look at the tests that Alex mentioned for creating a test table with different cardinalities and nvds? http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@95 PS8, Line 95: private static final long MIN_BLOOM_FILTER_SIZE = 4 * 1024; : private static final long MAX_BLOOM_FILTER_SIZE = 512 * 1024 * 1024; > the query option restrictions already take care of this limit, by making su You may remove them and add a check instead. http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@109 PS8, Line 109: _ nit: no need for '_' when the fields are public http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@125 PS8, Line 125: bloomFilterSizeLimits_.max_ = tQueryOptions.getRuntime_filter_max_size(); : bloomFilterSizeLimits_.max_ = Math.max(bloomFilterSizeLimits_.max_, : MIN_BLOOM_FILTER_SIZE); : bloomFilterSizeLimits_.max_ = Math.max(bloomFilterSizeLimits_.max_, : BackendConfig.INSTANCE.getMinBufferSize()); : bloomFilterSizeLimits_.max_ = Math.min( : BitUtil.roundUpToPowerOf2(bloomFilterSizeLimits_.max_), MAX_BLOOM_FILTER_SIZE); You may want to see if it makes sense to add this logic to FilterSizeLimits class. Something like setMax(), setMin(), setDefault() functions. http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@482 PS8, Line 482: sizeLimits 'filterSizeLimits' http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@494 PS8, Line 494: filterSizeBytes_ = Math.min(filterSizeBytes_, filterSizeLimits.max_); There is always the case that the size needed to achieve the desired fp rate is higher than the max limit size. In that case we should add a log message. -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 02 Feb 2018 18:12:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/8/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@95 PS8, Line 95: private static final long MIN_BLOOM_FILTER_SIZE = 4 * 1024; : private static final long MAX_BLOOM_FILTER_SIZE = 512 * 1024 * 1024; the query option restrictions already take care of this limit, by making sure that query options RUNTIME_FILTER_{MIN,MAX}_SIZE are in this range, Which would make it impossible to write a test case that exercises those hard limits here. Also, this makes the bounds applied below redundant. Should I still keep these in the constructor as a safeguard? or should i just put a precondition.check() instead? -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 01 Feb 2018 21:35:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#8). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 902 insertions(+), 549 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/8 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#7). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 880 insertions(+), 548 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/7 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/8971/6/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/8971/6/common/thrift/PlanNodes.thrift@150 PS6, Line 150: Should be non-zero for bloom filters > So, for other than bloom filters is it not set or is it set with a zero val it is set to zero for non-bloom filters. added clarification to comment http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@72 PS6, Line 72: > oops, will remove this extra space in next patch Done http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@75 PS6, Line 75: table > "associated table columns"? Done http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@75 PS6, Line 75: are bound by : * {MIN,MAX}_BLOOM_FILTER_SIZE, query options and the max buffer size that can be : * allocated by the bufferpool > Maybe say that they are computed based on ndvs, query options and max buffe Done http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@77 PS6, Line 77: In the absence NDV estimates, the default size : * specified by the query options is used. removing this part as the default size is also bound by the min/max size in the query option. Explaining it completely to this detail seems superfluous here. http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@177 PS6, Line 177: based on ndvEstimate_ and bounded by the max : // and min runtime filter size specified in the query options. Should be non-zero for : // bloom filters. > remove, you already have the description in the class comment above. Done http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@608 PS6, Line 608: filter.boundFilterSize(defaultFilterSize, maxFilterSize, minFilterSize); > I'd prefer if you pass the default, max and min sizes to the RuntimeFilter. Done http://gerrit.cloudera.org:8080/#/c/8971/6/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test File testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test: http://gerrit.cloudera.org:8080/#/c/8971/6/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test@59 PS6, Line 59: 174.39 > Hm, why did these change? these sizes seem to change across test builds too, maybe due to changes in data loading. i looked at the logs of some core test runs on jenkins and they seemed to be different. However the test validation logic is designed to ignore this part for the profile, which i think was done intentionally for this specific reason. -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 31 Jan 2018 01:54:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 6: (6 comments) Quick question about testing. How do we ensure that the min/max bounds are respected? Do we have any tests for that? http://gerrit.cloudera.org:8080/#/c/8971/6/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/8971/6/common/thrift/PlanNodes.thrift@150 PS6, Line 150: Should be non-zero for bloom filters So, for other than bloom filters is it not set or is it set with a zero value? http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@75 PS6, Line 75: table "associated table columns"? http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@75 PS6, Line 75: are bound by : * {MIN,MAX}_BLOOM_FILTER_SIZE, query options and the max buffer size that can be : * allocated by the bufferpool Maybe say that they are computed based on ndvs, query options and max buffer size and are bound by MIN/MAX_BLOOM_FILTER_SIZE. My point is to stress that their size can never be smaller/larger than the MIN/MAX values. http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@177 PS6, Line 177: based on ndvEstimate_ and bounded by the max : // and min runtime filter size specified in the query options. Should be non-zero for : // bloom filters. remove, you already have the description in the class comment above. http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@608 PS6, Line 608: filter.boundFilterSize(defaultFilterSize, maxFilterSize, minFilterSize); I'd prefer if you pass the default, max and min sizes to the RuntimeFilter.create and then to the constructor. Since these three are always used together you may create a tiny class to hold them and pass around. http://gerrit.cloudera.org:8080/#/c/8971/6/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test File testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test: http://gerrit.cloudera.org:8080/#/c/8971/6/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test@59 PS6, Line 59: 174.39 Hm, why did these change? -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 30 Jan 2018 19:02:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/6/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@72 PS6, Line 72: oops, will remove this extra space in next patch -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 29 Jan 2018 23:06:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/PlanNodes.thrift@151 PS5, Line 151: filter_byte_size > "filter_size_bytes" Done http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/Planner.thrift File common/thrift/Planner.thrift: http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/Planner.thrift@76 PS5, Line 76: runtime_filters_mem_requirement > I would rename to "runtime_filters_mem_requirement_bytes" to be consistent Done, renamed to "runtime_filters_reservation_bytes" http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@120 PS5, Line 120: runtimeFiltersReservation_ > nit: I would rename it to runtimeFilterMemReservation_. Also, you may want how about "runtimeFiltersReservationBytes_", to make it consistent with its corresponding thrift field, which i just renamed to "runtime_filters_reservation_bytes" to be consistent with "min_reservation_bytes" http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@234 PS5, Line 234: new HashSet(); > nit: we typically use guava's static methods, e.g. = Sets.newHashSet(); Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@432 PS5, Line 432: runtime-filters > maybe "runtime-filters-memory" Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@71 PS5, Line 71: * original predicates. > I think this is a good place to also comment on the resources used by runti Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@89 PS5, Line 89: MIN_BLOOM_FILTER_SIZE > I don't want to be pedantic but maybe use "RUNTIME" instead of "BLOOM". Sam These size restrictions are only for bloom filters, since min-max filters are already of a fixed size. Would you recommend I rename other variables like maxFilterSize, minFilterSize and defaultFilterSize to maxBloomFilterSize,minBloomFilterSize and defaultBLoomFilterSize respectively, just to be explicit. http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@91 PS5, Line 91: // Map of base table tuple ids to a list of runtime filters that > nit: be a good citizen, leave an empty line :) Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@106 PS5, Line 106: // Precomputed default BloomFilter size, rounded up to a power of two. > nit: "in bytes" Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@116 PS5, Line 116: BitUtil.roundUpToPowerOf2(Math.min(maxFilterSize, : MAX_BLOOM_FILTER_SIZE)); > Although they are equivalent, I think it's a bit more explicit if you say: Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@145 PS5, Line 145: BLOOM_FILTER_BUCKET_WORDS > Here it is ok to keep "BLOOM" Yes, this applies only to bloom filters http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@466 PS5, Line 466: the > remove Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@480 PS5, Line 480: ones > "values" Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@482 PS5, Line 482: public > Do we need these two functions to be public? Done http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@600 PS5, Line 600: boundFilterSize > Why calculate* and bound* happen in two different places? I had three options here: 1. do both in the RuntimeFilter constructor. => this would mean adding these three params to the constructor which already has an inflated list of params being passed in. 2. do both here => that would mean having a representation of an uninitialized bloom filter size like -1 after construction, then doing all the calculation here.
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#6). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 880 insertions(+), 543 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/6 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/PlanNodes.thrift@151 PS5, Line 151: filter_byte_size "filter_size_bytes" http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/Planner.thrift File common/thrift/Planner.thrift: http://gerrit.cloudera.org:8080/#/c/8971/5/common/thrift/Planner.thrift@76 PS5, Line 76: runtime_filters_mem_requirement I would rename to "runtime_filters_mem_requirement_bytes" to be consistent with the naming of other related fields. http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@120 PS5, Line 120: runtimeFiltersReservation_ nit: I would rename it to runtimeFilterMemReservation_. Also, you may want to mention here that this is set in computeResourceProfile() http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@234 PS5, Line 234: new HashSet(); nit: we typically use guava's static methods, e.g. = Sets.newHashSet(); http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@432 PS5, Line 432: runtime-filters maybe "runtime-filters-memory" http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@71 PS5, Line 71: * original predicates. I think this is a good place to also comment on the resources used by runtime filters (how big they are, do we cap their size, etc). http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@89 PS5, Line 89: MIN_BLOOM_FILTER_SIZE I don't want to be pedantic but maybe use "RUNTIME" instead of "BLOOM". Same for comments and stuff unless it is really important to mention that we use bloom filters. http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@91 PS5, Line 91: // Map of base table tuple ids to a list of runtime filters that nit: be a good citizen, leave an empty line :) http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@106 PS5, Line 106: // Precomputed default BloomFilter size, rounded up to a power of two. nit: "in bytes" http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@116 PS5, Line 116: BitUtil.roundUpToPowerOf2(Math.min(maxFilterSize, : MAX_BLOOM_FILTER_SIZE)); Although they are equivalent, I think it's a bit more explicit if you say: maxFilterSize = Math.min(BitUtil.roundUpToPowerOf2(maxFilterSize), MAX_BLOOM_FILTER_SIZE); Now, it's explicit that the filter will not be larger than MAX_BLOOM... http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@145 PS5, Line 145: BLOOM_FILTER_BUCKET_WORDS Here it is ok to keep "BLOOM" http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@466 PS5, Line 466: the remove http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@480 PS5, Line 480: ones "values" http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@482 PS5, Line 482: public Do we need these two functions to be public? http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@600 PS5, Line 600: boundFilterSize Why calculate* and bound* happen in two different places? http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: http://gerrit.cloudera.org:8080/#/c/8971/5/fe/src/main/java/org/apache/impala/service/BackendConfig.java@75 PS5, Line 75: public double getMaxFilterErrorRate() { : return backendCfg_.max_filter_error_rate; : } : : public long getMinBufferSize() { : return backendCfg_.min_buffer_size; : } single lines -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe,
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Dimitris Tsirogiannis, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#5). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 38 files changed, 876 insertions(+), 543 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/5 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 5 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/8971/3/testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test File testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test: PS3: > I did another path. The changes to this file look like they're only to size makes sense, I will revert changes to this and parquet-filtering.test that only have these unrelated size changes. -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 18 Jan 2018 21:38:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 ) Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/8971/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8971/3//COMMIT_MSG@12 PS3, Line 12: > Should we be adding any context related to the coordinator problem here? That's a fair point. I think the new JIRAs and links should make it clearer, so we can leave this. http://gerrit.cloudera.org:8080/#/c/8971/3/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/8971/3/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@263 PS3, Line 263: .sum(planTreeProfile.duringOpenProfile.max(fInstancePostOpenProfile)); > I think this would achieve the same. Just takes the max of duringOpenProfil Hm, oh right, the max() is nested inside the sum() instead of after it. http://gerrit.cloudera.org:8080/#/c/8971/3/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test File testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test: http://gerrit.cloudera.org:8080/#/c/8971/3/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test@59 PS3, Line 59:partitions=24/24 files=24 size=174.39KB > these sizes seem to change across test runs, maybe due to changes in data l I missed that this file had some expected changes to memory. http://gerrit.cloudera.org:8080/#/c/8971/3/testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test File testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test: PS3: > Changes to this file (and some of the later ones) look spurious too. I did another path. The changes to this file look like they're only to size, which is ignored by the test validation. I think we should revert changes to this file, because it's unrelated. -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 18 Jan 2018 21:22:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8971 to look at the new patch set (#4). Change subject: IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool .. IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool This patch adds changes to the planner to account for memory used by bloom filters at the fragment instance level. Also adds changes to allocate memory for those bloom filters from the buffer pool. Testing: - Modified Planner Tests and end to end tests to account for memory reservation for the runtime filters. - Modified backend tests and benchmarks to use the bufferpool for bloom filter allocation. - Add an end to end test. - Ran rest of the core tests. Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f --- M be/src/benchmarks/bloom-filter-benchmark.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/fe-support.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/util/backend-gflag-util.cc M be/src/util/bloom-filter-test.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/BackendGflags.thrift M common/thrift/ImpalaInternalService.thrift M common/thrift/PlanNodes.thrift M common/thrift/Planner.thrift M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters.test M testdata/workloads/functional-query/queries/QueryTest/bloom_filters_wait.test M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test M testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test M testdata/workloads/functional-query/queries/QueryTest/spilling.test 40 files changed, 883 insertions(+), 550 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8971/4 -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet VigGerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Tim Armstrong