impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Date Sun, 09 Oct 2016 23:17:17 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-4123: Fast bit unpacking
......................................................................


Patch Set 7:

(10 comments)

found with help of clang-tidy

http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/benchmarks/bit-packing-benchmark.cc
File be/src/benchmarks/bit-packing-benchmark.cc:

PS7, Line 330: int argc, char **argv
unused


http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/util/bit-packing.h
File be/src/util/bit-packing.h:

Line 22: #ifndef IMPALA_UTIL_BIT_PACKING_H
These usually go above the #includes


PS7, Line 48: BitPacking
Why not put this into use in this patch? In other words, why only the microbenchmark?


http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/util/bit-packing.inline.h
File be/src/util/bit-packing.inline.h:

PS7, Line 45: make_
make_ can be omitted.


PS7, Line 90: onstants
constants


PS7, Line 115: else
if branch returns, so you can drop the "else" here.


PS7, Line 118: else
else if branch returns, so you can drop the else here, if you want to. I'm ambivalent.


Line 120:     DCHECK_LT(VALUE_IDX, 31) << "Trailing bits after last word.";
Can you explain more why VALUE_IDX < 31? Can you add a static_assert at the top with the
maximum value it could possibly be?


PS7, Line 126: warning
What template values cause that warning to trigger?


PS7, Line 184: BIT_WIDTH
if BIT_WIDTH is 0, I don't think this is technically allowed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message