Dan Burkert 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. It probably doesn't make much sense without the context of the rest of the test cases. I've used [----) for ranges, so since equalities are just a single value I thought the best way to represent that was with a pipe. The point of these little diagrams was to convince myself that I had covered all of the possible interactions between the predicate types, but it's really only useful for the Range/Range and Range/Equality interactions. 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. Yes, it should be a CHECK (I've updated it). Postgres does allow IS NOT NULL/IS NULL predicates on non-nullable columns, but we should deal with that during scanner creation by just ignoring the predicates. 2. Yes, it's already the case. The selection vector by default is set to yes for all rows, and predicates only flip it from yes to no. 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" I called it IsNotNull because the SQL form of the predicate is literally 'IS NOT NULL'. None of the other predicates use the special "IS _" form in SQL. I also think it will be better for when we eventually want to add an IsNull predicate - just having it be Null could be quite confusing. 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 describin It's on line 275. -- 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: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
