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 <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 31 Jan 2018 01:54:00 +0000 Gerrit-HasComments: Yes
