Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711091853.vA9Irepb007166@ip-10-146-233-104.ec2.internal>
Date: Thu, 9 Nov 2017 18:53:40 +0000
From: "Thomas Tauber-Marshall (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mjacobs@apache.org>, Michael Ho <kwho@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>, Lars Volker <lv@cloudera.com>,
        Tim Armstrong <tarmstrong@cloudera.com>, Todd Lipcon <todd@apache.org>,
        Alex Behm <alex.behm@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4252=3A_Min-max_runtime_filters_for_Kudu=0A?=
In-Reply-To: <gerrit.1503529605000.I02bad890f5b5f78388a3041bf38f89369b5e2f1c@gerrit.cloudera.org>
References: <gerrit.1503529605000.I02bad890f5b5f78388a3041bf38f89369b5e2f1c@gerrit.cloudera.org>
Reply-To: tmarshall@cloudera.com, impala-cr@cloudera.com, lv@cloudera.com,
        marcelk@gmail.com, kwho@cloudera.com, tarmstrong@cloudera.com,
        dhecht@cloudera.com, mmokhtar@cloudera.com, todd@apache.org,
        alex.behm@cloudera.com, reviews@impala.incubator.apache.org,
        mjacobs@apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="VyCRIkwwQB0="; charset=UTF-8
archived-at: Thu, 09 Nov 2017 18:53:48 -0000

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

Thomas Tauber-Marshall has posted comments on this change=2E ( http://gerri=
t=2Ecloudera=2Eorg:8080/7793 )

Change subject: IMPALA-4252: Min-max runtim=
e filters for Kudu
=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 14:

(7 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#=
/c/7793/13/be/src/exec/kudu-scanner=2Ecc
File be/src/exec/kudu-scanner=2Ecc=
:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/7793/13/be/src/exec/kudu-scanner=
=2Ecc@200
PS13, Line 200:             if (!filter->GetCastIntMinMax(col_typ=
e, &int_min, &int_max)) {
> Unfortunate that that every Kudu client has to =
do this=2E
This is something that's been discussed before (see KUDU-931), I=
'm not sure why they decided not to do it=2E


http://gerrit=2Ecloudera=2Eo=
rg:8080/#/c/7793/13/be/src/util/min-max-filter=2Ecc
File be/src/util/min-ma=
x-filter=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/7793/13/be/src/util=
/min-max-filter=2Ecc@139
PS13, Line 139:     case TYPE_BIGINT:
> using std:=
:numeric_limits; at the top if this file
Done


http://gerrit=2Ecloudera=2E=
org:8080/#/c/7793/13/be/src/util/min-max-filter=2Ecc@165
PS13, Line 165: 
>=
 Brief comment especially about the return value would be good=2E
Done


ht=
tp://gerrit=2Ecloudera=2Eorg:8080/#/c/7793/13/fe/src/main/java/org/apache/i=
mpala/planner/RuntimeFilterGenerator=2Ejava
File fe/src/main/java/org/apach=
e/impala/planner/RuntimeFilterGenerator=2Ejava:

http://gerrit=2Ecloudera=
=2Eorg:8080/#/c/7793/13/fe/src/main/java/org/apache/impala/planner/RuntimeF=
ilterGenerator=2Ejava@604
PS13, Line 604:         // must be a SlotRef poin=
ting to a column=2E We can allow implicit integer casts
> typo: We can allo=
w implicit integer casts
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/779=
3/13/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator=2Eja=
va@605
PS13, Line 605:         // by casting the min/max values before send=
ing them to Kudu=2E
> min/max values
Done


http://gerrit=2Ecloudera=2Eorg:=
8080/#/c/7793/13/testdata/workloads/functional-query/queries/QueryTest/min_=
max_filters=2Etest
File testdata/workloads/functional-query/queries/QueryTe=
st/min_max_filters=2Etest:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/7793/13=
/testdata/workloads/functional-query/queries/QueryTest/min_max_filters=2Ete=
st@98
PS13, Line 98: where a=2Etinyint_col =3D b=2Eint_col and b=2Eint_col =
in (0, 1)
> Let's make the min/max filter selective, e=2Eg=2E by adding whe=
re b=2Eint_col in 
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/7793/13/t=
estdata/workloads/functional-query/queries/QueryTest/min_max_filters=2Etest=
@103
PS13, Line 103: # The min/max values in the filter are both above the =
range of the target col so all rows
> Let's also add a non-selective case w=
here the min/max values fall outside t
Done


-- 
To view, visit http://ge=
rrit=2Ecloudera=2Eorg:8080/7793
To unsubscribe, visit http://gerrit=2Ecloud=
era=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
G=
errit-MessageType: comment
Gerrit-Change-Id: I02bad890f5b5f78388a3041bf38f8=
9369b5e2f1c
Gerrit-Change-Number: 7793
Gerrit-PatchSet: 14
Gerrit-Owner: Th=
omas Tauber-Marshall <tmarshall@cloudera=2Ecom>
Gerrit-Reviewer: Alex Behm =
<alex=2Ebehm@cloudera=2Ecom>
Gerrit-Reviewer: Anonymous Coward #345
Gerrit-=
Reviewer: Dan Hecht <dhecht@cloudera=2Ecom>
Gerrit-Reviewer: Lars Volker <l=
v@cloudera=2Ecom>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache=2Eorg>
Ge=
rrit-Reviewer: Michael Ho <kwho@cloudera=2Ecom>
Gerrit-Reviewer: Mostafa Mo=
khtar <mmokhtar@cloudera=2Ecom>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm=
arshall@cloudera=2Ecom>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera=
=2Ecom>
Gerrit-Reviewer: Todd Lipcon <todd@apache=2Eorg>
Gerrit-Comment-Dat=
e: Thu, 09 Nov 2017 18:53:40 +0000
Gerrit-HasComments: Yes

--VyCRIkwwQB0=--