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 <vercego...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-HasComments: Yes
