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 <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: Sat, 03 Feb 2018 00:18:46 +0000
Gerrit-HasComments: Yes

Reply via email to