Todd Lipcon has posted comments on this change. Change subject: [java-client] implement ColumnPredicate API ......................................................................
Patch Set 1: (8 comments) only got part-way through this and have to head to the airport, but worth looking at the higher-level comment about the user-facing API http://gerrit.cloudera.org:8080/#/c/2591/1//COMMIT_MSG Commit Message: Line 16: ColumnRangePredicate API. will you do that in a follow-up? people might refer to unit tests, etc, to see example usage http://gerrit.cloudera.org:8080/#/c/2591/1/java/kudu-client/src/main/java/org/kududb/client/ColumnPredicate.java File java/kudu-client/src/main/java/org/kududb/client/ColumnPredicate.java: Line 46: None, style: should be all caps (https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names) Line 56: * the createEquality value if this is an Equality predicate. "the createEquality value" seems a little nonsensical to me Line 64: * Column predicate constructor. don't think this adds much - just use the @params I think Line 96: public static ColumnPredicate equality(ColumnSchema column, boolean value) { instead of the three different sets of factory functions (lower-bound, upper-bound, and equality) how about an enum ComparisonOperator { LT, LE, EQ, GE, GT }; and a single set of functions? This mirrors the C++ API more closely, and I think is easier for users too, since it is more like the way they typically express queries in SQL another thought: should we support some type coercion here? eg allow you to use an int for a predicate on a short column, so long as the value is within-range? We do that in the C++ client and I think it is a little easier to use. Line 468: * {@code ColumnPredicate} which matches the logical intersection ({@code OR}) this should be AND, no? Line 540: // optimization or predicate merging. can you clarify this a little more? if we aren't doing predicate merging, then how come we have the whole merge() function implemented above? is there a JIRA that this should be pointing towards if it's a TODO? Line 608: private boolean isIncrement(byte[] a, byte[] b) { 'areConsecutive' is more readable (increment is a verb) -- To view, visit http://gerrit.cloudera.org:8080/2591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icdca28139a2f4f15633cfd872e372429bad831cd Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
