[Impala-ASF-CR] IMPALA-6114: Require type equality for NumericLiteral::localEquals().

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality for 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality for NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals.

A test case has been added to AnalyzeStmtsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Reviewed-on: http://gerrit.cloudera.org:8080/8448
Reviewed-by: Alex Behm 
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
2 files changed, 11 insertions(+), 1 deletion(-)

Approvals:
  Alex Behm: Looks good to me, approved
  Impala Public Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6114: Require type equality for NumericLiteral::localEquals().

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality for 
NumericLiteral::localEquals().
..


Patch Set 6: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 15 Dec 2017 01:28:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 5:

Thanks for the comments. Please have a look at patch set #6


--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:31:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8448/5/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java
File fe/src/main/java/org/apache/impala/analysis/NullLiteral.java:

http://gerrit.cloudera.org:8080/#/c/8448/5/fe/src/main/java/org/apache/impala/analysis/NullLiteral.java@51
PS5, Line 51:   public boolean localEquals(Expr that) {
I looked into this a little deeper and concluded that it's safe to consider 
NullLiterals equal regardless of type. The reason is that a NullLiteral is 
compatible with any other type and can be cast to any other type.

So let's revert this change. Sorry for the back-and-forth.


http://gerrit.cloudera.org:8080/#/c/8448/5/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/8448/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2586
PS5, Line 2586: AnalyzesOk(String.format("select distinct cast(0 as 
decimal(14)), 0 from " +
> Add a test that covers the NullLiteral change
* Test is misplaced. Let's move it into AnalyzeStmtsTest#TestDistinct
* Simplify test: No need for String.format() and no need to pass in a custom 
analyzer with queryOptions.
* Typically we prefix an issue with the JIRA, e.g.:

IMPALA-6144: Test that numeric...



--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 14 Dec 2017 07:09:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8448/5/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/8448/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2586
PS5, Line 2586: AnalyzesOk(String.format("select distinct cast(0 as 
decimal(14)), 0 from " +
Add a test that covers the NullLiteral change



--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 12 Dec 2017 21:58:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Dimitris Tsirogiannis, Alex Behm,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8448

to look at the new patch set (#5).

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality of NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals. A similar check has been
added to NullLiteral as well.

A test case has been added to AnalyzeExprsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
---
M fe/src/main/java/org/apache/impala/analysis/NullLiteral.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 22 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/8448/5
--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 3:

(1 comment)

Thanks for the comments. Please see patch set #4.

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
File fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java:

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@141
PS2, Line 141: if ((isAnalyzed() && tmp.isAnalyzed()) && 
(!getType().equals(tmp.getType( return false;
> 2. NullLiterals may be cast to any type. Casts are applied in-place and wil
NullLiteral::localEquals() added.



--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 12 Dec 2017 21:17:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Dimitris Tsirogiannis, Alex Behm,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8448

to look at the new patch set (#4).

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality of NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals. A test case has been added
to AnalyzeExprsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 13 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/8448/4
--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
File fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java:

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@141
PS2, Line 141: if ((isAnalyzed() && tmp.isAnalyzed()) && (type_ != 
tmp.getType())) return false;
> 1. Done.
2. NullLiterals may be cast to any type. Casts are applied in-place and will 
directly change the type of literals (including NullLiteral).



--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 08 Dec 2017 23:30:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
File fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java:

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@141
PS2, Line 141: if ((isAnalyzed() && tmp.isAnalyzed()) && 
(!getType().equals(tmp.getType( return false;
> 1. Use equals() for the type
1. Done.
2. Not clear about how to add this for NullLiteral, because its type is always 
NULL.

Test case has been added.



--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 08 Dec 2017 20:35:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality of NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals. A test case has been added
to AnalyzeExprsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 13 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/8448/3
--
To view, visit http://gerrit.cloudera.org:8080/8448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga