impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Date Mon, 11 Sep 2017 21:02:06 GMT
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 <vercegovac@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message