impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Date Fri, 12 May 2017 01:32:58 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................


Patch Set 8:

(3 comments)

Thanks for taking a look!

http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:

PS8, Line 300: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(7), 8);
             :   EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(8), 8);
             :   EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(65535), 65536);
> Duplicate of L297-299. I guess the intent here was to call RoundUpToPowerOf
Original implementation required user specified bit width, so this was testing 32 and 64 bit
operators separately.  Will get rid of it.


PS8, Line 308:   EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(7), 8);
             :   EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(8), 8);
             :   EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(65535), 65536);
> same here
Done


http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util.h
File be/src/util/bit-util.h:

Line 376:     DCHECK(v > 0);
> DCHECK on L389 asserts that RoundUpToPowerOfTwo(int32_t) never overflows. D
I would argue we don't need that check for 64-bit math.


-- 
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message