Vuk Ercegovac has posted comments on this change. Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. ......................................................................
Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/8014/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 2933: {: RESULT = p; :} > It's a little weird that "IS NULL" is handled this way and "IS TRUE" is han they're separate predicates in the standard (is null vs. is boolean)so I've kept them separate in the grammar as well. one option to make things more uniform here is to put the not handling for is null in a separate rule. open to other suggestions as well to make this clearer. for null vs unknown, I've kept them separate since the rule for boolean places more restrictions on type of the lhs. afaik, unknown expects that the lhs type is null or boolean but when checking null, the lhs can be any type or null. for example, if you test a boolean column with unknown or null in postgres, it works. switch the column's type to int, and is null type checks but is unknown does not. http://gerrit.cloudera.org:8080/#/c/8014/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS3, Line 1583: > whitespace Done PS3, Line 3034: > whitespace, here and below Done http://gerrit.cloudera.org:8080/#/c/8014/3/fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java File fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java: PS3, Line 26: Class describing a predicate that tests a boolean value using IS. : * The form of a bool test > You should explain more thoroughly what this expr actually does, eg. the ha more detailed comment was in the rewrite.. makes more sense here so moved it. PS3, Line 28: s a bool or null and it returns a bool : * (much like IS [NO > weird line wrapping here Done PS3, Line 33: l > We don't use trailing underscores on constant values. Done PS3, Line 94: thro > Preconditions.checkState Done http://gerrit.cloudera.org:8080/#/c/8014/2/fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java File fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java: Line 29: * Rewrites a bool test predicate to compound expressions. > Please consider adding the rewrite rule (i.e., line 50 and 47) up here to t Done http://gerrit.cloudera.org:8080/#/c/8014/3/fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java File fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java: PS3, Line 39: : public class BoolT > single line Done PS3, Line 64: } : > single line Done http://gerrit.cloudera.org:8080/#/c/8014/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: PS3, Line 164: > whitespace Done http://gerrit.cloudera.org:8080/#/c/8014/3/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: Line 1257: // BoolTestPredicate. > modify one of these tests to use "NOT" added a test using not, but I'm not sure if I've captured what you're after. -- To view, visit http://gerrit.cloudera.org:8080/8014 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-HasComments: Yes
