Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. ......................................................................
Patch Set 6: (22 comments) http://gerrit.cloudera.org:8080/#/c/8014/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS1, Line 2876: don't think you need that PS1, Line 2878: same here Line 2891: | UNMATCHED_STRING_LITERAL:l expr:e nit: extra spaces (see marked as red). http://gerrit.cloudera.org:8080/#/c/8014/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS2, Line 273: KW_UNBOUNDED, KW_UNCACHED, KW_UNION, KW_UPDATE, KW_UPDATE_FN, KW_UPSERT, KW_USE, nit: long line (see vertical separator, that's ok for .test files but for everything else, we try to stay < 90). http://gerrit.cloudera.org:8080/#/c/8014/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: PS6, Line 337: rules.add(BoolTestToCompoundRule.INSTANCE); Maybe add a brief comment about this rule as well? PS6, Line 1093: it nit: them PS6, Line 1098: For all conjuncts other than : // the ones listed below, the method should be a no-op. remove. Similar comment above (L1082). http://gerrit.cloudera.org:8080/#/c/8014/2/fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java File fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java: PS2, Line 26: predicate that tests a boolean value using IS. "a boolean test predicate."? I think you can also add the grammar spec here besides the example. PS2, Line 33: it to be executable, i.e., it is illegal to call For literals we usually make them static as well; also, remove "_" from the name. PS2, Line 38: ivate No need for "this". We use "_" in the name to indicate private fields. PS2, Line 41: super(); : this.isNegated_ = You can use the addChild() function here, same thing. PS2, Line 88: Will that work with something like "select 1 is true"? For instance, "select 1 = true" returns true. Type is not boolean but it can be casted into one. PS2, Line 94: new AnalysisException("Ex Similar comment as above. e.g. select 1 is 1; 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: PS2, Line 29: expressions nit re terminology: "compound predicate". Technically, it is an expression (subclass of Expr) but try to use the more specific term. http://gerrit.cloudera.org:8080/#/c/8014/6/fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java File fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java: PS6, Line 29: to compound expressions "a compound predicate." Line 64: } Preconditions.checkNotNull(result); 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 578: nit: extra space PS6, Line 580: TestBoolTestPredicate A few more test cases to consider: 1. bool test predicate in the select list 2. nested bool test predicates, e.g. ((bool_col is true) is true) 3. other bool exprs than bool_col, e.g. function returning bool, case expr 4. wrap the bool test in an istrue/isfalse function PS6, Line 586: null Also use "unknown" PS6, Line 591: where 1 is true" This doesn't seem to be consistent with how we handle for instance expr like 1 = true (we allow that). PS6, Line 591: AnalysisError Add a few more negative cases. Example: 1. subquery 2. literals that can't be cast to true/false, e.g. 10 is true http://gerrit.cloudera.org:8080/#/c/8014/6/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: PS6, Line 168: // TODO: figure out how/if parens are printed to reflect expression tree. Did you figure that out? If so, remove the TODO. -- 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
