impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
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:


found with help of clang-tidy
File be/src/benchmarks/

PS7, Line 330: int argc, char **argv
File be/src/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?
File be/src/util/bit-packing.inline.h:

PS7, Line 45: make_
make_ can be omitted.

PS7, Line 90: onstants

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
To unsubscribe, visit

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

View raw message