impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Date Wed, 13 Sep 2017 17:45:37 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................


Patch Set 7:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

PS7, Line 333: Bool tests
BoolTestPredicates?


http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java:

PS7, Line 28: <bool_expr> IS [NOT] <val>
Replace with the one in L31.


PS7, Line 30: <val> is one of (TRUE | FALSE | UNKNOWN).
Remove, it's kind of redundant.


http://gerrit.cloudera.org:8080/#/c/8014/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

PS6, Line 591: 
> added negative tests and coverage to rewrites and end-to-end. the analysis 
Sorry, I don't think I understand this comment. What do you mean by analysis for the expr
does not do much? Aren't the negative test cases captured by AnalysisError()? I think rewrite
tests should handle the rewrite logic but everything else (analysis related) should be tested
here. Maybe I am missing something.


http://gerrit.cloudera.org:8080/#/c/8014/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

PS7, Line 217: // Incorrect type for LHS.
             :     AnalyzedRewritesError("'foo' is true", rule, "'foo' = TRUE AND 'foo' IS
NOT NULL",
             :         "operands of type STRING and BOOLEAN are not comparable: 'foo' = TRUE");
             :     // No subqueries allowed.
             :     RewritesError(
             :         "(select max(tinyint_col) from functional.alltypessmall) is true",
rule,
             :         "Subqueries are not supported in the select list.");
Hm, it's not clear to me why this should be here and not as an Analysis test.


-- 
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: 7
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