Dan Burkert has posted comments on this change. Change subject: [java-client] IN-list predicate ......................................................................
Patch Set 1: (9 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. Done 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 removed. Line 363: public KuduPredicate(ColumnSchema column, byte[][] inListValues) { > Why is this public? Shouldn't clients go through the builder? Done PS1, Line 490: . > nit, remove the trailing period Done PS1, Line 504: . > same Done 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 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. Done Line 825: * Builder for an IN-list predicate. > I don't really like this abstraction, for a couple reasons: Done 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? 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
