Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13703 )
Change subject: KUDU-2483 (part 1/3) Add bloom filter predicate for Java API ...................................................................... Patch Set 1: (8 comments) Thanks for the contribution! I did a partial first pass review. http://gerrit.cloudera.org:8080/#/c/13703/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13703/1//COMMIT_MSG@7 PS1, Line 7: KUDU-2483 (part 1/3) Add bloom filter predicate for Java API You can remove the "part" piece, that could change and we will close the jira when all the work is done. http://gerrit.cloudera.org:8080/#/c/13703/1//COMMIT_MSG@13 PS1, Line 13: and fixed some bugs in bloom filter of Java. Are there unit tests for BloomFilter? Could you add some? http://gerrit.cloudera.org:8080/#/c/13703/1//COMMIT_MSG@15 PS1, Line 15: other types than nit: all types other than http://gerrit.cloudera.org:8080/#/c/13703/1//COMMIT_MSG@16 PS1, Line 16: the DECIMAL type. I will support it in later patches. What is the challenge with supporting DECIMAL? http://gerrit.cloudera.org:8080/#/c/13703/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java: http://gerrit.cloudera.org:8080/#/c/13703/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java@512 PS1, Line 512: public static KuduPredicate newComparisonPredicate(ColumnSchema column, Can you add BloomFilter support here? http://gerrit.cloudera.org:8080/#/c/13703/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java@552 PS1, Line 552: boolean isNotNone = false; Does it make sense to move this isNotNone logic into the BloomFilter implementation? Perhaps an "isNone()" or "isEmpty()" method for the BloomFilter? http://gerrit.cloudera.org:8080/#/c/13703/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java@933 PS1, Line 933: boolean bloomFiltersContains(byte[] value) { Should this be `bloomFiltersMayContain` to indicate that it's not guaranteed to contain if true? The logic here also requires all bloom filters to possibly contain the value. Javadoc describing the intent would probably be good. http://gerrit.cloudera.org:8080/#/c/13703/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java File java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java: http://gerrit.cloudera.org:8080/#/c/13703/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@65 PS1, Line 65: private final byte[] bitSet; I am curious why you changed to a byte array? Should this be called `bytes` now? `bitSet seems wrong. -- To view, visit http://gerrit.cloudera.org:8080/13703 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If96ec164e86091f8fb3b7f92b3623ca3e728edfd Gerrit-Change-Number: 13703 Gerrit-PatchSet: 1 Gerrit-Owner: Yao Xu <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 21 Jun 2019 23:56:21 +0000 Gerrit-HasComments: Yes
