[Impala-ASF-CR] IMPALA-5519: Allocate fragment's runtime filter memory from Buffer pool

2018-02-13 Thread Impala Public Jenkins (Code Review)
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 Vig 
Tested-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

2018-02-13 Thread Impala Public Jenkins (Code Review)
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 Vig 
Gerrit-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

2018-02-12 Thread Bikramjeet Vig (Code Review)
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 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

2018-02-12 Thread Impala Public Jenkins (Code Review)
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 Vig 
Gerrit-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

2018-02-07 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-07 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-07 Thread Dimitris Tsirogiannis (Code Review)
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 Vig 
Gerrit-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

2018-02-07 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-07 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-07 Thread Dimitris Tsirogiannis (Code Review)
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 Vig 
Gerrit-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

2018-02-06 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-02 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-02 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-02 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-02 Thread Dimitris Tsirogiannis (Code Review)
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 Vig 
Gerrit-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

2018-02-01 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-02-01 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-01-30 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-01-30 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-01-30 Thread Dimitris Tsirogiannis (Code Review)
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 Vig 
Gerrit-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

2018-01-29 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-01-29 Thread Bikramjeet Vig (Code Review)
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

2018-01-29 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-01-25 Thread Dimitris Tsirogiannis (Code Review)
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

2018-01-18 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-01-18 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-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

2018-01-18 Thread Tim Armstrong (Code Review)
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 Vig 
Gerrit-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

2018-01-17 Thread Bikramjeet Vig (Code Review)
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 Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong