You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Adam Holley (Code Review)" <ge...@cloudera.org> on 2018/07/03 19:52:12 UTC
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Adam Holley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10858
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
IMPALA-7027: Mulitple varchar cast fails with distinct
When AggregateInfo removes duplicates of groupingExprs during the
second pass of rewrites, the CastExprs becomes StringLiterals.
Because the localEquals checks only comparase value, which in this case
is "" for both, the second cast expr is removed. This causes the
second castTo() to fail. This fix adds a check to StringLiteral.localEquals
to compare the type so that different items will not be removed.
Testing:
- Added test to validate distinct with casts
- Ran all FE tests
- Ran all E2E tests
Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
---
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/10858/1
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@65
PS1, Line 65: (!(type_ == null ? that.type_ == null : type_ == that.type_))
This condition is not very readable. It can be simplified to
if (type != null && type_ != that.type_) return false;
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@151
PS1, Line 151: AnalyzesOk("select distinct cast('' as varchar(101)) as a, " +
Need additional test for "select distinct cast('' as varchar(100)) as a, cast('' as varchar(101)) as b from functional.alltypes"
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Jul 2018 15:50:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has abandoned this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
Abandoned
Removing for now to revisit approach.
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
IMPALA-7027: Mulitple varchar cast fails with distinct
When AggregateInfo removes duplicates of groupingExprs during the
second pass of rewrites, the CastExprs becomes StringLiterals.
Because the localEquals checks only comparase value, which in this case
is "" for both, the second cast expr is removed. This causes the
second castTo() to fail. This fix adds a check to StringLiteral.localEquals
to compare the type so that different items will not be removed.
Testing:
- Added test to validate distinct with casts
- Ran all FE tests
- Ran all E2E tests
Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
---
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/10858/2
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
IMPALA-7027: Mulitple varchar cast fails with distinct
When AggregateInfo removes duplicates of groupingExprs during the
second pass of rewrites, the CastExprs becomes StringLiterals.
Because the localEquals checks only comparase value, which in this case
is "" for both, the second cast expr is removed. This should be fine
because we can cast from a smaller varchar to a larger, however
castResultExprs() tries to recast because varchar(100) and varchar(101)
are not strictly equal. This fix adds a check to
StatementBase.castResultExprs() to not try recasting if the current
type length is greater than the target type length.
Testing:
- Added test to validate distinct with casts
- Ran all FE tests
- Ran all E2E tests
Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
---
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
2 files changed, 24 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/10858/3
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10858/4/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
File fe/src/main/java/org/apache/impala/analysis/StatementBase.java:
http://gerrit.cloudera.org:8080/#/c/10858/4/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@115
PS4, Line 115: // If varchar is longer length, the recasting is unnecessary.
> It doesn't feel right putting this logic in StatementBase and the logic see
It can either go here, or have a specific check in base Expr.castTo next to the specific isDecimal check. Where it is, it short circuits several additional checks.
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Jul 2018 14:24:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@63
PS1, Line 63: localEquals
I know we don't currently have some unit tests for this, but I think we should. The unit tests can also provide some sort of documentation what the behavior should be. As far as the CR is concerned, we can limit the scope to only StringLiteral tests.
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@65
PS1, Line 65: (!(type_ == null ? that.type_ == null : type_ == that.type_))
> That wouldn't catch the case where type_==null and that.type_ != null, whic
In that case, you can change it to
if ((type != null || that.type_ != null) && type_ != that.type_) return false;
Doing an inverse of a boolean statement that returns another boolean can be a bit difficult to read.
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Jul 2018 17:11:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
Patch Set 1:
(2 comments)
Reworked because of breaking change to StringLiteral.
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@63
PS1, Line 63: localEquals
> I know we don't currently have some unit tests for this, but I think we sho
N/A because of rework.
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@65
PS1, Line 65: (!(type_ == null ? that.type_ == null : type_ == that.type_))
> In that case, you can change it to
N/A because of rework.
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Jul 2018 15:53:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10858/4/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
File fe/src/main/java/org/apache/impala/analysis/StatementBase.java:
http://gerrit.cloudera.org:8080/#/c/10858/4/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@115
PS4, Line 115: // If varchar is longer length, the recasting is unnecessary.
It doesn't feel right putting this logic in StatementBase and the logic seems too specific for this particular issue.
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Jul 2018 02:57:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@65
PS1, Line 65: (!(type_ == null ? that.type_ == null : type_ == that.type_))
> This condition is not very readable. It can be simplified to
That wouldn't catch the case where type_==null and that.type_ != null, which should also return false. I'll remove the double space though.
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@151
PS1, Line 151: AnalyzesOk("select distinct cast('' as varchar(101)) as a, " +
> Need additional test for "select distinct cast('' as varchar(100)) as a, ca
Done
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Jul 2018 15:25:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
......................................................................
IMPALA-7027: Mulitple varchar cast fails with distinct
When AggregateInfo removes duplicates of groupingExprs during the
second pass of rewrites, the CastExprs becomes StringLiterals.
Because the localEquals checks only compares value, which in this case
is "" for both, the second cast expr is removed. This should be fine
because we can cast from a smaller varchar to a larger, however
castResultExprs() tries to recast because varchar(100) and varchar(101)
are not strictly equal. This fix adds a check to
StatementBase.castResultExprs() to not try recasting if the current
type length is greater than the target type length.
Testing:
- Added test to validate distinct with casts
- Ran all FE tests
- Ran all E2E tests
Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
---
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
2 files changed, 24 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/10858/4
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>