Dan Burkert has posted comments on this change. Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates ......................................................................
Patch Set 2: (17 comments) http://gerrit.cloudera.org:8080/#/c/5439/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 487: return new KuduPredicate(PredicateType.IS_NULL, column, null, null); Is it correct to check here if the column is non-nullable, and if so return a NONE predicate? http://gerrit.cloudera.org:8080/#/c/5439/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java: Line 518: // IS NOT NULL > extra line? or if you meant this to be a section header, separate and add a Yah I didn't do a great job of making the section headers stand out when I wrote this, adding some dashes to all of them would help. Line 552: // [-------> Please add the other two range types (upper bounded range, upper + lower bounded range) to IS NOT NULL and IS NULL. Line 659: // [--) Do you mind fixing this ascii while you are making changes here? Line 667: // [--) same here Line 937: public void testToString() { add IS NULL test here http://gerrit.cloudera.org:8080/#/c/5439/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java: Line 630: // timestamp IS NOT NULL Add an IS NULL case. http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 764: KuduPredicate* KuduTable::NewIsNullPredicate(const Slice& col_name) { Same comment here about IS NULL on non-nullable columns. http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: PS2, Line 805: CountRows(table, {}) - 1 I think this can be simplified to values.size() - 1 here and elsewhere. http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: Line 214: //ASSERT_GE(3, tokens.size()); > the above seems to indicate that pruning is working _better_ than we expect Perhaps this confusion is because https://github.com/apache/kudu/commit/ab76440e9c2447ba1c572b957a958e90e9e9b87b landed only recently? Line 219: { // IS NOT NULL predicate Could also use an IS NULL case here and below. http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: Line 681: // [-------) Add [---> and <-----) http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.cc File src/kudu/common/column_predicate.cc: Line 149: None(move(column)); OK cool - looks like you are doing this simplification. Could you add the equivalent on the Java side and add specific tests where appropriate? PS2, Line 669: // Fallthrough intended. either add this to the None case or just remove it. Not sure what our style guide says, but I think it's pretty clear when there isn't a body. http://gerrit.cloudera.org:8080/#/c/5439/2/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: Line 184: return false; This is a little suspicious. I'm having a hard time following when CopyAndEval is and is not used in cfile_reader.cc. Does this codepath at least have good coverage? Todd, did you check this out? PS2, Line 257: attach the & to the type PS2, Line 260: e same -- To view, visit http://gerrit.cloudera.org:8080/5439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
