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 Tue, 11 Oct 2016 21:28:49 GMT
Jim Apple has posted comments on this change.

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


Patch Set 8:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/4494/8//COMMIT_MSG
Commit Message:

PS8, Line 17: 64
32, now?


PS8, Line 18: ors at 64-bit boundaries
What does it mean to have an or at a k-bit boundary?


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

Line 273: #include "common/names.h"
This isn't needed if using namespace std;, yes?


http://gerrit.cloudera.org:8080/#/c/4494/8/be/src/benchmarks/bswap-benchmark.cc
File be/src/benchmarks/bswap-benchmark.cc:

Line 123
Thanks for fixing this. When this is committed, can you file a bug against me to fix the other
posix_memalign locations?


http://gerrit.cloudera.org:8080/#/c/4494/8/be/src/testutil/mem-util.h
File be/src/testutil/mem-util.h:

PS8, Line 31: posix_memalign
#include stdlib.h. I'd say cstdlib, but posix_memalign doesn't seem to be part of the C standard,
so I don't know if it is technically in that header.


Line 45:  private:
DISALLOW_COPY_AND_ASSIGN


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

PS7, Line 42: values' va
> In practice it should be 64-bit aligned with TCMalloc beyond a certain allo
I think it would help the reader here to spell out that misaligned is with respect to 64.


PS7, Line 132: 
> Other ones should work but I don't think improve coverage - can't see a sce
Still, if we're only allowing those two you could switch it to a bool 'misaligned'


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

Line 39: void UnpackSubset(const uint32_t* in, const uint8_t* packed, int num_in_values,
I think it would aid clarity to add a short description of what the meanings are of 'in' and
'packed'


PS8, Line 45: uint32_t
const uint32_t * in


PS8, Line 72: INFO
Was this just for debugging, or did you mean to leave it in?


PS8, Line 84: 8
Why 8 and not 4?

Can you add a more exact ASSERT here about num_to_unpack and it's relationship with writer.bytes_written()?


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 69:   // First unpack as many full batches as possible.
> Yep, that was a bug.
Should there be an additional test that would demonstrate that bug?


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

PS8, Line 64: //LOG(INFO) << "bit_width " << BIT_WIDTH << "\n"
            :   //  << "in_bytes " << in_bytes << " num_values " <<
num_values << " max_input_values " << max_input_values << "\n"
            :    // << "values_to_read " << values_to_read << " batches_to_read
" << batches_to_read << "\n"
            :    // << "remainder_values " << remainder_values;
remove


-- 
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: 8
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