Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 )
Change subject: IMPALA-5519: Allocate Runtime filter memory from Buffer pool ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.h@112 PS3, Line 112: directoty_ typo: directory http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/8971/3/be/src/util/bloom-filter.cc@50 PS3, Line 50: Close(); Calling Close() is a little subtle. Maybe mention the reason for doing this so it isn't lost. 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 missed this on the first pass. I think we need to add the runtime filters reservation to both duringOpenProfile and fInstancePostOpenProfile, then take the max of those. This is because the filters may be constructed during open and live after that. 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 Changes in this file look spurious. 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. http://gerrit.cloudera.org:8080/#/c/8971/2/testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test File testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test: http://gerrit.cloudera.org:8080/#/c/8971/2/testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test@391 PS2, Line 391: # mem requirement after accounting for the memory > the old test was already flawed in that it never exercised the code path th Thanks for the detailed explanation. -- 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 <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 18 Jan 2018 00:18:28 +0000 Gerrit-HasComments: Yes
