Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16377 )
Change subject: IMPALA-10110: allow setting bloom filter fpp ...................................................................... Patch Set 2: (4 comments) Thanks for the comments, I think I addressed all of it. http://gerrit.cloudera.org:8080/#/c/16377/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/16377/2/be/src/runtime/coordinator.cc@624 PS2, Line 624: double fpp > The calculated value may be not exactly equal to user's setting? Right, I wanted this to show what the fpp estimate is given the chosen filter size. E.g. if the filter was clamped by RUNTIME_FILTER_MAX_SIZE and the FPP may be high. http://gerrit.cloudera.org:8080/#/c/16377/2/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: http://gerrit.cloudera.org:8080/#/c/16377/2/be/src/service/query-options-test.cc@387 PS2, Line 387: -1 > According to the comments in ImpalaService.thrift, if -1, falls back to max The comment was out of date. I think this behaviour makes more sense. http://gerrit.cloudera.org:8080/#/c/16377/2/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/16377/2/be/src/service/query-options.cc@949 PS2, Line 949: val < 0 || val > 1 > Here we allow fpp equal 0 or 1. But the DCHECK for fpp in BlockBloomFilter: Good point, it's better to avoid the edge cases. http://gerrit.cloudera.org:8080/#/c/16377/2/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/16377/2/common/thrift/ImpalaService.thrift@572 PS2, Line 572: If -1, falls back to max_filter_error_rate > In impala::SetQueryOption(), it return error for any value less than 0. Ple Ah yeah, missed updating the comment. Fixed that. -- To view, visit http://gerrit.cloudera.org:8080/16377 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb123a0ea1e0e95d95df9837c1f0222fd60361f3 Gerrit-Change-Number: 16377 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 27 Aug 2020 17:41:00 +0000 Gerrit-HasComments: Yes
