Dan Burkert 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/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 In

Line 363:   public KuduPredicate(ColumnSchema column, byte[][] inListValues) {
> Why is this public? Shouldn't clients go through the builder?

PS1, Line 490: .
> nit, remove the trailing period

PS1, Line 504: .
> same

PS1, Line 580:       case IS_NOT_NULL: {
             :         return newIsNotNullPredicate(column);
             :       }
> Whoops. What was the effect of missing this?
Exception when deserializing scan tokens containing an IS NOT NULL predicate.  
I've moved this fix to a bug fix patch that should be included in 1.0.1.

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:

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?
KUDU-1652.  Fix moved to separate patch.

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: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to