Henry Robinson has posted comments on this change. Change subject: IMPALA-3480: Add query options for min/max filter sizes ......................................................................
Patch Set 2: (8 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 I don't think we should assume that the options always come in through query option parsing; that seems like tightly coupling the way the query options are set to this class. 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 Done 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 messag Done Line 348: "$0 is not a valid Bloom filter size. Valid sizes are in [$1, $2].", value, > long line Done - in a previous review we decided not to pretty-print the values so that it was very clear to users what the bounds were. Unless you feel strongly, I think the clarity outweighs the loss in presentation. Line 352: query_options->__set_runtime_filter_min_size(size); > you should also check that min <= max Can't do that here, because we don't know what order the options will be parsed in (imagine the default max is X, then we set min and max > X at once - could get into a situation where that valid set-up is rejected). I added a check to RuntimeFilterBank. 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 The problem is that leads to pain adding a new query option (you have to hunt for a free index). Since all the options are prefixed with runtime_filter_* they're easy to search for. 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 See comment elsewhere. 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 It doesn't. I was going to file a ramp-up JIRA, but it was very easy to fix. -- 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: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
