Adar Dembo has posted comments on this change.

Change subject: [java-client] IN-list predicate

Patch Set 1:

File java/kudu-client/src/main/java/org/apache/kudu/client/

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 
             :    * @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.
File java/kudu-client/src/main/java/org/apache/kudu/client/

PS1, Line 448: case IS_NOT_NULL: break;
And here too; what was the effect of missing this?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to