Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711040028.vA40SGJh024544@ip-10-146-233-104.ec2.internal>
Date: Sat, 4 Nov 2017 00:28:16 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Tianyi Wang <twang@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-2281=3A_Replace_FNV_with_FastHash_in_exchange_nodes=0A?=
In-Reply-To: <gerrit.1509393365000.I778317d982dcdb94173a369a65b39f32b4f7ded2@gerrit.cloudera.org>
References: <gerrit.1509393365000.I778317d982dcdb94173a369a65b39f32b4f7ded2@gerrit.cloudera.org>
Reply-To: tarmstrong@cloudera.com, impala-cr@cloudera.com, twang@cloudera.com,
        marcelk@gmail.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="VF+t9B+rIzI="; charset=UTF-8
archived-at: Sat, 04 Nov 2017 00:28:22 -0000

--VF+t9B+rIzI=
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/8417 )

Change subject: IMPALA-2281: Replace FNV with FastHa=
sh in exchange nodes
=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:

(12 comments)

Did a first pass over it=2E

http://g=
errit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/codegen/gen_ir_descriptions=
=2Epy
File be/src/codegen/gen_ir_descriptions=2Epy:

http://gerrit=2Ecloude=
ra=2Eorg:8080/#/c/8417/2/be/src/codegen/gen_ir_descriptions=2Epy@99
PS2, Li=
ne 99:   ["HASH_FAST", "IrFastHash"],
Do we use this? I don't see any place=
s where it's used currently=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/84=
17/2/be/src/runtime/data-stream-sender=2Ecc
File be/src/runtime/data-stream=
-sender=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/runtim=
e/data-stream-sender=2Ecc@a468
PS2, Line 468: 
Lol, weird=2E


http://gerri=
t=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/runtime/data-stream-sender=2Ecc@4=
74
PS2, Line 474: partition_expr_evals_[j
Use 'eval' directly=2E


http://g=
errit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/runtime/raw-value=2Ecc
File b=
e/src/runtime/raw-value=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8417=
/2/be/src/runtime/raw-value=2Ecc@220
PS2, Line 220:       return HashUtil::=
FastHash64(v, static_cast<size_t>(type=2EGetByteSize()), seed);
I think thi=
s might be slightly slower - with the previous approach I think we get a sp=
ecialised version of the hash function for that input length for each data =
type, whereas here we have to go through another switch in GetByteSize()=2E=


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/util/hash-util=2Eh=

File be/src/util/hash-util=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8=
417/2/be/src/util/hash-util=2Eh@235
PS2, Line 235:   /* The MIT License
I t=
hink this should go at the top of the file beneath the apache license=2E Th=
en we can just say that the FastHash64 implementation came with that licens=
e=2E E=2Eg=2E

/* 
  FastHash64 implementation derived from MIT-licensed co=
de written by Zilong Tan

  The MIT License
  =2E=2E=2E
*/


http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/util/hash-util=2Eh@263
PS2, Line 2=
63: size_t
use int64_t - we generally use signed integers for lengths=2E


=
http://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/util/hash-util=2Eh@26=
6
PS2, Line 266: (const uint64_t *)
We use c-style casts - i=2Ee=2E reinter=
pret_cast<>


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/util/ha=
sh-util=2Eh@267
PS2, Line 267: pos
I believe the C++ standard doesn't allow=
 pointer arithmetic on void - we should convert to uint8_t for doing that=
=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/util/hash-util=
=2Eh@278
PS2, Line 278:  (const unsigned char*
See above comments=2E


http=
://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/be/src/util/hash-util=2Eh@282
PS=
2, Line 282: (uint64_t)
should use static_cast<uint64_t>()


http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8417/2/testdata/workloads/functional-query/queri=
es/QueryTest/nested-types-runtime=2Etest
File testdata/workloads/functional=
-query/queries/QueryTest/nested-types-runtime=2Etest:

http://gerrit=2Eclou=
dera=2Eorg:8080/#/c/8417/2/testdata/workloads/functional-query/queries/Quer=
yTest/nested-types-runtime=2Etest@145
PS2, Line 145: 8,'k1',-1,'k1',1
Why d=
oes this make a difference when we have VERIFY_IS_EQUAL_SORTED above?


htt=
p://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/testdata/workloads/tpcds/querie=
s/tpcds-q77a=2Etest
File testdata/workloads/tpcds/queries/tpcds-q77a=2Etest=
:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8417/2/testdata/workloads/tpcds/=
queries/tpcds-q77a=2Etest@125
PS2, Line 125: 'catalog channel',NULL,538912=
=2E55,2050279=2E74,-1383554=2E73
I'll file a JIRA to make this test determi=
nistically pass: IMPALA-6155=2E In the meantime, can you change this to

  =
 ---- RESULTS: VERIFY_IS_EQUAL_SORTED

That will make it ignore the order o=
f results=2E


-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8080/8417=

To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/settings

Gerrit=
-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerr=
it-Change-Id: I778317d982dcdb94173a369a65b39f32b4f7ded2
Gerrit-Change-Numbe=
r: 8417
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <twang@cloudera=2Ecom>=

Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Comment-=
Date: Sat, 04 Nov 2017 00:28:16 +0000
Gerrit-HasComments: Yes

--VF+t9B+rIzI=--