[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8122 ) Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. .. Patch Set 6: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 6 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 23 Sep 2017 04:33:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8122 ) 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: IS [NOT] (TRUE | FALSE | UNKNOWN) The expression is expanded in the parser to istrue/false for the checks against true and false respectively and to isnull for the check against unknown. Compared to the other approaches (rewrites, extended backend expr), this change is the simplest. Main downside is that error messages are in terms of the lowered expression. Testing: - fe: parser, tosql, analyze exprs - e2e: query exprs Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Reviewed-on: http://gerrit.cloudera.org:8080/8122 Reviewed-by: Alex Behm Tested-by: Impala Public Jenkins --- M be/src/exprs/expr-test.cc M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 6 files changed, 184 insertions(+), 8 deletions(-) Approvals: Alex Behm: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 7 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8122 ) Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 6 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 23 Sep 2017 00:29:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8122 ) Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. .. Patch Set 6: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1263/ -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 6 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 23 Sep 2017 00:29:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/8122 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false,
unknown.
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8122/5/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8122/5/be/src/exprs/expr-test.cc@2551
PS5, Line 2551: TEST_F(ExprTest, BoolTestExpr) {
> BoolTestExpr
Done
--
To view, visit http://gerrit.cloudera.org:8080/8122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40
Gerrit-Change-Number: 8122
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Sat, 23 Sep 2017 00:21:42 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8122 to look at the 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: IS [NOT] (TRUE | FALSE | UNKNOWN) The expression is expanded in the parser to istrue/false for the checks against true and false respectively and to isnull for the check against unknown. Compared to the other approaches (rewrites, extended backend expr), this change is the simplest. Main downside is that error messages are in terms of the lowered expression. Testing: - fe: parser, tosql, analyze exprs - e2e: query exprs Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 --- M be/src/exprs/expr-test.cc M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 6 files changed, 184 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8122/6 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 6 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/8122 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false,
unknown.
..
Patch Set 5: Code-Review+2
(1 comment)
I'll merge after the final simple change
http://gerrit.cloudera.org:8080/#/c/8122/5/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/8122/5/be/src/exprs/expr-test.cc@2551
PS5, Line 2551: TEST_F(ExprTest, IsBoolPredicate) {
BoolTestExpr
--
To view, visit http://gerrit.cloudera.org:8080/8122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40
Gerrit-Change-Number: 8122
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Sat, 23 Sep 2017 00:04:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/8122 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false,
unknown.
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2731
PS1, Line 2731: {:
> How did you conclude that bool_value_expr is the standard terminology? My u
I took the name one-level too high in the spec-- thx for pointing it out.
Agreed that bool_test_expr is a better name so going with that (made the change
in several places).
--
To view, visit http://gerrit.cloudera.org:8080/8122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40
Gerrit-Change-Number: 8122
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 22 Sep 2017 23:43:45 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8122 to look at the 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: IS [NOT] (TRUE | FALSE | UNKNOWN) The expression is expanded in the parser to istrue/false for the checks against true and false respectively and to isnull for the check against unknown. Compared to the other approaches (rewrites, extended backend expr), this change is the simplest. Main downside is that error messages are in terms of the lowered expression. Testing: - fe: parser, tosql, analyze exprs - e2e: query exprs Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 --- M be/src/exprs/expr-test.cc M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 6 files changed, 184 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8122/5 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 5 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/8122 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false,
unknown.
..
Patch Set 4:
(1 comment)
Nice, almost done
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2731
PS1, Line 2731: {:
> Changed it to bool_value_expr to be consistent with the standard.
How did you conclude that bool_value_expr is the standard terminology? My
understanding is that the bool_test_expr you had before is more consistent with
what Greg posted in the JIRA.
If you don't like truth_test_expr (which is fine), I prefer the bool_test_expr
over bool_value_expr.
--
To view, visit http://gerrit.cloudera.org:8080/8122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40
Gerrit-Change-Number: 8122
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 22 Sep 2017 23:14:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Vuk Ercegovac has abandoned this change. ( http://gerrit.cloudera.org:8080/8105 ) 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/8105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ieea87e6fc8eeaf899726809d4daa4053c2bea54c Gerrit-Change-Number: 8105 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/8122 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false,
unknown.
..
Patch Set 4:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2731
PS1, Line 2731: {:
> More precisely this is a truth_test_pred, right? The rule should also be un
Changed it to bool_value_expr to be consistent with the standard.
I put this under non_pred_expr since FunctionCallExpr is an Expr, not a
Predicate.
The workaround I'm including here is to declare nonterminal predicate as an
Expr (not a predicate)-- let me know if that's preferable.
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2742
PS1, Line 2742: RESULT = FunctionCallExpr.createExpr(
> add something like server_ident for handling UNKNOWN as an ident
done-- thanks for the tip, I was looking for something like that.
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@612
PS1, Line 612: public void TestIsNullOrBoolPredicates() throws
AnalysisException {
> Let's add the new tests under a separate TestTruthTestPredicate()
done. changed to consistently named method. we can change them all in one go to
something else if needed.
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@617
PS1, Line 617: AnalysisError("select 1 from functional.allcomplextypes
where int_map_col is null",
> use Java camel-case style: rhsOptions, lhsOptions
Done
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@628
PS1, Line 628: public void TestBoolValueExpression() throws AnalysisException
{
> rhsTruthVals? these are definitely not literals
Done
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@629
PS1, Line 629: String[] rhsOptions = new String[] {"true", "false",
"unknown"};
> camel case
Done
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@648
PS1, Line 648: for (String lhs : lhsOptions) {
> also test the NOT variants since those produce a FunctionCallExpr with a di
Done
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@1449
PS1, Line 1449: ArrayList boolTestVals = new ArrayList();
> Rename literals to boolTestOperations or truthTestVals? Most of these are a
done. went with boolTestVals
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1257
PS1, Line 1257: // Boolean value expression (expanded to istrue/false).
> Truth value test
changed test to expression to be consistent.
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1258
PS1, Line 1258: testToSql("select (true is true)", "SELECT (istrue(TRUE))");
> Let's exhaustively try all possibilities for full coverage
Done
http://gerrit.cloudera.org:8080/#/c/8122/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:
http://gerrit.cloudera.org:8080/#/c/8122/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test@74
PS1, Line 74: # boolean test: IS [NOT] (TRUE | FALSE | UNKNOWN)
> These tests using literals only are more appropriate in expr-test.cc
done. moved the expression tests to expr-test.cc and expanded them a bit.
--
To view, visit http://gerrit.cloudera.org:8080/8122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40
Gerrit-Change-Number: 8122
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 22 Sep 2017 06:20:16 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8122 to look at the 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: IS [NOT] (TRUE | FALSE | UNKNOWN) The expression is expanded in the parser to istrue/false for the checks against true and false respectively and to isnull for the check against unknown. Compared to the other approaches (rewrites, extended backend expr), this change is the simplest. Main downside is that error messages are in terms of the lowered expression. Testing: - fe: parser, tosql, analyze exprs - e2e: query exprs Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 --- M be/src/exprs/expr-test.cc M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 6 files changed, 185 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8122/4 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 4 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8122 to look at the 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: IS [NOT] (TRUE | FALSE | UNKNOWN) The expression is expanded in the parser to istrue/false for the checks against true and false respectively and to isnull for the check against unknown. Compared to the other approaches (rewrites, extended backend expr), this change is the simplest. Main downside is that error messages are in terms of the lowered expression. Testing: - fe: parser, tosql, analyze exprs - e2e: query exprs Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 --- M be/src/exprs/expr-test.cc M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 6 files changed, 185 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8122/3 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8122 to look at the 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: IS [NOT] (TRUE | FALSE | UNKNOWN) The expression is expanded in the parser to istrue/false for the checks against true and false respectively and to isnull for the check against unknown. Compared to the other approaches (rewrites, extended backend expr), this change is the simplest. Main downside is that error messages are in terms of the lowered expression. Testing: - fe: parser, tosql, analyze exprs - e2e: query exprs Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 --- M be/src/exprs/expr-test.cc M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 6 files changed, 187 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8122/2 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Alex Behm
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/8122 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false,
unknown.
..
Patch Set 1:
(11 comments)
Yup, this approach is the winner!
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2731
PS1, Line 2731: bool_test_expr ::=
More precisely this is a truth_test_pred, right? The rule should also be under
predicate and not under non_pred_expr
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2742
PS1, Line 2742: if (!l.toUpperCase().equals("UNKNOWN")) {
add something like server_ident for handling UNKNOWN as an ident
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@612
PS1, Line 612: public void TestIsNullOrBoolPredicates() throws
AnalysisException {
Let's add the new tests under a separate TestTruthTestPredicate()
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@617
PS1, Line 617: String[] rhs_options = new String[] {"true", "false",
"unknown"};
use Java camel-case style: rhsOptions, lhsOptions
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@628
PS1, Line 628: String literal_lhs = rhs == "unknown" ? "null" : rhs;
rhsTruthVals? these are definitely not literals
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@629
PS1, Line 629: String negated_rhs = "not " + rhs;
camel case
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@648
PS1, Line 648: AnalysisError("select ('foo' is true)",
also test the NOT variants since those produce a FunctionCallExpr with a
different fn name
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@1449
PS1, Line 1449: ArrayList literals = new ArrayList();
Rename literals to boolTestOperations or truthTestVals? Most of these are
actually keywords and definitely not literals.
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1257
PS1, Line 1257: // Boolean value test (expanded to istrue/false).
Truth value test
http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1258
PS1, Line 1258: testToSql("select (true is true)", "SELECT (istrue(TRUE))");
Let's exhaustively try all possibilities for full coverage
http://gerrit.cloudera.org:8080/#/c/8122/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:
http://gerrit.cloudera.org:8080/#/c/8122/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test@74
PS1, Line 74: # boolean test: IS [NOT] (TRUE | FALSE | UNKNOWN)
These tests using literals only are more appropriate in expr-test.cc
--
To view, visit http://gerrit.cloudera.org:8080/8122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40
Gerrit-Change-Number: 8122
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Alex Behm
Gerrit-Comment-Date: Fri, 22 Sep 2017 00:47:29 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8122 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: IS [NOT] (TRUE | FALSE | UNKNOWN) The expression is expanded in the parser to istrue/false for the checks against true and false respectively and to isnull for the check against unknown. Compared to the other approaches (rewrites, extended backend expr), this change is the simplest. Main downside is that error messages are in terms of the lowered expression. Testing: - fe: parser, tosql, analyze exprs - e2e: query exprs Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 --- M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 5 files changed, 170 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8122/1 -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac
[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/8105 Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. .. IMPALA-1767 Adds predicate to test boolean values true, false, unknown. Replaces IsNullPredicate with IsNullOrBooleanPredicate in the frontend to handle testing against null and boolean values. Similarly, the backend expression is replaced with a corresponding expression that handles the additional tests. This change is a replacement candidate for gerrit.cloudera.org/8014, which is based on front-end rewrites. Added tests: - Frontend: parser, analyzer, tosql - EndToEnd query expressions Change-Id: Ieea87e6fc8eeaf899726809d4daa4053c2bea54c --- M be/src/codegen/impala-ir.cc M be/src/exprs/CMakeLists.txt M be/src/exprs/expr-test.cc A be/src/exprs/is-null-or-bool-predicate-ir.cc A be/src/exprs/is-null-or-bool-predicate.h D be/src/exprs/is-null-predicate-ir.cc D be/src/exprs/is-null-predicate.h M be/src/exprs/scalar-expr-evaluator.cc M be/src/exprs/scalar-expr.cc M common/function-registry/impala_functions.py M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java A fe/src/main/java/org/apache/impala/analysis/IsNullOrBoolPredicate.java D fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.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 26 files changed, 670 insertions(+), 347 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/8105/1 -- To view, visit http://gerrit.cloudera.org:8080/8105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ieea87e6fc8eeaf899726809d4daa4053c2bea54c Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: IS [NOT] . > 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: IS [NOT]
Replace with the one in L31.
PS7, Line 30: 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
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 )
d
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Vuk Ercegovac Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Vuk Ercegovac Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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: 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
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 ... 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Vuk Ercegovac Gerrit-HasComments: Yes
[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: 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
