Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-3038: Add multistream gzip/bzip2 test coverage ......................................................................
Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/2543/5/be/src/util/decompress-test.cc File be/src/util/decompress-test.cc: Line 32: const int UNCOMPRESSED_BUFFER_SIZE = 32 * RAW_INPUT_SIZE; Put these const ints in the test class instead of directly in the impala namespace Line 159: int64_t* bytes_decompressed) { Hm, maybe this is too fancy, but since callers don't usually use the bytes_decompressed output, you can do do this: int64_t* bytes_decompressed = NULL) { int64_t unreturned_bytes_decompressed; if (bytes_decompressed == NULL) bytes_decompressed = &unreturned_bytes_decompressed; Then only the one callsite where you actually need this value needs to explicitly declare it and pass it in. Line 180: if (stream_end) { one line Line 244: int64_t truncated_decompressed = 0; Put a comment above this line, like // Decompress the remaining Line 250: EXPECT_ERROR(StreamingDecompress(decompressor.get(), 16 * 1024 * 1024, COMPRESSED_BUFFER_SIZE Line 258: // pick random size input data(~1MB), compress it, then concatenate those small nit: Pick random-size input data (~1 MB) Line 270: raw_input[RAW_INPUT_SIZE] = 0; Move L265-L270 to the top of the function, above the comment. This code is confusing in the context of the comment because it's size is not random, as the comment states. Line 272: while (*compressed_len < (COMPRESSED_BUFFER_SIZE - RAW_INPUT_SIZE) Add a comment above this loop explaining why BUFFER_SIZE - RAW_INPUT_SIZE, something like: // Make sure we don't write past the end of the buffers and instead leave some space at the end of the buffers. -- 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: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Skye Wanderman-Milne <s...@cloudera.com> Gerrit-HasComments: Yes
