Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710092107.v99L71Rw024119@ip-10-146-233-104.ec2.internal>
Date: Mon, 9 Oct 2017 21:07:01 +0000
From: "Tianyi Wang (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>,
        Sailesh Mukil <sailesh@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5789=3A_Add_always_false_flag_in_bloom_filter=0A?=
In-Reply-To: <gerrit.1506648261000.If680240a3cd4583fc97c3192177d86d9567c4f8d@gerrit.cloudera.org>
References: <gerrit.1506648261000.If680240a3cd4583fc97c3192177d86d9567c4f8d@gerrit.cloudera.org>
Reply-To: twang@cloudera.com, impala-cr@cloudera.com, sailesh@cloudera.com,
        marcelk@gmail.com, tarmstrong@cloudera.com,
        reviews@impala.incubator.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="gflP15Fy7P8="; charset=UTF-8
archived-at: Mon, 09 Oct 2017 21:07:07 -0000

--gflP15Fy7P8=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Tianyi Wang has posted comments on this change=2E ( http://gerrit=2Eclouder=
a=2Eorg:8080/8170 )

Change subject: IMPALA-5789: Add always_false flag in =
bloom filter
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=


Patch Set 2:

(3 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170=
/2/be/src/runtime/coordinator-filter-state=2Eh
File be/src/runtime/coordina=
tor-filter-state=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/2/be/sr=
c/runtime/coordinator-filter-state=2Eh@57
PS2, Line 57: class Coordinator::=
FilterState {
> Could you add a comment up here explaining the different me=
anings of 'alway
These implications does not apply to other parts of the co=
de=2E We are essentially doing an OR(a list of bloom_filters) here so OR(em=
pty list) should be false=2E And always_true short-circuits the evaluation =
since future filters can no longer make any difference=2E  In comparison, i=
n 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=2E And if one filter re=
turns false we terminate the evaluation=2E

If we use disabled_ and is_init=
ed_ we need additional synchronization between these two set of flags, and =
the two sets of flags have no difference=2E And I think using always_false =
and always_true here is algebraically more natural than is_inited_ and disa=
bled_=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/2/be/src/runtime/co=
ordinator=2Ecc
File be/src/runtime/coordinator=2Ecc:

http://gerrit=2Ecloud=
era=2Eorg:8080/#/c/8170/2/be/src/runtime/coordinator=2Ecc@1121
PS2, Line 11=
21:     swap(rpc_params=2Ebloom_filter, aggregated_filter);
> What are the =
possible states after this swap() ? Since we're not explicitly
A check is a=
dded=2E (It could be always_false as well)


http://gerrit=2Ecloudera=2Eorg=
:8080/#/c/8170/2/be/src/runtime/coordinator=2Ecc@1123
PS2, Line 1123: size(=
)
> Since you changed the mem tracking from size() to capacity() in L1120 a=
nd L
You are right=2E Done=2E


-- 
To view, visit http://gerrit=2Eclouder=
a=2Eorg:8080/8170
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080=
/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageT=
ype: comment
Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d
Ge=
rrit-Change-Number: 8170
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <twan=
g@cloudera=2Ecom>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera=2Ecom>
G=
errit-Reviewer: Tianyi Wang <twang@cloudera=2Ecom>
Gerrit-Reviewer: Tim Arm=
strong <tarmstrong@cloudera=2Ecom>
Gerrit-Comment-Date: Mon, 09 Oct 2017 21=
:07:01 +0000
Gerrit-HasComments: Yes

--gflP15Fy7P8=--