Vuk Ercegovac has posted comments on this change. Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. ......................................................................
Patch Set 7: (21 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 Done. removed in latest patch. PS1, Line 2878: > same here Done. removed in latest patch. Line 2891: | UNMATCHED_STRING_LITERAL:l expr:e > nit: extra spaces (see marked as red). Done. removed in latest patch. 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 e Done. reverted this change when I moved away from using a keyword. 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(BetweenToCompoundRule.INSTANCE); > Maybe add a brief comment about this rule as well? Both rules treated the same way, so updated this comment. PS6, Line 1093: ss > nit: them Done PS6, Line 1098: : // TODO: add a method to Expr to take care of this. > remove. Similar comment above (L1082). Done 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: > "a boolean test predicate."? I think you can also add the grammar spec here Done PS2, Line 33: This predicate needs to be rewritten into a Compo > For literals we usually make them static as well; also, remove "_" from the Done PS2, Line 38: ivate > No need for "this". We use "_" in the name to indicate private fields. Done PS2, Line 41: blic BoolTestPredicate(Expr e, LiteralExpr v, boolean isNegated) { : super(); > You can use the addChild() function here, same thing. Done PS2, Line 88: analyzeImpl(analyzer); > Will that work with something like "select 1 is true"? For instance, "selec since "select 1 = true" is supported, it makes sense to inject an implicit cast here as well. I changed this so that I don't type-check here since it would duplicate the type-checking that "eq" already does. so, to be consistent with "eq", I let the lhs through and assume that the analyzed rewritten expr will handle type-checking (and subqueries, etc). the main downside I see with this is that the error message from "eq" is exposed, which could be surprising to the user since its at a lower level of abstraction. PS2, Line 94: LHS is type-checked follow > Similar comment as above. e.g. select 1 is 1; I've specified the parser rule to require that the rhs is true, false, or unknown (that's per the standard). I'd prefer to check as-is; let me know if you think it makes more sense to relax the parser and move more flexibility here. 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: > "a compound predicate." Done Line 64: result = new CompoundPredicate(CompoundPredicate.Operator.NOT, result, null); > Preconditions.checkNotNull(result); Done 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 Done PS6, Line 580: TestBoolTestPredicate > A few more test cases to consider: added select list tests here added the others to the rewrites and end-to-end since the analysis pass does not do much now. if there is a sure-fire way to be consistent with the casting rules used by "=" (and without code duplication), it would make more sense to add the checks to analysis (and therefore more tests here). PS6, Line 586: > Also use "unknown" whoops, those nulls should not have been here. PS6, Line 591: > This doesn't seem to be consistent with how we handle for instance expr lik see comment above. PS6, Line 591: > Add a few more negative cases. Example: added negative tests and coverage to rewrites and end-to-end. the analysis for this expr does not do as much. 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: RewritesOk("50.0 between null and 5000", rule, > Did you figure that out? If so, remove the TODO. the rewritten query does not come with parens, as shown, but I verified that the sql string returned from the shell (e.g., via a show create table <view>) does retain parens. -- 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
