Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3480: Add query options for min/max filter sizes ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/2966/2/be/src/runtime/runtime-filter.cc File be/src/runtime/runtime-filter.cc: Line 45: max_filter_size_ = max<int64_t>(max_filter_size_, MIN_BLOOM_FILTER_SIZE); that seems more like a dcheck, given the checks during option parsin Line 58: default_filter_size_ = max<int64_t>(default_filter_size_, min_filter_size_); you should also check when you're parsing the options http://gerrit.cloudera.org:8080/#/c/2966/2/be/src/service/query-options.cc File be/src/service/query-options.cc: Line 329: case TImpalaQueryOptions::RUNTIME_BLOOM_FILTER_SIZE: { how about collapsing all these cases (you only need to differentiate in the __set_... call) Line 335: "$0 is not a valid Bloom filter size. Valid sizes are in [$1, $2].", value, you should reference the exact name of the query option in the error message (i know, a lot of these don't, but it's still not a good practice). Line 348: "$0 is not a valid Bloom filter size. Valid sizes are in [$1, $2].", value, long line pretty-print mem values Line 352: query_options->__set_runtime_filter_min_size(size); you should also check that min <= max http://gerrit.cloudera.org:8080/#/c/2966/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 184: // Minimum runtime filter size, in bytes group with other runtime filter options http://gerrit.cloudera.org:8080/#/c/2966/2/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: Line 214: RUNTIME_FILTER_MAX_SIZE, group with other runtime filter options http://gerrit.cloudera.org:8080/#/c/2966/2/testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test File testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test: Line 262: SET RUNTIME_FILTER_MAX_SIZE=4096; 4k should work -- To view, visit http://gerrit.cloudera.org:8080/2966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c13c200a0f1855f38a5da50ca34a737e741868b Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
