Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710132034.v9DKY6tR014621@ip-10-146-233-104.ec2.internal>
Date: Fri, 13 Oct 2017 20:34:05 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Tianyi Wang <twang@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Sailesh Mukil <sailesh@cloudera.com>,
        Thomas Tauber-Marshall <tmarshall@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: tarmstrong@cloudera.com, impala-cr@cloudera.com,
        sailesh@cloudera.com, twang@cloudera.com, marcelk@gmail.com,
        tmarshall@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="EucaTb946aw="; charset=UTF-8
archived-at: Fri, 13 Oct 2017 20:34:14 -0000

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

Tim Armstrong has posted comments on this change=2E ( http://gerrit=2Ecloud=
era=2Eorg:8080/8170 )

Change subject: IMPALA-5789: Add always_false flag i=
n 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 4:

(5 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170=
/4//COMMIT_MSG
Commit Message:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/817=
0/4//COMMIT_MSG@15
PS4, Line 15: 
Did you do any perf runs? It would be goo=
d to verify that the extra flag checking doesn't affect perf (I suspect it =
doesn't)=2E

It would also be good to confirm that testdata/workloads/targe=
ted-perf/queries/primitive_empty_build_join_1=2Etest gets faster (I think i=
t should!)=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/4/be/src/exec/=
hdfs-parquet-scanner=2Ecc
File be/src/exec/hdfs-parquet-scanner=2Ecc:

http=
://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/4/be/src/exec/hdfs-parquet-scanner=
=2Ecc@403
PS4, Line 403: std::
Shouldn't need std:: - it's imported in comm=
on/names=2Eh


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/4/be/src/exec/h=
dfs-scanner=2Ecc
File be/src/exec/hdfs-scanner=2Ecc:

http://gerrit=2Ecloud=
era=2Eorg:8080/#/c/8170/4/be/src/exec/hdfs-scanner=2Ecc@107
PS4, Line 107: =
    if (!BaseSequenceScanner::FileFormatIsSequenceBased(
Might be more read=
able to factor subexpression into variable e=2Eg=2E is_sequence_based=2E


=
http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/4/tests/custom_cluster/test_al=
ways_false_filter=2Epy
File tests/custom_cluster/test_always_false_filter=
=2Epy:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/4/tests/custom_cluster=
/test_always_false_filter=2Epy@53
PS4, Line 53:   seq_table_suffixes =3D ['=
_avro', '_rc', '_seq']
We'd normally create a test matrix based on these=2E=
 Is the idea here to avoid restarting the cluster for each file format? If =
so it would be good to leave a comment so that readers understand why=2E


=
http://gerrit=2Ecloudera=2Eorg:8080/#/c/8170/4/tests/custom_cluster/test_al=
ways_false_filter=2Epy@82
PS4, Line 82:   def test_skip_file(self, cursor):=

Does this need to be a custom cluster test? I=2Ee=2E does it need a specia=
l minicluster to execute=2E It's best to make things query tests if possibl=
e since starting a cluster is slow and the tests aren't parallelisable=2E

=


-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8080/8170
To unsubscrib=
e, visit http://gerrit=2Ecloudera=2Eorg:8080/settings

Gerrit-Project: Impa=
la-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: =
If680240a3cd4583fc97c3192177d86d9567c4f8d
Gerrit-Change-Number: 8170
Gerrit=
-PatchSet: 4
Gerrit-Owner: Tianyi Wang <twang@cloudera=2Ecom>
Gerrit-Review=
er: Sailesh Mukil <sailesh@cloudera=2Ecom>
Gerrit-Reviewer: Thomas Tauber-M=
arshall <tmarshall@cloudera=2Ecom>
Gerrit-Reviewer: Tianyi Wang <twang@clou=
dera=2Ecom>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerr=
it-Comment-Date: Fri, 13 Oct 2017 20:34:05 +0000
Gerrit-HasComments: Yes

--EucaTb946aw=--