[kudu-CR] [java-client] IN-list predicate
Dan Burkert has submitted this change and it was merged. Change subject: [java-client] IN-list predicate .. [java-client] IN-list predicate Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Reviewed-on: http://gerrit.cloudera.org:8080/4530 Tested-by: Kudu Jenkins Reviewed-by: Jean-Daniel Cryans --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java 5 files changed, 616 insertions(+), 118 deletions(-) Approvals: Jean-Daniel Cryans: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] IN-list predicate
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 6: Code-Review+2 -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] IN-list predicate
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 4: Code-Review+2 -- 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java-client] IN-list predicate
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4530 to look at the new patch set (#4). Change subject: [java-client] IN-list predicate .. [java-client] IN-list predicate Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java 5 files changed, 616 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/4530/4 -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] IN-list predicate
Dan Burkert has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4530/3/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 393: for (T value : values) { > Agreed with JD; I'd like to see test coverage for this case. Done -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
Adar Dembo has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4530/3/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 393: for (T value : values) { > So are we missing a test here? Agreed with JD; I'd like to see test coverage for this case. -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4530/3/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 393: for (T value : values) { So are we missing a test here? -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
Dan Burkert has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4530/2/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 392: checkColumn(column, Type.BINARY); > Don't want to add the bytes? Or maybe this is http://www.snopes.com/music/a Never attribute to malice that which can be adequately explained by stupidity. -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4530 to look at the new patch set (#3). Change subject: [java-client] IN-list predicate .. [java-client] IN-list predicate Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java 5 files changed, 566 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/4530/3 -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] IN-list predicate
Adar Dembo has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/4530/2/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: PS2, Line 351: if (t instanceof Boolean) { Yess (AKA "thanks, type erasure") Line 392: checkColumn(column, Type.BINARY); Don't want to add the bytes? Or maybe this is http://www.snopes.com/music/artists/vanhalen.asp? -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4530 to look at the new patch set (#2). Change subject: [java-client] IN-list predicate .. [java-client] IN-list predicate Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java 5 files changed, 563 insertions(+), 118 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/4530/2 -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java-client] IN-list predicate
Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] IN-list predicate .. Patch Set 1: (2 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: PS1, Line 490: . nit, remove the trailing period PS1, Line 504: . same -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
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 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java-client] IN-list predicate
Dan Burkert has uploaded a new change for review. http://gerrit.cloudera.org:8080/4530 Change subject: [java-client] IN-list predicate .. [java-client] IN-list predicate Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java 5 files changed, 672 insertions(+), 144 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/4530/1 -- To view, visit http://gerrit.cloudera.org:8080/4530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia556196963153f9db64d67bc699f96cb920ecac6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert