impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Skye Wanderman-Milne (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3038: Add multistream gzip/bzip2 test coverage
Date Mon, 21 Mar 2016 23:50:17 GMT
Skye Wanderman-Milne has posted comments on this change.

Change subject: IMPALA-3038: Add multistream gzip/bzip2 test coverage
......................................................................


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/2543/3/be/src/testutil/gtest-util.h
File be/src/testutil/gtest-util.h:

Line 26: #define EXPECT_ERROR(status, err) EXPECT_TRUE(status.code() == err);
Should this be EXPECT_EQ(status.code(), err) ?


http://gerrit.cloudera.org:8080/#/c/2543/3/be/src/util/decompress-test.cc
File be/src/util/decompress-test.cc:

Line 154:       int64_t uncompressed_len, uint8_t* uncompressed_input, bool expect_streamend)
{
expect_stream_end


Line 160:     bool stream_end = compressed_bytes_remaining == 0;
Do you need to set stream_end here? Won't it be set by ProcessBlockStreaming below?


Line 168:       if (!status.ok()) return status;
RETURN_IF_ERROR


Line 239:     EXPECT_ERROR(StreamingDecompress(decompressor.get(), compressed_len + junk_len,
Why not pass in 16 * 1024 * 1024 instead of compressed_len + junk_len?


Line 255:       uint8_t** uncompressed_data, int64_t* compressed_len, uint8_t** multistream_data)
{
initialize *uncompressed_len and *compressed_len to 0

Aslo rename multistream_data to compressed_data for consistency


Line 259:     uint8_t* uncompressed = mem_pool_.Allocate(32 * 1024 * 1024 + 1);
Why not used *multistream_data and *uncompressed_data directly?


Line 265:       *ip++ = 'a' + rand() % 26;
Since you're using rand(), I would do something like this in main():

int rand_seed = time(NULL);
LOG(INFO) << "rand_seed: " << rand_seed;
srand(rand_seed);

This way if there's a test failure, we can replay the same random sequence.


Line 274:       memcpy(&compressed_streams[*compressed_len], compressed_stream, compressed_length);
It's more idiomatic here to do compressed_streams + *compressed_len, since compressed_streams
wasn't declared as an array (here and below)


Line 279:         && *uncompressed_len < (31 * 1024 * 1024));
I think you can make this a while instead of a do while.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0e1971145dd457e71fc9c00ce7c06fff8dea88
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message