Todd Lipcon has posted comments on this change. Change subject: KUDU-1600 (part 2): store uncompressed blocks when codec can't compress ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/5679/2/src/kudu/cfile/block_compression.cc File src/kudu/cfile/block_compression.cc: Line 137: // Check if the on-disk data size matches with the buffer > Nit: terminate with period. Done Line 144: } else { > Shouldn't we explicitly test for == 2? Otherwise if/when we add cfile v3, o I think if we were to make another change to this format, we would not bump the cfile version number, but instead we'd introduce it as an 'incompatible_feature' flag in the footer. Put another way, I dont expect us to ever have a new cfile version number. PS2, Line 149: uncompressed_size_ > Isn't this -1 at this point? woops. I wonder how this worked... perhaps due to unsigned integer nonsense. Will move it back where it belongs. Line 172: DCHECK_GE(uncompressed_size_, 0); > If you call uncompressed_size() below, you won't need this. yea, though I think calling your own public accessors is a bit ugly because it obscures what's going on (if I saw that I might think something's getting calculated, for example) http://gerrit.cloudera.org:8080/#/c/5679/2/src/kudu/cfile/block_compression.h File src/kudu/cfile/block_compression.h: Line 43: // CFile version 2 > Should include the <compressed data> bit in here too. Done Line 61: // Sets "*result" to the compressed version of the "data". > Update to data_slices. Done http://gerrit.cloudera.org:8080/#/c/5679/2/src/kudu/cfile/compression-test.cc File src/kudu/cfile/compression-test.cc: Line 77: class TestCompression : public CFileTestBase { > Curious: what prompted the changes to this test? (a) switched from uint32/GROUP_VARINT to INT32/BIT_SHUFFLE since users can no longer specify uint32 columns in the first place and GROUP_VARINT encoding is deprecated. (b) added the 'random' data generation with PLAIN encoding to ensure that some blocks we created were non-compressible and exercise the new code path. Will add a comment. -- To view, visit http://gerrit.cloudera.org:8080/5679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8306f1f2139ebf7500a83ee46b4ccd6c7b5e137f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
