Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8170 )
Change subject: IMPALA-5789: Add always_false flag in bloom filter ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator-filter-state.h File be/src/runtime/coordinator-filter-state.h: http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator-filter-state.h@57 PS2, Line 57: class Coordinator::FilterState { > Could you add a comment up here explaining the different meanings of 'alway These implications does not apply to other parts of the code. We are essentially doing an OR(a list of bloom_filters) here so OR(empty list) should be false. And always_true short-circuits the evaluation since future filters can no longer make any difference. In comparison, in scanner we need to apply multiple bloom_filters to a row, which is an AND, so any non-yet-received filter is treated as true. And if one filter returns false we terminate the evaluation. If we use disabled_ and is_inited_ we need additional synchronization between these two set of flags, and the two sets of flags have no difference. And I think using always_false and always_true here is algebraically more natural than is_inited_ and disabled_. http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc@1121 PS2, Line 1121: swap(rpc_params.bloom_filter, aggregated_filter); > What are the possible states after this swap() ? Since we're not explicitly A check is added. (It could be always_false as well) http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc@1123 PS2, Line 1123: size() > Since you changed the mem tracking from size() to capacity() in L1120 and L You are right. Done. -- To view, visit http://gerrit.cloudera.org:8080/8170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d Gerrit-Change-Number: 8170 Gerrit-PatchSet: 2 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 09 Oct 2017 21:07:01 +0000 Gerrit-HasComments: Yes
