You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org> on 2017/09/08 22:13:34 UTC
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Vuk Ercegovac has uploaded a new change for review.
http://gerrit.cloudera.org:8080/8014
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
10 files changed, 318 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/1
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 2:
(1 comment)
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_UNKNOWN
Treating "unknown" as a keyword conflicts with kudu specification: encoding and compression may both specify "unknown". I can work-around this, but in general, adding "unknown" as a keyword may break existing queries. Any precedence on adding keywords (I saw the handling of "default", for example)?
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 3:
(10 comments)
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
PS3, Line 3034:
whitespace, here and below
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 values using IS.
: * Example: (1 < 1) IS TRUE
You should explain more thoroughly what this expr actually does, eg. the handling of NULLs.
PS3, Line 28: a
: * CompoundPredicate
weird line wrapping here
PS3, Line 33: _
We don't use trailing underscores on constant values.
PS3, Line 94: assert
Preconditions.checkState
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: if (!(expr instanceof BoolTestPredicate))
: return expr;
single line
PS3, Line 64: private BoolTestToCompoundRule() {
: }
single line
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
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"
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 3:
(2 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 handled in line 3034. (And even more weird that IS NULL and IS UNKOWN are handled differently.)
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. The form of a bool
Please consider adding the rewrite rule (i.e., line 50 and 47) up here to the documentation.
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#8).
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
9 files changed, 462 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/8
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#5).
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
9 files changed, 337 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/5
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
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 <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#7).
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
9 files changed, 467 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/7
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 3:
(1 comment)
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_UPDATE,
> Treating "unknown" as a keyword conflicts with kudu specification: encoding
Backed out the approach where "unknown" is a keyword. That lets us not require that "unknown" be reserved, and does not require any changes for the existing kudu spec.
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#6).
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
9 files changed, 337 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/6
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 8:
thanks for the suggestion!
I got the sense that the rewrite approach might be a bit tricky when I saw special casing for between in the analyzer. Would like to see if such small rewrites can be easier however. I'll try the other approach in a separate change.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8014/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:
PS1, Line 3070:
just noticed an issue here for STRUCT<unknown:INT> ... looking into it.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#2).
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
10 files changed, 321 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/2
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#4).
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
9 files changed, 336 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/4
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has abandoned this change. ( http://gerrit.cloudera.org:8080/8014 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Abandoned
Abandoning in favor of https://gerrit.cloudera.org/8122/
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-Change-Number: 8014
Gerrit-PatchSet: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 8:
(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: BooTestPre
> BoolTestPredicates?
Done
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: <expr> IS [NOT] <val>.
> Replace with the one in L31.
Done
PS7, Line 30: ool, compatible with bool or null
> Remove, it's kind of redundant.
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 591: for (String r
> Sorry, I don't think I understand this comment. What do you mean by analysi
ignore that comment. tests are now included here.
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: "(bool_col is null and string_col = '10' and int_col < 10)", rule,
: "int_col < 10 AND bool_col IS NULL AND " +
: "((bigint_col < 10) OR (string_col = '10'))");
: // Negated common conjunct: !(int_col=5 or tinyint_col > 9)
: RewritesOk(
: "(!(int_col=5 or tinyint_col > 9) and double_col = 7) or " +
: "(!(int_col=5 or tinyint_col > 9) and double_col = 8
> Hm, it's not clear to me why this should be here and not as an Analysis tes
agreed, I understood incorrectly that analysis tests also run rewrites and that these rewrite tests are just for testing the result of rewrites, not including the subsequent analysis of the rewritten expression.
removed these tests.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#3).
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Adds a new expression to represent the following boolean predicate:
<expr> IS [NOT] (TRUE | FALSE | UNKNOWN)
The expression is rewritten into compound expressions using
negation, is null, and equality. As a result, there are no BE
operators to eval this predicate.
Testing:
- Frontend: parser, analyzer, rewriter, tosql
- EndToEnd query expressions
Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/BoolTestPredicate.java
A fe/src/main/java/org/apache/impala/rewrite/BoolTestToCompoundRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
9 files changed, 330 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/8014/3
--
To view, visit http://gerrit.cloudera.org:8080/8014
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 8:
Our other approach for these small rewrites can be seen in FunctionCallExpr#createExpr(). Basically we transform directly in the parser. An example of that approach is NVL2().
I still think it makes sense to unify with IsNUllPredicate in the grand scheme of things.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
......................................................................
Patch Set 8:
Vuk, I have concerns regarding the high-level approach. From a bird's eye view this patch adds a lot of new code and complexity for a simple new function.
I think the simplest (and preferable) implementation would look like this:
1. Unify the new expr with the existing IsNullPredicate; the new expr provides a super set of functionality
2. Have a proper BE implementation; the function should have 3 arguments: one is the input expr, another is a boolean indicating negation and the third is an int indicating the kind of check we are doing (checking for isnull/isfalse/istrue). That way we can codegen away the runtime overhead of checking those cases.
The current solution takes the approach of BetweenPredicate which is tricky and error prone. For example, in your patch you forget to add the new rewrite rule into the HdfsPartitionPruner. Overall it's tricky to find all those places, and In think that eventually we may want to change how BetweenPredicate works as well.
What do you think? Happy to discuss.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
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 <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-HasComments: Yes