Adar Dembo has posted comments on this change. Change subject: Add internal IS NOT NULL predicate type ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/2671/2/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: Line 213: // | Huh, I didn't know pipe is the symbol for equality. http://gerrit.cloudera.org:8080/#/c/2671/2/src/kudu/common/column_predicate.cc File src/kudu/common/column_predicate.cc: Line 291: if (block.is_nullable()) { This is all new code to me so take my questions with a grain of salt: 1. Shouldn't this be asserted forcefully? How might we end up evaluating IS NOT NULL on a column that wasn't nullable in the first place? 2. If the column isn't nullable, shouldn't we select all of the rows in the block? Or is that already the case coming into this function? http://gerrit.cloudera.org:8080/#/c/2671/2/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: Line 45: // A predicate which evaluates to true if the value is not null. Nit: shouldn't we be referring to this as "NotNull" rather than "IsNotNull"? We aren't calling the others "IsWithinRange", "IsEqualTo", etc. Also asking because the IsNotNull() factory method below is confusing; the name suggests a comparison. http://gerrit.cloudera.org:8080/#/c/2671/2/src/kudu/common/common.proto File src/kudu/common/common.proto: Line 304: oneof predicate { Nit: what happened to field 1? Don't we typically leave a comment describing removed fields? -- To view, visit http://gerrit.cloudera.org:8080/2671 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifcf29b1f274df2ef5c5ac7a7a17cc06dfd59e191 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
