You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tianyi Wang (Code Review)" <ge...@cloudera.org> on 2018/05/15 00:17:43 UTC
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Tianyi Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10402
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
IMPALA-7029: Clone LHS when rewriting a between predicate
Currently when `lhs BETWEEN x AND y` is rewritten, lhs is not cloned
but it obviously shoud be.
Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
---
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/10402/1
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10402 )
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
Patch Set 2:
Is the invariant that the tree of Exprs supposed to have unique object identity?
If that's true, could we amend some of the tests to assert that? It's tempting to even make this a runtime assertion, but I don't know if we have the equivalent of DCHECK configured on the Java side.
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 May 2018 02:31:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10402 )
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
Patch Set 2:
Tianyi, before we pursue this further let's get together for me to understand what exactly is going wrong.
There should be no requirement to have different expr objects in different expr trees. Arguably, having such a guarantee would be cleaner, but I'd like to understand if there are other subtle issues lurking in the current model and whether we need a more fundamental change.
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 May 2018 18:39:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10402 )
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
Patch Set 1:
Do you have a test case that shows what can break?
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 May 2018 01:16:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10402 )
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
IMPALA-7029: Clone LHS when rewriting a between predicate
Currently when `lhs BETWEEN x AND y` is rewritten, lhs is not cloned
but it obviously shoud be.
Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
---
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/10402/2
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10402 )
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
Patch Set 1:
(1 comment)
> Patch Set 1:
>
> Do you have a test case that shows what can break?
I don't know if it breaks anything in the current state. It breaks things when I was working on IMPALA-4025.
http://gerrit.cloudera.org:8080/#/c/10402/1/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java:
http://gerrit.cloudera.org:8080/#/c/10402/1/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@47
PS1, Line 47: bp.getChild(0).clone(), bp.getChild(1));
> even cleaner to clone all children
Done
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 May 2018 01:22:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10402 )
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10402/1/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java:
http://gerrit.cloudera.org:8080/#/c/10402/1/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@47
PS1, Line 47: bp.getChild(0).clone(), bp.getChild(1));
even cleaner to clone all children
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 May 2018 01:16:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7029: Clone LHS when rewriting a between predicate
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has abandoned this change. ( http://gerrit.cloudera.org:8080/10402 )
Change subject: IMPALA-7029: Clone LHS when rewriting a between predicate
......................................................................
Abandoned
This turns out to be a non-issue. The case should be that I changed some expr in place which is wrong.
--
To view, visit http://gerrit.cloudera.org:8080/10402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I2db3af99593bc207ee8dc2a1550acf21ebd61c41
Gerrit-Change-Number: 10402
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>