You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Yuanhao Luo (Code Review)" <ge...@cloudera.org> on 2016/10/19 09:14:49 UTC
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Yuanhao Luo has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4753
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4753/1
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 3: Code-Review+2
Did you check whether the expected output of other planner tests changed?
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 4: Verified-1
Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/326/
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 2:
(3 comments)
Thanks for adding the test. Almost good to be merged.
http://gerrit.cloudera.org:8080/#/c/4753/2/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
File testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test:
Line 102: # Subquery with predicate in the WHERE clause
For bugfixes we comment tests like this:
IMPALA-4325: Preserve parenthesis of expressions when rewriting subqueries.
Line 106: select t2.int_col
remove tab
Line 108: where (t2.int_col is not null and (t2.int_col < 0 or t2.int_col > 10) or t2.bigint_col is not null and (t2.bigint_col < 0 or t2.bigint_col > 10))
please chop this up into multiple lines, nicely formatted
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Yuanhao Luo (Code Review)" <ge...@cloudera.org>.
Yuanhao Luo has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 3:
Thank you for your patience.
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Yuanhao Luo (Code Review)" <ge...@cloudera.org>.
Yuanhao Luo has uploaded a new patch set (#3).
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
StmtRewrite lost parentheses of CompoundPredicate in pushNegationToOperands()
and leads to incorrect toSql() result. Even though this issue would not leads
to incorrect result of query, it makes user confuse of the logical operator
precedence of predicates shown in EXPLAIN statement.
Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
2 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4753/3
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Internal Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4753
to look at the new patch set (#5).
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
StmtRewrite lost parentheses of CompoundPredicate in pushNegationToOperands()
and leads to incorrect toSql() result. Even though this issue would not leads
to incorrect result of query, it makes user confuse of the logical operator
precedence of predicates shown in EXPLAIN statement.
Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
2 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4753/5
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Yuanhao Luo (Code Review)" <ge...@cloudera.org>.
Yuanhao Luo has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 3:
Yes, I have walk around other planner tests in this file(but not other files), and the expected output of them would not change. Only the output of the query which there were parentheses in where clause of sub-query would be changed.
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 4: Code-Review+2
rebased
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 5: Code-Review+2
fixed trivial issue with new expected plan
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 1:
(1 comment)
Thanks for finding and fixing this! Let me know if you have trouble with adding a test and I'll help.
http://gerrit.cloudera.org:8080/#/c/4753/1/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 1164: CompoundPredicate compoundPredicate =
Can you add a short planner test for this? See PlannerTest.java. Maybe add it to subquery-rewrite.test
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
Patch Set 3:
Great, thanks! I'll get this merged for you.
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Yuanhao Luo (Code Review)" <ge...@cloudera.org>.
Yuanhao Luo has uploaded a new patch set (#2).
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
StmtRewrite lost parentheses of CompoundPredicate in pushNegationToOperands()
and leads to incorrect toSql() result. Even though this issue would not leads
to incorrect result of query, it makes user confuse of the logical operator
precedence of predicates shown in EXPLAIN statement.
Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
2 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/4753/2
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Re: [Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by Alex Behm <al...@cloudera.com>.
Thanks for your contribution!
On Fri, Oct 21, 2016 at 12:45 AM, Internal Jenkins (Code Review) <
gerrit@cloudera.org> wrote:
> Internal Jenkins has submitted this change and it was merged.
>
> Change subject: IMPALA-4325: StmtRewrite lost parentheses of
> CompoundPredicate
> ......................................................................
>
>
> IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
>
> StmtRewrite lost parentheses of CompoundPredicate in
> pushNegationToOperands()
> and leads to incorrect toSql() result. Even though this issue would not
> leads
> to incorrect result of query, it makes user confuse of the logical operator
> precedence of predicates shown in EXPLAIN statement.
>
> Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
> Reviewed-on: http://gerrit.cloudera.org:8080/4753
> Reviewed-by: Alex Behm <al...@cloudera.com>
> Tested-by: Internal Jenkins
> ---
> M fe/src/main/java/org/apache/impala/analysis/Expr.java
> M testdata/workloads/functional-planner/queries/PlannerTest/
> subquery-rewrite.test
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> Approvals:
> Internal Jenkins: Verified
> Alex Behm: Looks good to me, approved
>
>
>
> --
> To view, visit http://gerrit.cloudera.org:8080/4753
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: merged
> Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
> Gerrit-PatchSet: 6
> Gerrit-Project: Impala-ASF
> Gerrit-Branch: master
> Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
> Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
> Gerrit-Reviewer: Internal Jenkins
> Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>
>
> --
> You received this message because you are subscribed to the Google Groups
> "impala-cr" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to impala-cr+unsubscribe@cloudera.com.
> For more options, visit https://groups.google.com/a/cloudera.com/d/optout.
>
[Impala-ASF-CR] IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
......................................................................
IMPALA-4325: StmtRewrite lost parentheses of CompoundPredicate
StmtRewrite lost parentheses of CompoundPredicate in pushNegationToOperands()
and leads to incorrect toSql() result. Even though this issue would not leads
to incorrect result of query, it makes user confuse of the logical operator
precedence of predicates shown in EXPLAIN statement.
Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Reviewed-on: http://gerrit.cloudera.org:8080/4753
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Internal Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
2 files changed, 28 insertions(+), 1 deletion(-)
Approvals:
Internal Jenkins: Verified
Alex Behm: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/4753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I79bfc67605206e0e026293bf7032a88227a95623
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yuanhao Luo <lu...@software.ict.ac.cn>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Yuanhao Luo <lu...@software.ict.ac.cn>