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 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 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"? 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 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 computed? Same for L121. 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? 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 we're already checking these bounds when we set the query options. You may remove them. Same fore L114-L115. 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 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 could rename to maxSize, minSize and defaultSize but up to you. -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 06 Feb 2018 18:08:27 +0000 Gerrit-HasComments: Yes
