Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9193 )
Change subject: [tests] Add more test coverage for INT128 columns ...................................................................... Patch Set 8: (2 comments) LGTM, but seems like it may be a good opportunity to fill in the 64-bit tests as well. http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/cfile-test.cc@563 PS8, Line 563: // Test for BitShuffle builder for UINT8, INT8, UINT16, INT16, UINT32, INT32, FLOAT, DOUBLE Update this comment. Also, is the omission of the 64 bit types intentional here? Seems like maybe we overlooked it at some point. http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/9193/8/src/kudu/cfile/encoding-test.cc@694 PS8, Line 694: // Test for bitshuffle block, for INT32, INT128, FLOAT, DOUBLE Same here, seems like 64bit should be added as well. -- To view, visit http://gerrit.cloudera.org:8080/9193 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295 Gerrit-Change-Number: 9193 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 05 Feb 2018 16:08:24 +0000 Gerrit-HasComments: Yes
