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<RuntimeFilterId>(); 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, 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: 5 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: Thu, 25 Jan 2018 23:12:54 +0000 Gerrit-HasComments: Yes
