Todd Lipcon has posted comments on this change. Change subject: KUDU-734. Add test coverage for encodings and strings ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/5588/1//COMMIT_MSG Commit Message: Line 9: added a TestEncodings class to check various encodings for all column types I'm not sure this belongs in the Java client tests. We already have similar tests on the server side in C++, and since the client itself doesn't care about the column encoding, I don't think we gain much from this coverage. The JIRA mentions *key encoding* which is implemented by the KeyEncoder class and has some coverage already in TestKeyEncoding.java. The question is whether TestKeyEncoding actually covers all types. (but this encoding is separate from the columnar encodings such as bitshuffle/RLE/etc) That said, the test you have is nice in that it does cover setting/getting all of the available _types_. I just don't think it needs to cover the different columnar encodings. http://gerrit.cloudera.org:8080/#/c/5588/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestStrings.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestStrings.java: Line 51: public void testMultiStringColumnsInsert() throws Exception { I think this is already covered by TestKuduClient#testStrings() (the right-to-left order thing) Line 90: public void testUTF8StringEncoding() throws Exception { I think this is already covered by TestKuduClient#testUTF8 added in df02bb4af961817028e152ec7767423d87958ab0 It looks like I added the coverage a day or two after commenting on KUDU-734 but didn't mention it on the JIRA. sorry about that. -- To view, visit http://gerrit.cloudera.org:8080/5588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia3fbeb0f889a4c454a48d11a6e42c7da0e58a329 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
