Adar Dembo has posted comments on this change. Change subject: [java-client] IN-list predicate ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/4530/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: Line 91: /** IN-list values. */ May want to indicate that the outer array (the actual "list") is sorted. PS1, Line 330: /** : * Creates a new builder for an IN list predicate. : * : * Only values of the correct type for the column may be added to the builder. : * : * @param column the column schema : * @return the IN list builder : */ : public static InListBuilder newInListBuilder(ColumnSchema column) { : return new InListBuilder(column); : } I don't really see the advantage of this indirection vs. just making the InListBuilder constructor public. Line 363: public KuduPredicate(ColumnSchema column, byte[][] inListValues) { Why is this public? Shouldn't clients go through the builder? PS1, Line 580: case IS_NOT_NULL: { : return newIsNotNullPredicate(column); : } Whoops. What was the effect of missing this? PS1, Line 596: /** : * Compares two bounds based on the type of this predicate's column. : * @param a the first serialized value : * @param b the second serialized value : * @return the comparison of the serialized values based on the column type : */ Update the param list. Line 825: * Builder for an IN-list predicate. I don't really like this abstraction, for a couple reasons: 1. Given the various addValue() variants, it suggests that it's possible to add values of different types in the same predicate. 2. Once you strip that away, all that's left is the ability to add a list of values, and there's no need for a builder to get that. What about a generic KuduPredicate factory method that takes a List<T> of values? That way the language guarantees that all of the values are of the same type, modulo funky casting. I expect that it'll be hard to implement the switch statement in addValue() this way, but maybe there's a clever approach to do it. Or maybe you force callers to provide the data type as an additional argument. http://gerrit.cloudera.org:8080/#/c/4530/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java: PS1, Line 448: case IS_NOT_NULL: break; And here too; what was the effect of missing this? -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
