[Impala-ASF-CR] IMPALA-2422: Fix escaping in the LIKE clause

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/10910 )

Change subject: IMPALA-2422: Fix escaping in the LIKE clause
..

IMPALA-2422: Fix escaping in the LIKE clause

There are two stages to processing a like clause. First, we determine if
it is possible to convert the expression to a simpler function, such as
StartsWith() or EndsWith(). If not, then we use a Regex libarary to
compute the result.

There was a problem in the logic that determines if it is possible to
use a simpler function. It did not take into account escape characters.
For example, "abc\%" was incorrectly converted to StartsWith("abc\").

There was another problem. We always unescaped strings in the frontend.
The RE2 regex function also unescapes the regex before proceeding. So
regexes were unescaped twice, which caused some ambiguity. For example,
"abc\%" and "abc\\%" are unescaped in the frontend and the same pattern,
"abc\%" is sent to the backend. The backend could not decide if this
pattern is an exact or prefix match. To fix this problem, we avoid
unescaping regex pattens in the frontend.

Testing:
 -Added expr tests.

Change-Id: I553412318525820a36d2f401aa7e93958d22f70e
---
M be/src/exprs/expr-test.cc
M be/src/exprs/like-predicate.cc
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 68 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10910/2
--
To view, visit http://gerrit.cloudera.org:8080/10910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I553412318525820a36d2f401aa7e93958d22f70e
Gerrit-Change-Number: 10910
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10898 )

Change subject: IMPALA-7211: Fix the between predicate for decimals
..


Patch Set 3: Code-Review+2

Rebased. Forwarding the +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
Gerrit-Change-Number: 10898
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 11 Jul 2018 06:05:21 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/10898 )

Change subject: IMPALA-7211: Fix the between predicate for decimals
..

IMPALA-7211: Fix the between predicate for decimals

Before this patch, some queries would fail where the inputs to the
between predicate were decimal types that are not compatible with each
other. We would needlessly cast them to the same type in the FE. This
was not necessary because we are able to handle decimals of different
types in the backend already for this predicate. This patch should even
result in a slight performance improvement.

Testing:
- Added some tests to AnalyzeExprsTest.

Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 51 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
Gerrit-Change-Number: 10898
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10898 )

Change subject: IMPALA-7211: Fix the between predicate for decimals
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10898/2/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@54
PS2, Line 54: that
> nit: then
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
Gerrit-Change-Number: 10898
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 11 Jul 2018 05:59:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2422: Fix escaping in the LIKE clause

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10910


Change subject: IMPALA-2422: Fix escaping in the LIKE clause
..

IMPALA-2422: Fix escaping in the LIKE clause

There are two stages to processing a like clause. First, we determine if
it is possible to convert the expression to a simpler function, such as
StartsWith() or EndsWith(). If not, then we use a Regex libarary to
compute the result.

There was a problem in the logic that determines if it is possible to
use a simpler function. It did not take into account escape characters.
For example, "abc\%" was incorrectly converted to StartsWith("abc\").

There was another problem. We always unescaped strings in the frontend.
The RE2 regex function also unescapes the regex before proceeding. So
regexes were unescaped twice, which caused some ambiguity. For example,
"abc\%" and "abc\\%" are unescaped in the frontend and the same pattern,
"abc\%" is sent to the backend. The backend could not decide if this
pattern is an exact or prefix match. To fix this problem, we avoid
unescaping regex pattens in the frontend.

Testing:
 -Added expr tests.

Change-Id: I553412318525820a36d2f401aa7e93958d22f70e
---
M be/src/exprs/expr-test.cc
M be/src/exprs/like-predicate.cc
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 77 insertions(+), 23 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10910/1
--
To view, visit http://gerrit.cloudera.org:8080/10910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I553412318525820a36d2f401aa7e93958d22f70e
Gerrit-Change-Number: 10910
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-7211: Fix the between predicate for decimals

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/10898 )

Change subject: IMPALA-7211: Fix the between predicate for decimals
..

IMPALA-7211: Fix the between predicate for decimals

Before this patch, some queries would fail where the inputs to the
between predicate were decimal types that are not compatible with each
other. We would needlessly cast them to the same type in the FE. This
was not necessary because we are able to handle decimals of different
types in the backend already for this predicate. This patch should even
result in a slight performance improvement.

Testing:
- Added some tests to AnalyzeExprsTest.

Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 51 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/10898/2
--
To view, visit http://gerrit.cloudera.org:8080/10898
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
Gerrit-Change-Number: 10898
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] PREVIEW: IMPALA-7211: Fix the between predicate for decimals

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10898 )

Change subject: PREVIEW: IMPALA-7211: Fix the between predicate for decimals
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java:

http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@60
PS1, Line 60: (
> remove or should something be added here?
Done


http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@62
PS1, Line 62: noFloats
> worth it to add a comment about why floats are specially handled?
Done


http://gerrit.cloudera.org:8080/#/c/10898/1/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java@72
PS1, Line 72:   for(int i = 0; i < children_.size(); ++i) {
> if I read this correctly, for children with types decimal, int, int, we'll
Added a comment



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
Gerrit-Change-Number: 10898
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 10 Jul 2018 23:48:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/10888 )

Change subject: IMPALA-7260: Fix decimal binary predicates
..

IMPALA-7260: Fix decimal binary predicates

When casting the inputs to a function call, we would try to cast non
decimal numbers to a specific decimal type even though this is not
necessary. The specific decimal type would be calculated by looking at
all the inputs. It was possible for this calculation to fail due to some
decimal types being incompatible with each other.

We solve the problem by casting the non-decimal numerical inputs to a
generic getMinResolutionDecimal().

Testing:
- Added some analysis tests.

Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 23 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4
Gerrit-Change-Number: 10888
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10882 )

Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN 
predicate
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d
Gerrit-Change-Number: 10882
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 10 Jul 2018 21:58:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] PREVIEW: IMPALA-7211: Fix the between predicate for decimals

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10898


Change subject: PREVIEW: IMPALA-7211: Fix the between predicate for decimals
..

PREVIEW: IMPALA-7211: Fix the between predicate for decimals

Before this patch, some queries would fail where the inputs to the
between predicate were decimal types that are not compatible with each
other. We would needlessly cast them to the same type in the FE. This
was not necessary because we are able to handle decimals of different
types in the backend already for this predicate. This patch should even
result in a slight performance improvement.

Testing:
- Added some tests to AnalyzeExprsTest.

Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 43 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/10898/1
--
To view, visit http://gerrit.cloudera.org:8080/10898
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
Gerrit-Change-Number: 10898
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10882 )

Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN 
predicate
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2286
PS3, Line 2286: Collections.sort(sortedExprs, new Comparator() {
> The code actually puts all the decimals at the beginning. If we handle non-
Ok, I think it's fine the way it is written now.


http://gerrit.cloudera.org:8080/#/c/10882/3/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/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2451
PS3, Line 2451: 2 as decimal(38, 37)
> I have test cases for that in L2464 and L2491.
Yeah, but it would be better to have decimals that are incompatible with each 
other here also. This test case is not adding anything new the way it is 
written now. Many of these tests would pass even without this patch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d
Gerrit-Change-Number: 10882
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 10 Jul 2018 03:25:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/10888 )

Change subject: IMPALA-7260: Fix decimal binary predicates
..

IMPALA-7260: Fix decimal binary predicates

When casting the inputs to a function call, we would try to cast non
decimal numbers to a specific decimal type even though this is not
necessary. The specific decimal type would be calculated by looking at
all the inputs. It was possible for this calculation to fail due to some
decimal types being incompatible with each other.

We solve the problem by casting the non-decimal numerical inputs to a
generic getMinResolutionDecimal().

Testing:
- Added some analysis tests.

Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 18 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/10888/2
--
To view, visit http://gerrit.cloudera.org:8080/10888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4
Gerrit-Change-Number: 10888
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10888 )

Change subject: IMPALA-7260: Fix decimal binary predicates
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10888/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/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@265
PS1, Line 265:   AnalyzesOk("select cast(1 as decimal(38,37)) " + operator 
+ " cast(2 as bigint)");
> add a comment explaining what this is trying to test.
Done


http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2418
PS1, Line 2418: testDecimalExpr(decimal_5_5 + " + cast(1 as tinyint)",
> these tests look more specific than the added ones since the return type is
No we don't get lucky here. Arithmetic operations go through a different code 
path.


http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2496
PS1, Line 2496:   public void TestDecimalFunctions() throws AnalysisException {
> worth it to add cases here for non-binary predicates?
I don't think so. This patch only affects the behavior of decimal binary 
predicates.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4
Gerrit-Change-Number: 10888
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 09 Jul 2018 22:12:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7260: Fix decimal binary predicates

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10888


Change subject: IMPALA-7260: Fix decimal binary predicates
..

IMPALA-7260: Fix decimal binary predicates

When casting the inputs to a function call, we would try to cast non
decimal numbers to a specific decimal type even though this is not
necessary. The specific decimal type would be calculated by looking at
all the inputs. It was possible for this calculation to fail due to some
decimal types being incompatible with each other.

We solve the problem by casting the non-decimal numerical inputs to a
generic getMinResolutionDecimal().

Testing:
- Added some analysis tests.

Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 14 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/10888/1
--
To view, visit http://gerrit.cloudera.org:8080/10888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4
Gerrit-Change-Number: 10888
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10882 )

Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN 
predicate
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2286
PS3, Line 2286: Collections.sort(sortedExprs, new Comparator() {
It looks like we are sorting here in order to put the decimals at the end of 
the list.

Do you think it would be simpler if we just did 2 passes over exprs instead 
(without creating a new list)? Where we handle the non-decimals in the first 
pass and the decimals in the second pass.


http://gerrit.cloudera.org:8080/#/c/10882/3/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/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2451
PS3, Line 2451: 2 as decimal(38, 37)
One should be decimal(38,37) and the other one decimal(38,1) - to make sure 
that the decimals are not compatible with each other.

Here and elsewhere.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d
Gerrit-Change-Number: 10882
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 06 Jul 2018 23:25:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7202: Add width bucket() to the decimal fuzz test

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10859 )

Change subject: IMPALA-7202: Add width_bucket() to the decimal fuzz test
..


Patch Set 1:

Yeah, those should probably be merged before this change, because our build 
would get broken.

Alternatively, we can merge this change, but disable this test with an xfail. 
The test can be re-enabled in the IMPALA-7242 patch by whoever fixes the Jira.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0
Gerrit-Change-Number: 10859
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 06 Jul 2018 00:45:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7202: Add width bucket() to the decimal fuzz test

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10859


Change subject: IMPALA-7202: Add width_bucket() to the decimal fuzz test
..

IMPALA-7202: Add width_bucket() to the decimal fuzz test

In this patch we include the newly added width_bucket() function in the
decimal fuzz test.

Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0
---
M tests/query_test/test_decimal_fuzz.py
1 file changed, 57 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/10859/1
--
To view, visit http://gerrit.cloudera.org:8080/10859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0
Gerrit-Change-Number: 10859
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-7236: Fix the parsing of ALLOW ERASURE CODED FILES

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10857 )

Change subject: IMPALA-7236: Fix the parsing of ALLOW_ERASURE_CODED_FILES
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife1e791541e3f4fed6bec00945390c7d7681e824
Gerrit-Change-Number: 10857
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 03 Jul 2018 18:49:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 21: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 21
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Sat, 30 Jun 2018 01:25:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 20:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6023/20/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/6023/20/be/src/exprs/math-functions-ir.cc@565
PS20, Line 565:   ctx->SetError("Encountered division by 0");
> clang fails with division by 0. Since we check for
This is not the right way to deal with this. I looked at it again, and I really 
don't think it's possible for y to be zero. This needless check will make the 
code slower.

Include this comment on the line that is causing the problem to suppress that 
clang error:

// NOLINT: clang-tidy thinks y may equal zero here.

You just need to put this on line 584 I think



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 20
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 29 Jun 2018 21:54:10 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7102 (Part 1): Disable reading of erasure coding by default

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10646 )

Change subject: IMPALA-7102 (Part 1): Disable reading of erasure coding by 
default
..


Patch Set 4: Code-Review+2

Fixed a typo in the test file. Forwarding the +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3
Gerrit-Change-Number: 10646
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Adrian Ng
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Jun 2018 20:10:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7102(Part 1): Disable reading of erasure coding by default

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/10646 )

Change subject: IMPALA-7102(Part 1): Disable reading of erasure coding by 
default
..

IMPALA-7102(Part 1): Disable reading of erasure coding by default

In this patch we add a query option ALLOW_ERASURE_CODED_FILES, that
allows us to enable or disable the support of erasure coded files. Even
though Impala should be able to handle HDFS erasure coded files already,
this feature hasn't been tested thoroughly yet. Also, Impala lacks
metrics, observability and DDL commands related to erasure coding. This
is a query option instead of a startup flag because we want to make it
possible for advanced users to enable the feature.

We may also need a follow on patch to also disable the write path with
this flag.

Cherry-picks: not for 2.x

Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M bin/run-all-tests.sh
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M testdata/bin/create-load-data.sh
A testdata/workloads/functional-query/queries/QueryTest/hdfs-erasure-coding.test
M tests/common/custom_cluster_test_suite.py
M tests/common/skip.py
M tests/query_test/test_observability.py
M tests/query_test/test_scanners.py
12 files changed, 61 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/10646/2
--
To view, visit http://gerrit.cloudera.org:8080/10646
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3
Gerrit-Change-Number: 10646
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Adrian Ng
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7102: Disable support of erasure coding by default

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10646 )

Change subject: IMPALA-7102: Disable support of erasure coding by default
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10646/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10646/1//COMMIT_MSG@9
PS1, Line 9: In this patch we add a query option ALLOW_ERASURE_CODED_FILES, that
> I had a high-level question: what's the rationale for making it a query opt
Updated the commit message.

The feature should work in principle and has been tested to a certain extent 
already and it works. It wouldn't hurt to make it possible for advanced users 
to be able to enabled the feature if they know what they are doing.


http://gerrit.cloudera.org:8080/#/c/10646/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/10646/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@786
PS1, Line 786:   "The query involves scanning an erasure coded file 
%s located in %s. " +
Updated the message



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3
Gerrit-Change-Number: 10646
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Adrian Ng
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 27 Jun 2018 00:01:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7149: Disable some tests in the EC build

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10804


Change subject: IMPALA-7149: Disable some tests in the EC build
..

IMPALA-7149: Disable some tests in the EC build

We temporarily disable the resource limits tests in the EC build to
make it pass. We also disable the tests marked with
"tuned_for_minicluster" in the EC build.

Cherry-picks: not for 2.x.

Change-Id: I0975b1a28b318625f853b612bdfea3a8adcd776e
---
M tests/common/skip.py
M tests/query_test/test_resource_limits.py
2 files changed, 4 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/10804/1
--
To view, visit http://gerrit.cloudera.org:8080/10804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0975b1a28b318625f853b612bdfea3a8adcd776e
Gerrit-Change-Number: 10804
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 16: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 16
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 22 Jun 2018 23:46:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6642 (Part 2): clean up start-impala-cluster.py

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/10780 )

Change subject: IMPALA-6642 (Part 2): clean up start-impala-cluster.py
..

IMPALA-6642 (Part 2): clean up start-impala-cluster.py

We clean up start-impala-cluster.py in general in this patch by using
logging instead of "print" and formatting strings using the format()
function. We make sure to include a timestamp in each log message in
order to make it easier to debug failures in custom cluster tests that
happen when starting the cluster.

Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5
---
M bin/start-impala-cluster.py
M tests/common/impala_service.py
2 files changed, 147 insertions(+), 83 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/80/10780/2
--
To view, visit http://gerrit.cloudera.org:8080/10780
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5
Gerrit-Change-Number: 10780
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10780 )

Change subject: IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10780/1/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

http://gerrit.cloudera.org:8080/#/c/10780/1/bin/start-impala-cluster.py@460
PS1, Line 460: print 'Error starting cluster: {0}'.format(e)
> If you switch to logging, this becomes just "logging.exception("Error start
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5
Gerrit-Change-Number: 10780
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 22 Jun 2018 23:02:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10780


Change subject: IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py
..

IMPALA-6642 (Part 2): Add timestamps to start-impala-cluster.py

We add some timestamps to the output of start-impala-cluster.py in
order to make it easier to debug failures in custom cluster tests that
happen when starting the cluster.

Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5
---
M bin/start-impala-cluster.py
M tests/common/impala_service.py
2 files changed, 22 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/80/10780/1
--
To view, visit http://gerrit.cloudera.org:8080/10780
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5
Gerrit-Change-Number: 10780
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 15:

Also, I agree with Tim that it's a good idea to add this to the fuzz test. I 
think that it is ok to do that in a separate commit.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 15
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 20 Jun 2018 22:57:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 15:

I already let Anuj know offline that it's ok to remove the precondition.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 15
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 20 Jun 2018 22:55:22 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7149: Skip q7 in test mem usage scaling in erasure coding build

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10647


Change subject: IMPALA-7149: Skip q7 in test_mem_usage_scaling in erasure 
coding build
..

IMPALA-7149: Skip q7 in test_mem_usage_scaling in erasure coding build

The test is flaky in the erasure coding build. Let's disable it for now.

Change-Id: Ic9a34a91eef40e1da9c7134cfb7054006d9115de
---
M tests/query_test/test_mem_usage_scaling.py
1 file changed, 2 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/10647/1
--
To view, visit http://gerrit.cloudera.org:8080/10647
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic9a34a91eef40e1da9c7134cfb7054006d9115de
Gerrit-Change-Number: 10647
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-7102: Disable support of erasure coding by default

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10646


Change subject: IMPALA-7102: Disable support of erasure coding by default
..

IMPALA-7102: Disable support of erasure coding by default

In this patch we add a query option ALLOW_ERASURE_CODED_FILES, that
allows us to enable or disable the support of erasure coded files. At
this time erasure coding is an experimental feature and is not supported
by Impala, so we want to prevent users from using it without explictly
enabling it.

Cherry-picks: not for 2.x

Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3
---
M be/src/service/query-options.cc
M be/src/service/query-options.h
M bin/run-all-tests.sh
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M testdata/bin/create-load-data.sh
A testdata/workloads/functional-query/queries/QueryTest/hdfs-erasure-coding.test
M tests/common/custom_cluster_test_suite.py
M tests/common/skip.py
M tests/query_test/test_observability.py
M tests/query_test/test_scanners.py
12 files changed, 64 insertions(+), 5 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/10646/1
--
To view, visit http://gerrit.cloudera.org:8080/10646
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icd3b1754541262467a6e67068b0b447882a40fb3
Gerrit-Change-Number: 10646
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-7079: Disable the multiple blocks test in erasure coding build

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10521


Change subject: IMPALA-7079: Disable the multiple blocks test in erasure coding 
build
..

IMPALA-7079: Disable the multiple blocks test in erasure coding build

The test is currently failing in erasure coding build, so disable it to
make the build pass.

Change-Id: I00af0914d907b8dcff69f687f71239e76b6ff335
---
M tests/query_test/test_scanners.py
1 file changed, 1 insertion(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/10521/1
--
To view, visit http://gerrit.cloudera.org:8080/10521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I00af0914d907b8dcff69f687f71239e76b6ff335
Gerrit-Change-Number: 10521
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 15:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6023/15/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

http://gerrit.cloudera.org:8080/#/c/6023/15/fe/src/main/java/org/apache/impala/analysis/Expr.java@490
PS15, Line 490:   result = ((ScalarType)result).getMinResolutionDecimal();
Why do we need this?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 15
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 22 May 2018 00:17:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7019: Schedule EC as remote & disable failed tests

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10413 )

Change subject: IMPALA-7019: Schedule EC as remote & disable failed tests
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I138738d3e28e5daa1718c05c04cd9dd146c4ff84
Gerrit-Change-Number: 10413
Gerrit-PatchSet: 5
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Mon, 21 May 2018 21:33:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7039: Ignore the port in HBase planner tests

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10459


Change subject: IMPALA-7039: Ignore the port in HBase planner tests
..

IMPALA-7039: Ignore the port in HBase planner tests

Before this patch, we used to check the HBase port in the HBase planner
tests. This caused a failure when HBase was running on a different port
than expected. We fix the problem in this patch by not checking the
HBase port.

Testing: ran the FE tests and they passed.

Change-Id: I8eb7628061b2ebaf84323b37424925e9a64f70a0
---
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M testdata/workloads/functional-planner/queries/PlannerTest/hbase.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
3 files changed, 20 insertions(+), 23 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/10459/1
--
To view, visit http://gerrit.cloudera.org:8080/10459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8eb7628061b2ebaf84323b37424925e9a64f70a0
Gerrit-Change-Number: 10459
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-7019: Schedule EC as remote & disable failed tests

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10413 )

Change subject: IMPALA-7019: Schedule EC as remote & disable failed tests
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10413/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java:

http://gerrit.cloudera.org:8080/#/c/10413/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@129
PS4, Line 129: static FileDescriptor createEc(FileStatus fileStatus, 
BlockLocation[] blockLocations,
The logic here seems almost identical to the create() function. Maybe modify 
the create() function to accept the isEC argument instead of creating a new 
function?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I138738d3e28e5daa1718c05c04cd9dd146c4ff84
Gerrit-Change-Number: 10413
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 18 May 2018 23:05:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7019: Schedule EC as remote & disable failed tests

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10413 )

Change subject: IMPALA-7019: Schedule EC as remote & disable failed tests
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10413/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10413/3//COMMIT_MSG@12
PS3, Line 12:
Mention how this patch was tested. Did you run the exhaustive build?


http://gerrit.cloudera.org:8080/#/c/10413/3/tests/common/skip.py
File tests/common/skip.py:

http://gerrit.cloudera.org:8080/#/c/10413/3/tests/common/skip.py@29
PS3, Line 29: IS_ISILON,
: IS_LOCAL,
: IS_HDFS,
: IS_S3,
: IS_ADLS,
: SECONDARY_FILESYSTEM,
: IS_EC
This should be sorted alphabetically


http://gerrit.cloudera.org:8080/#/c/10413/3/tests/common/skip.py@150
PS3, Line 150: doesn't work.
"do not work"


http://gerrit.cloudera.org:8080/#/c/10413/3/tests/util/filesystem_utils.py
File tests/util/filesystem_utils.py:

http://gerrit.cloudera.org:8080/#/c/10413/3/tests/util/filesystem_utils.py@33
PS3, Line 33: os.getenv("ERASURE_CODING")
We want to check if ERASURE_CODING == true here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I138738d3e28e5daa1718c05c04cd9dd146c4ff84
Gerrit-Change-Number: 10413
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 18 May 2018 21:57:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10275 )

Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..


Patch Set 5: Code-Review+2

Added Cherry-picks: not for 2.x
Forwarding the +2.
Thanks, Phil!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 04 May 2018 21:42:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/10275 )

Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..

IMPALA-6949: Add the option to start the minicluster with EC enabled

In this patch we add the "ERASURE_CODING" enviornment variable. If we
enable it, a cluster with 5 data nodes will be created during data
loading and HDFS will be started with erasure coding enabled.

Testing:
I ran the core build, and verified that erasure coding gets enabled in
HDFS. Many of our EE tests failed however.

Cherry-picks: not for 2.x

Change-Id: I397aed491354be21b0a8441ca671232dca25146c
---
M bin/impala-config.sh
M bin/run-all-tests.sh
M testdata/bin/create-load-data.sh
M testdata/bin/setup-hdfs-env.sh
M testdata/cluster/admin
M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl
6 files changed, 41 insertions(+), 9 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/10275 )

Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..

IMPALA-6949: Add the option to start the minicluster with EC enabled

In this patch we add the "ERASURE_CODING" enviornment variable. If we
enable it, a cluster with 5 data nodes will be created during data
loading and HDFS will be started with erasure coding enabled.

Testing:
I ran the core build, and verified that erasure coding gets enabled in
HDFS. Many of our EE tests failed however.

Change-Id: I397aed491354be21b0a8441ca671232dca25146c
---
M bin/impala-config.sh
M bin/run-all-tests.sh
M testdata/bin/create-load-data.sh
M testdata/bin/setup-hdfs-env.sh
M testdata/cluster/admin
M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl
6 files changed, 41 insertions(+), 9 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10275 )

Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10275/3/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/10275/3/bin/impala-config.sh@450
PS3, Line 450: elif [ "${TARGET_FILESYSTEM}" == "hdfs" ]; then
> I'm trying this out and found that == don't work with zsh.
Done


http://gerrit.cloudera.org:8080/#/c/10275/3/bin/impala-config.sh@451
PS3, Line 451:   if [[ "${ERASURE_CODING}" = true ]]; then
> I think we should error out or warn here if MINICLUSTER_PROFILE < 3. I.e.,
Done


http://gerrit.cloudera.org:8080/#/c/10275/3/testdata/bin/setup-hdfs-env.sh
File testdata/bin/setup-hdfs-env.sh:

http://gerrit.cloudera.org:8080/#/c/10275/3/testdata/bin/setup-hdfs-env.sh@80
PS3, Line 80: HDFS_EC_PATH
> Please add:
I agree. I don't think it makes sense to unset it here, because it's not going 
to do anything. Removed.


http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl
File testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl:

http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl@26
PS2, Line 26: cloudera.erasure_coding.enabled
> Add:
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 04 May 2018 01:40:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/10275 )

Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..

IMPALA-6949: Add the option to start the minicluster with EC enabled

In this patch we add the "ERASURE_CODING" enviornment variable. If we
enable it, a cluster with 5 data nodes will be created during data
loading and HDFS will be started with erasure coding enabled.

Testing:
I ran the core build, and verified that erasure coding gets enabled in
HDFS. Many of our EE tests failed however.

Change-Id: I397aed491354be21b0a8441ca671232dca25146c
---
M bin/impala-config.sh
M bin/run-all-tests.sh
M testdata/bin/setup-hdfs-env.sh
M testdata/cluster/admin
M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl
5 files changed, 28 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10275 )

Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..


Patch Set 2:

(5 comments)

Yes, nested data loading succeeds every time I tried it.

http://gerrit.cloudera.org:8080/#/c/10275/2/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/10275/2/bin/impala-config.sh@449
PS2, Line 449: elif [ "${TARGET_FILESYSTEM}" = "hdfs-ec" ]; then
> By making this its own TARGET_FILESYSTEM, you end up disabling a bunch of t
I don't think it makes sense to make erasure coding its own file system. I made 
a separate flag for it.


http://gerrit.cloudera.org:8080/#/c/10275/2/bin/impala-config.sh@454
PS2, Line 454:   echo "Valid values are: hdfs, isilon, s3, local"
> Please update?
No need to update any more.


http://gerrit.cloudera.org:8080/#/c/10275/2/bin/run-all-tests.sh
File bin/run-all-tests.sh:

http://gerrit.cloudera.org:8080/#/c/10275/2/bin/run-all-tests.sh@71
PS2, Line 71:   FE_TEST=false
> Add a comment about why?
Done


http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/bin/setup-hdfs-env.sh
File testdata/bin/setup-hdfs-env.sh:

http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/bin/setup-hdfs-env.sh@80
PS2, Line 80:   hdfs ec -unsetPolicy -path "${HDFS_EC_PATH:=/test-warehouse/}"
> Do you know what this does underneath the covers? Do we need to block while
This does not convert the existing files in the directory. It only affects the 
files that will be placed in the directory in the future. This is why it is 
possible to have some files erasure coded and some not in the same directory.


http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl
File testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl:

http://gerrit.cloudera.org:8080/#/c/10275/2/testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl@26
PS2, Line 26: cloudera.erasure_coding.enabled
> What's this for?
This is needed because running the following command

hdfs ec -enablePolicy -policy RS-3-2-1024k

Causes this error:

RemoteException: enableErasureCodingPolicy is not allowed because 
cloudera.erasure_coding.enabled=false



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Wed, 02 May 2018 21:23:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/10275 )

Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..

IMPALA-6949: Add the option to start the minicluster with EC enabled

In this patch we add the "hdfs-ec" target file system option. If we
enable it, a cluster with 5 data nodes will be created during data
loading and HDFS will be started with erasure coding enabled.

Testing:
I ran the core build and the majority of the EE tests passed.

Change-Id: I397aed491354be21b0a8441ca671232dca25146c
---
M bin/impala-config.sh
M bin/run-all-tests.sh
M buildall.sh
M testdata/bin/setup-hdfs-env.sh
M testdata/cluster/admin
M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl
6 files changed, 24 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/10275/2
--
To view, visit http://gerrit.cloudera.org:8080/10275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6949: Add the option to start the minicluster with EC enabled

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10275


Change subject: IMPALA-6949: Add the option to start the minicluster with EC 
enabled
..

IMPALA-6949: Add the option to start the minicluster with EC enabled

In this patch we add the "hdfs-ec" target file system option. If we
enable it, a cluster with 5 data nodes will be created during data
loading and HDFS will be started with erasure coding enabled.

Change-Id: I397aed491354be21b0a8441ca671232dca25146c
---
M bin/impala-config.sh
M bin/run-all-tests.sh
M buildall.sh
M testdata/bin/setup-hdfs-env.sh
M testdata/cluster/admin
M testdata/cluster/node_templates/common/etc/hadoop/conf/hdfs-site.xml.tmpl
6 files changed, 24 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/10275/1
--
To view, visit http://gerrit.cloudera.org:8080/10275
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I397aed491354be21b0a8441ca671232dca25146c
Gerrit-Change-Number: 10275
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10066 )

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@50
PS10, Line 50:   This data type is suitable for financial and other 
arithmetic calculations where the
We should have a better introduction. We would not be comparing it to FLOAT. We 
should not mention that it's suitable for financial purposes.

How about this:
Precision is the maximum number of decimal digits and scale is the number of 
digits to the right of the decimal point. The data type is useful for storing 
and doing operations on precise decimal values.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@56
PS10, Line 56:   The key differences between this version of 
DECIMAL and the previous
We should move this table to the bottom. Instead of calling it DECIMAL_V1 and 
DECIMAL_v2 we should say "Decimal in Impala 2.x" and "Decimal in Impala 3.x". 
Say that the old behavior can be enabled by disabling the query option "set 
decimal_v2=false"


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@293
PS10, Line 293:   If the precision of the result would be greater than 38, 
Impala truncates the result from
> truncates and rounds, right Taras?
yes, "truncates and rounds"

Let's make a separate section called "Decimal Assignment" as Alex mentioned and 
put UNION in that section.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@668
PS10, Line 668:   expressions to DECIMAL as long as the 
overall number of digits and digits
> Taras?
Alex is right. This contradicts what it says on line 678.

should be "... as number fits within the specified target DECIMAL type without 
overflow"


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@879
PS10, Line 879: Any values that do not fit within the new 
precision and scale are returned as
> Taras, is this really true? I would have expected that we round or error.
No, we do not round. We will error if the query option ABORT_ON_ERROR is set to 
true. If that option is set to FALSE, we will get a NULL and warning that 
conversion failed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 01:09:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Speed up Python dependencies.

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10234 )

Change subject: Speed up Python dependencies.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py@172
PS2, Line 172: if results:
> If the import above fails, then results = [] will never execute and this st
Nice catch. We can get rid of the "if results:" line



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 22:03:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10066 )

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@539
PS10, Line 539: OUBLE
DOUBLE and FLOAT


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@540
PS10, Line 540: error returns.
Maybe add the following.

The reasoning is that the range of the floating point types is much wider than 
the range of the DECIMAL, so not every FLOAT or DOUBLE can be converted to a 
DECIMAL. However, every DECIMAL can be converted to a double or float with a 
loss of precision, which is why we allow implicit casting in this case. This is 
somewhat dangerous, but we made this design choice to match the behavior of 
other SQL engines.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@545
PS10, Line 545:
Add this:
We do not allow this because calculations involving decimals are meant to be 
precise, so we are strict and require explicit casts. However, if an 
approximate type like FLOAT is involved, then our behavior is less strict.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 21:59:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Speed up Python dependencies.

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10234 )

Change subject: Speed up Python dependencies.
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 21:34:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] Speed up Python dependencies.

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10234 )

Change subject: Speed up Python dependencies.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@165
PS1, Line 165: results.append(pool.apply_async(download_package, 
args=[pkg_name.strip(), pkg_version.strip()]))
long line


http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@173
PS1, Line 173: x.get()
What happens if the download fails for some reason? Will this be detected?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 20:03:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10066 )

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@526
PS9, Line 526: 38,0
We should be consistent about putting a space after the comma. Either put it 
everywhere in the DOC or don't put it everywhere.


http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@654
PS9, Line 654: is the INT
 : type with the precision 10.
... because all digits do not fit into DECIMAL(3,0)


http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@681
PS9, Line 681:
There should be no space before the open brace. Here and elsewhere.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 9
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:21:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#10). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are 
compatible in FE
..

IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran a core build which passed.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
34 files changed, 550 insertions(+), 292 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/10
--
To view, visit http://gerrit.cloudera.org:8080/9930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 10
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java:

http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@377
PS8, Line 377:   Analyzer analyzer, AnalyticWindow.Boundary boundary) 
throws AnalysisException {
> No need to pass analyzer.
Done


http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
File fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java:

http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@217
PS8, Line 217: Preconditions.checkState(!t0.isDecimal() && 
!t1.isDecimal());
> remove
Done


http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/Expr.java@429
PS8, Line 429:* If strictDecimal is true, the function will throw an 
exception if it is not possible
> This isn't accurate because we will still allow decimal->double conversions
Done


http://gerrit.cloudera.org:8080/#/c/9930/8/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/9930/8/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@178
PS8, Line 178:* message.
> Needs comment for 'strictDecimal'
Done


http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
File fe/src/main/java/org/apache/impala/catalog/ScalarType.java:

http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@433
PS8, Line 433:* is INVALID_TYPE.
> Especially these very visible and heavily used functions need a comment for
Done


http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/Type.java
File fe/src/main/java/org/apache/impala/catalog/Type.java:

http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/catalog/Type.java@293
PS8, Line 293:* If strictDecimal is true, only consider casts that result 
in no loss of information
> This is a nice description! I suggest replicating it in more places.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 8
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 26 Apr 2018 21:07:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#9). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are 
compatible in FE
..

IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran the BE and FE tests locally and they all passed.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
34 files changed, 547 insertions(+), 292 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/9
--
To view, visit http://gerrit.cloudera.org:8080/9930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 9
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran the BE and FE tests locally and they all passed.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
34 files changed, 520 insertions(+), 288 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/8
--
To view, visit http://gerrit.cloudera.org:8080/9930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 8
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..


Patch Set 7: Code-Review+2

Made a minor fix to widetable.py in patch 6. Rebased. Forwarding the +2 from 
Alex.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 7
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 24 Apr 2018 22:33:40 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#6). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Core build passed. Ran an exhaustive build. The errors discovered by
  the exhaustive build were fixed.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M testdata/common/widetable.py
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/small-query-opt.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M 

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#7). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Core build passed. Ran an exhaustive build. The errors discovered by
  the exhaustive build were fixed.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M testdata/common/widetable.py
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/small-query-opt.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M 

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Core build passed. Ran an exhaustive build. The errors discovered by
  the exhaustive build were fixed.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M testdata/common/widetable.py
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/small-query-opt.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M 

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
File fe/src/main/java/org/apache/impala/catalog/ScalarType.java:

http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@119
PS4, Line 119: Preconditions.checkState(precision <= MAX_PRECISION);
> Pretty sure these should break a few tests. I believe in some places we rel
Yes, it broke some tests. Removed.


http://gerrit.cloudera.org:8080/#/c/9930/4/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/9930/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2570
PS4, Line 2570: AnalysisContext decimalV1Ctx = 
createAnalysisCtx(Catalog.DEFAULT_DB);
> you can remove the Catalog.DEFAULT_DB arg
Done


http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java:

http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2654
PS4, Line 2654: AnalysisContext decimalV1Ctx = 
createAnalysisCtx("functional");
> Use createAnalysisCtx() without an arg. There's no significance to "functio
Done


http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

http://gerrit.cloudera.org:8080/#/c/9930/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@348
PS4, Line 348: if (!expectedPlan.get(0).contains("NotImplementedException") 
&&
> Do startsWith() instead of contains() and expect only the exception name an
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 24 Apr 2018 00:15:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran a core build and almost all tests passed. I still need to fix
  A few tests in PlannerTest.testJoinOrder.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M testdata/workloads/functional-query/queries/QueryTest/insert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
M 

[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran a core build and almost all tests passed. I still need to fix
  A few tests in PlannerTest.testJoinOrder.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/avro-writer.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M testdata/workloads/functional-query/queries/QueryTest/insert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
M 

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..


Patch Set 2:

(19 comments)

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

http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@377
PS2, Line 377:   Analyzer analyzer, AnalyticWindow.Boundary boundary) 
throws AnalysisException {
> analyzer arg not needed anymore
Done


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

http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@217
PS2, Line 217: boolean strict = analyzer.isDecimalV2() && 
(t0.isDecimal() || t1.isDecimal());
> remove?
Done


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

http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@423
PS2, Line 423:* Otherwise, if the function signature contains wildcard 
decimals, each wildcard child
> Describe how the decimalV2 argument changes the behavior of this function.
Done


http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@451
PS2, Line 451: argTypes.append(childType.toString());
> childType.toSql()
Done


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

http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@453
PS2, Line 453: if (analyzer.isDecimalV2() && digitsBefore + digitsAfter > 
38) return Type.INVALID;
> I think it's clearer to not call createClippedDecimaltype() for V2 at all b
Done


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

http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java@72
PS2, Line 72: if (decimalV2 && digitsBefore + digitsAfter > 38) return 
Type.INVALID;
> Let's avoid calling createClippedDecimalType() for V2
Done


http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/catalog/Type.java
File fe/src/main/java/org/apache/impala/catalog/Type.java:

http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/catalog/Type.java@310
PS2, Line 310:   Type t1, Type t2, boolean strict) {
> move into previous line
Done


http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java:

http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@91
PS2, Line 91:   "Operand types: %s = %s.", eqPred.toSql(), 
t0.toString(), t1.toString()));
> also use toSql() for types
Done


http://gerrit.cloudera.org:8080/#/c/9930/2/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/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@1838
PS2, Line 1838: AnalyzesOk("select coalesce(1.8, cast(0 as 
decimal(38,38)))", decimalV1Ctx);
> Don't we already have tests for this below in TestDecimalFunctions()? I thi
I forgot about those. Added explicit context for v1 and v2 there.


http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@1840
PS2, Line 1840: "Cannot resolve DECIMAL types of the 
_impala_builtins.coalesce(DECIMAL(2,1), " +
> Should we maybe omit the database when printing the FunctionName? Most of t
How do we avoid printing the function name? It gets printed when we call 
fn.getName().


http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@1843
PS2, Line 1843: String query = "with x as ( " +
> Why have a WITH clause? A UNION alone should suffice
Done


http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2538
PS2, Line 2538: String.format("Cannot resolve DECIMAL precision and 
scale from NULL type in _impala_builtins.%s function.", alias));
> long lines (here and below)
Done

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..


Patch Set 1:

(27 comments)

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java:

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@385
PS1, Line 385: rangeExpr.getType(), 
orderByElements_.get(0).getExpr().getType(),
> Let's change this to be strict regardless of decimal_v2. My understanding i
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
File fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java:

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@218
PS1, Line 218: false, analyzer.getQueryOptions().isDecimal_v2());
> Since this decimalV2 check is so common, let's add a function in Analyzer d
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@429
PS1, Line 429:   protected void castForFunctionCall(boolean 
ignoreWildcardDecimals, boolean decimal_v2)
> decimalV2 (camel-case)
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@440
PS1, Line 440: if (resolvedWildcardType.isInvalid()) {
> Check this immediately after L433?
Not a good idea because if ignoreWildcardDecimals is true we don't want to 
throw.


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@442
PS1, Line 442:   "Unable to resolve the decimal types of the %s 
function arguments. " +
> Also include the argument types in the error message
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@461
PS1, Line 461: StringBuilder errorMsg = new StringBuilder();
> unused?
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@482
PS1, Line 482:   if (result.isNull()) {
> Let's consistently throw either in this function or in the caller. The call
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Function.java@217
PS1, Line 217:   other.argTypes_[i], this.argTypes_[i], strict, false)) 
{
> This looks like a case that shows that the decimalV2 arg and 'strict' shoul
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
File fe/src/main/java/org/apache/impala/catalog/ScalarType.java:

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@141
PS1, Line 141:   public static ScalarType createClippedDecimalType(
> It feels cleaner to me to move the decimalV2 check to the callers and not i
Done, checking at callers.


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@381
PS1, Line 381:   Preconditions.checkState(scale_ <= 38);
> <= MAX_PRECISION
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@393
PS1, Line 393:   Preconditions.checkState(scale_ <= 38);
> <= MAX_PRECISION
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Type.java
File fe/src/main/java/org/apache/impala/catalog/Type.java:

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Type.java@292
PS1, Line 292:* If strict is true, only consider casts that result in no 
loss of precision.
> Why is the decimalV2 case different than the strict case?
Done


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/catalog/Type.java@296
PS1, Line 296:   Type t1, Type t2, boolean strict, boolean decimal_v2) {
> decimalV2
This is not relevant any more.


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java:

http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@88
PS1, Line 88:   throw new InternalException(String.format(
> Is it guaranteed that both types are decimal at this point?
Actually no. Updated the error message text.

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9930 )

Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran a core build and almost all tests passed. I still need to fix
  A few tests in PlannerTest.testJoinOrder.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/QueryTest/aggregation.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_update.test
M testdata/workloads/functional-query/queries/QueryTest/partition-col-types.test
M testdata/workloads/functional-query/queries/QueryTest/union.test
M testdata/workloads/tpcds/queries/tpcds-decimal_v2-q21.test
41 files changed, 454 insertions(+), 251 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/2
--
To view, visit http://gerrit.cloudera.org:8080/9930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master

[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9927 )

Change subject: IMPALA-6805: Show current database in Impala shell prompt
..


Patch Set 6: Code-Review+1

Looks good to me. David, do you want to give the final +2?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc
Gerrit-Change-Number: 9927
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 06 Apr 2018 05:17:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9927 )

Change subject: IMPALA-6805: Show current database in Impala shell prompt
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9927/5/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9927/5/shell/impala_shell.py@1150
PS5, Line 1150: # args == current_db means -d option was passed but the 
"use [db]" operation failed.
This comments needs to be updated. Move this comment under elif.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc
Gerrit-Change-Number: 9927
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 06 Apr 2018 00:21:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9927 )

Change subject: IMPALA-6805: Show current database in Impala shell prompt
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py@781
PS4, Line 781: db=self.current_db if self.current_db else 
ImpalaShell.DEFAULT_DB)
At this point we don't know if current_db exists or not. I think we should 
always set db to ImpalaShell.DEFAULT_DB here. It will get updated later.


http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py@1148
PS4, Line 1148: args
Why not args.strip() here?


http://gerrit.cloudera.org:8080/#/c/9927/4/shell/impala_shell.py@1156
PS4, Line 1156:   if args.strip('`') == self.current_db:
Make this an "elif".

Also, prompt does not need to be updated because of my first comment. It's a 
good idea to set self.current_db = None here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc
Gerrit-Change-Number: 9927
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 05 Apr 2018 01:33:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9927 )

Change subject: IMPALA-6805: Show current database in Impala shell prompt
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9927/3/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9927/3/shell/impala_shell.py@1151
PS3, Line 1151: strip('`')
why do we strip() here, but not on line 781?


http://gerrit.cloudera.org:8080/#/c/9927/3/shell/impala_shell.py@1153
PS3, Line 1153:   # args == current_db means -d option was passed but the 
database does not exist.
It's confusing to me why args == current_db means that the db does not exist. 
Can you explain this a little better? What are the contents of args and 
current_db?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc
Gerrit-Change-Number: 9927
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 04 Apr 2018 23:43:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9930


Change subject: IMPALA-6518,IMPALA-6340: Check that decimal types are 
compatible in FE
..

IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran a core build and all tests passed

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.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/TypesUtilTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
A testdata/workloads/functional-query/queries/QueryTest/insert_decimal.test
M tests/query_test/test_insert.py
32 files changed, 461 insertions(+), 231 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/9930/1
--
To view, visit http://gerrit.cloudera.org:8080/9930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6805: Show current database in Impala shell prompt

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9927 )

Change subject: IMPALA-6805: Show current database in Impala shell prompt
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9927/2/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9927/2/shell/impala_shell.py@777
PS2, Line 777: [%s:%s] %s
I would prefer to change this to use the format() function. Something like 
"[{host}:{port}] {db}> ".format(host=self.impalad[0], port=self.impalad[1], db 
=...


http://gerrit.cloudera.org:8080/#/c/9927/2/shell/impala_shell.py@1147
PS2, Line 1147: [%s:%s] %s>
Since this is used several times, this template should be a constant declared 
somewhere around line 137.


http://gerrit.cloudera.org:8080/#/c/9927/2/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/9927/2/tests/shell/test_shell_interactive.py@62
PS2, Line 62:  %s>" % db
it's better to use format() here too.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0ae58507321e426e5f0f16518671420974a3fc
Gerrit-Change-Number: 9927
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 04 Apr 2018 22:03:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6791: distcc server setup script

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9901 )

Change subject: IMPALA-6791: distcc server setup script
..


Patch Set 7: Code-Review+2

lgtm


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a145911f095eb8e173694475cc2dac65eb7c7bb
Gerrit-Change-Number: 9901
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 03 Apr 2018 23:31:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6791: distcc server setup script

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9901 )

Change subject: IMPALA-6791: distcc server setup script
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9901/6/bin/distcc/distcc_server_setup.sh
File bin/distcc/distcc_server_setup.sh:

http://gerrit.cloudera.org:8080/#/c/9901/6/bin/distcc/distcc_server_setup.sh@62
PS6, Line 62:  a
nit: 6 and 7


http://gerrit.cloudera.org:8080/#/c/9901/6/bin/distcc/distcc_server_setup.sh@81
PS6, Line 81: # Configure ccache for distccd user
Do you think it's a good idea to turn these comments into echos? I saw that 
Phil Z has been doing this in some of our bash scripts.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a145911f095eb8e173694475cc2dac65eb7c7bb
Gerrit-Change-Number: 9901
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 03 Apr 2018 23:19:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6791: distcc server setup script

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9901 )

Change subject: IMPALA-6791: distcc server setup script
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9901/4/bin/distcc/distcc_server_setup.sh
File bin/distcc/distcc_server_setup.sh:

http://gerrit.cloudera.org:8080/#/c/9901/4/bin/distcc/distcc_server_setup.sh@20
PS4, Line 20: Ubuntu
Also CentOS


http://gerrit.cloudera.org:8080/#/c/9901/4/bin/distcc/distcc_server_setup.sh@60
PS4, Line 60:   DISTCCD_USER=nobody
Do we need to check the version of the OS here, like we do for Ubuntu?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a145911f095eb8e173694475cc2dac65eb7c7bb
Gerrit-Change-Number: 9901
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 03 Apr 2018 22:12:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 13: Code-Review+1

(2 comments)

I'm happy with this change. Alex, can you take a look please?

http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc@5331
PS13, Line 5331:   TestValue("width_bucket(1, 
19.77, 999.9"
We should test the two conditions that can lead to requiring int256_t 
separately, which is what we are trying to do here. Are we sure that one 
condition is true and the other is false in these two tests?


http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc@5333
PS13, Line 5333:
Maybe add a few more cases where int256_t is used? I would add these tests:
- smallest values where int256_t is needed.
- largest values where int256_t is not needed
- largest values where int256_t is needed and there is no overflow
- smallest values where int256_t is needed and there should be an overflow



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 13
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Mon, 02 Apr 2018 23:41:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6070: Expose using Docker to run tests faster.

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9085 )

Change subject: IMPALA-6070: Expose using Docker to run tests faster.
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9085/4/bin/bootstrap_system.sh
File bin/bootstrap_system.sh:

http://gerrit.cloudera.org:8080/#/c/9085/4/bin/bootstrap_system.sh@131
PS4, Line 131:   JDK_VERSION=7
Is there a reason why we use JDK 7 on Ubuntu 14.04? This means that the current 
apache/master branch does not work after running this script because it 
requires JDK 8 (because of Hadoop 3).

Maybe add a comment why we can't use JDK8 on 14.04?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82052ef31979564968effef13a3c6af0d5c62767
Gerrit-Change-Number: 9085
Gerrit-PatchSet: 4
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 30 Mar 2018 00:00:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/9346 )

Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and 
related fns
..

IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.

After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.

We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.

Testinging:
- Updated tests
- Ran an exhaustive build which passed.

Cherry-picks: not for 2.x

Change-Id: I77541678012edab70b182378b11ca8753be53f97
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 110 insertions(+), 68 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9346 )

Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and 
related fns
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9346/2/be/src/exprs/math-functions-ir.cc@152
PS2, Line 152:   // TODO: should we support non-constant seed?
> I think this will DCHECK if a negative scale is passed
oops, fixed.


http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266
PS1, Line 266:   [['sign'], 'DOUBLE', ['DOUBLE'], 
'impala::MathFunctions::Sign'],
> I think this is a question for Greg. It's mostly about whether this functio
Agreed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 23 Mar 2018 22:37:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](2.x) IMPALA-6622: Backport parts of IMPALA-4924 to 2.x

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9768 )

Change subject: IMPALA-6622: Backport parts of IMPALA-4924 to 2.x
..


Patch Set 2: Code-Review+2

(1 comment)

Forwarding the +2

http://gerrit.cloudera.org:8080/#/c/9768/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9768/1//COMMIT_MSG@9
PS1, Line 9: We enabled Decimal V2 by default on master (but not on the 2.x 
branch)
> nit: typo
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b
Gerrit-Change-Number: 9768
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 23 Mar 2018 00:31:36 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](2.x) IMPALA-6622: Backport parts of IMPALA-4924 to 2.x

[Taras Bobrovytsky (Code Review)]

Hello Lars Volker, Tim Armstrong, Alex Behm, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6622: Backport parts of IMPALA-4924 to 2.x
..

IMPALA-6622: Backport parts of IMPALA-4924 to 2.x

We enabled Decimal V2 by default on master (but not on the 2.x branch)
in IMPALA-4924. There were some other code changes that are not
specific to enableing Decimal V2 that are causing merge conflicts. In
this patch, we backport those changes to reduce the chance of
conflicts.

Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b
---
M be/src/exprs/expr-test.cc
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M tests/hs2/test_hs2.py
M tests/query_test/test_aggregation.py
M tests/query_test/test_decimal_casting.py
6 files changed, 127 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/9768/2
--
To view, visit http://gerrit.cloudera.org:8080/9768
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b
Gerrit-Change-Number: 9768
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 11:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@455
PS10, Line 455: a
> Done
This wasn't fixed.


http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@505
PS10, Line 505: f
> If expr < min_range, we return 0
Suppose min_range = -... and max_range=..., so the subtraction 
overflows. This function would return only 3 possible values then. If 
value=min_range-1, then we return 0. If value = max_range+1, then we return 
num_buckets. Otherwise, we return null. This seems like odd behavior to me.

I think we should always return null if there is an overflow when computing 
range_size. What do you think? (btw, if we're going to do this, this needs to 
be moved up to the top of the function)


http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@502
PS11, Line 502: Decimal16Value range_size;
  :   range_size =
nit: why not combine the two lines?


http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@524
PS11, Line 524:   if (int total_leading_zeros = 
BitUtil::CountLeadingZeros(abs(buckets.value())) +
Why do we need to declare total_leading_zeros here? Also, make this UNLIKELY


http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@529
PS11, Line 529: c
nit: capital letter


http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@531
PS11, Line 531:   if (!needs_int256 && 
BitUtil::CountLeadingZeros(range_size.value()) +
You should use MinLeadingZerosAfterScaling() here. Also, make this UNLIKELY


http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h
File be/src/exprs/math-functions.h:

http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h@115
PS10, Line 115:   static BigIntVal WidthBucket(FunctionContext* ctx, const 
DecimalVal& expr,
> I made this private. Not sure, any pointers where I can move this function
I think the place you moved it to is ok.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 11
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 23 Mar 2018 00:27:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](2.x) IMPALA-6622: Backport parts of IMPALA-4924 to 2.x

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9768


Change subject: IMPALA-6622: Backport parts of IMPALA-4924 to 2.x
..

IMPALA-6622: Backport parts of IMPALA-4924 to 2.x

We enabled Deciaml V2 by default on master (but not on the 2.x branch)
in IMPALA-4924. There were some other code changes that are not
specific to enableing Decimal V2 that are causing merge conflicts. In
this patch, we backport those changes to reduce the change of
conflicts.

Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b
---
M be/src/exprs/expr-test.cc
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M tests/hs2/test_hs2.py
M tests/query_test/test_aggregation.py
M tests/query_test/test_decimal_casting.py
6 files changed, 127 insertions(+), 36 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/9768/1
--
To view, visit http://gerrit.cloudera.org:8080/9768
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2504fb6ec7fa350296b058156b6fd7fb97bc4f9b
Gerrit-Change-Number: 9768
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG@11
PS1, Line 11: nd support
Nit: "and enables support of all hash algorithms"



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 15 Mar 2018 23:50:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9346 )

Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and 
related fns
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120:   return DoubleVal(trunc(
> pow(10.0, scale.val) is likely expensive to compute and it isn't clear that
Done. According to my benchmark, this implementation is much faster (over 10x) 
than the old one (for cases where scale is less than 20).


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120:   return DoubleVal(trunc(
> Do we have a test that shows the better behavior for trunc() versus the pre
Yes, Alex, there is a test in expr-test.cc on line 5465


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121:   v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) 
/ pow(10.0, scale.val));
> Can you undo the code movement here so that the change is more clear?
Done


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121:   v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) 
/ pow(10.0, scale.val));
> Should we also check for overflows here , set FunctionContext with an error
Anuj, the problem here is with double arithmetic. In general, it's not possible 
to represent a double number precisely. Which is why it is weird to do rounding 
and specify the number of digits to round to in decimal. I don't think that 
anything can be done about this.

This is exactly the reason why people should be using the decimal type.


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h
File be/src/exprs/math-functions.h:

http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h@80
PS1, Line 80:   static DoubleVal RoundUpTo(FunctionContext*, const DoubleVal&, 
const BigIntVal&);
> Why the ordering change?  Also, why allow BigIntVal range here, isn't that
Fixed the ordering.

It makes sense to use bigintval because if someone passes in a smallint, it can 
be implicitly cast to bigint, but not the other way around. So bigint is kind 
of like an integer wildcard. Which overflow are you talking about?


http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266
PS1, Line 266:   [['sign'], 'DOUBLE', ['DOUBLE'], 
'impala::MathFunctions::Sign'],
> Should we also have a DECIMAL version?
Do you think we would need to have a sign() function for double, float, int, 
bigint, decimal, etc then?


http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@280
PS1, Line 280:   [['ceil', 'ceiling', 'dceil'], 'DOUBLE', ['DOUBLE'], 
'impala::MathFunctions::Ceil'],
> Don't we also need FLOAT versions of these if our goal is to return the sam
I agree, this should be addressed in a separate patch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 15 Mar 2018 21:51:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9346 )

Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and 
related fns
..

IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.

After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.

We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.

Testinging:
- Updated tests
- Ran an exhaustive build which passed.

Cherry-picks: not for 2.x

Change-Id: I77541678012edab70b182378b11ca8753be53f97
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 107 insertions(+), 68 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/9346/2
--
To view, visit http://gerrit.cloudera.org:8080/9346
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR](2.x) IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9530 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..


Patch Set 1:

I started a dry run build: 
https://jenkins.impala.io/job/gerrit-verify-dryrun/2071/

I will submit the patch once the build passes.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9530
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 08 Mar 2018 22:34:19 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9530 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..


Patch Set 1: Code-Review+2

Cherry-picked and resolved conflicts


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9530
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 08 Mar 2018 22:05:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@588
PS9, Line 588: for index in xrange(0, len(parsed_statements)):
> Isn't xrange is faster in Python 2.7 compared to enumerate?
It probably is faster, but I don't think performance should be a concern in 
this case.

If you want to keep xrange(), then you can get rid of the first argument. 
xrange(len(parsed_statements))



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 07 Mar 2018 00:24:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@601
PS9, Line 601: if not found_error:
> Ok, but what will happen if the list looks like the following:
I tried it, it looks like it throws away the middle "not error" statement. I 
think that's weird behavior.

I tried it with this query in impala-shell:
select 1; \; select 2; \; select 3;

"Select 2" vanishes



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 07 Mar 2018 00:16:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@588
PS9, Line 588: for index in xrange(0, len(parsed_statements)):
> for index, statement in enumate(parsed_statements)
enumerate*



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 06 Mar 2018 23:45:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@588
PS9, Line 588: for index in xrange(0, len(parsed_statements)):
for index, statement in enumate(parsed_statements)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 06 Mar 2018 23:44:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@601
PS9, Line 601: if not found_error:
> When found_error is true at the very end (implied by having non empty joine
Ok, but what will happen if the list looks like the following:

not error,
error,
not error,
error,
not error

Also, maybe add this to the test cases?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 06 Mar 2018 23:42:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6337: Fix infinite loop in Impala shell

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9195 )

Change subject: IMPALA-6337: Fix infinite loop in Impala shell
..


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@590
PS9, Line 590: filter
It's not very Pythonic to use the filter function. The following is much more 
Pythonic:
error_count = len([tok for tok in statement.tokens if tok.ttype == 
tokens.Error])


http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@601
PS9, Line 601: if not found_error:
What if found error is true here? Then something must have gone wrong, right? 
Maybe return split_statements in that case?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b
Gerrit-Change-Number: 9195
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 06 Mar 2018 22:12:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..

IMPALA-6405: Error when string to decimal cast overflows

Before this patch, when there was an error when converting a string to
a decimal, a NULL was returned. In this patch, we change this behavior
so that an error is returned if decimal_v2 is enabled. We also add a
warning if there is an underflow.

The reasoning is that we want stricter behavior in decimal_v2.

Testing:
- Added some EE tests.
- Ran an exhaustive build, which passed.

Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 57 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 5
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> Right, but given we've made an explicit choice to round, it seems to me tha
Postgres does not issue any warnings. I think then it would make sense to not 
issue the warning for both decimal v1 and v2. Updated the code.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:31:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9339/2/be/src/exprs/decimal-operators-ir.cc@584
PS2, Line 584: ctx->AddWarning("String to Decimal cast underflowed");
> Doesn't that mean that the comment for IMPALA-5014: Part 1 isn't doing what
IMPALA-5014: Part 1 changes the behavior so that when then there is an 
underflow, we round instead of truncating when decimal_v2 is enabled. So that 
patch is correct. Before this patch, underflows happened silently. After this 
patch we will issue a warning.

"Underflow" means that not all digits to the right of the dot in the string 
could fit into the resulting decimal. "Overflow" means that not all digits to 
the left of the dot could fit into the resulting decimal.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:08:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5752: Add support for DECIMAL on Kudu tables

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9368 )

Change subject: IMPALA-5752: Add support for DECIMAL on Kudu tables
..


Patch Set 4: Code-Review+1

The patch looks good to me.

One thing we need to do for TPCDS and possibly TPCH is change some of the 
columns from DOUBLE to DECIMAL for Kudu. The expected results need to be 
updated as well. The expected results should be the same as for other storage 
types.

It probably makes sense to do that in a separate patch.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a9fe5acadc53ec198585d765a8cfb0abe56e199
Gerrit-Change-Number: 9368
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Wed, 21 Feb 2018 20:05:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..

IMPALA-6405: Error when string to decimal cast overflows

Before this patch, when there was an error when converting a string to
a decimal, a NULL was returned. In this patch, we change this behavior
so that an return in error is returned if decimal_v2 is enabled. We also
add a warning if there is an underflow.

The reasoning is that we want stricter behavior in decimal_v2.

Testing:
- Added some EE tests.
- Ran an exhaustive build, which passed.

Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M tests/query_test/test_decimal_casting.py
5 files changed, 81 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/9339/2
--
To view, visit http://gerrit.cloudera.org:8080/9339
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-6405: Error when string to decimal cast overflows

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9339 )

Change subject: IMPALA-6405: Error when string to decimal cast overflows
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9339/1//COMMIT_MSG@10
PS1, Line 10: a decimal, a MULL was returned. In this patch, we change this 
behavior
> NULL
oops, done.


http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
File fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java:

http://gerrit.cloudera.org:8080/#/c/9339/1/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@85
PS1, Line 85: if (resultOfReverseCast != null &&
> It seems weird (at least to me) that LiteralExpr.create would return null i
No, other casts do not raise errors.

LiteralExpr.create() returns a null when there is an error or a warning 
according to the comment in the Java file. To me this does not seem weird.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icffccac1c1c2361447ae4b0de9b6c2ec7de071db
Gerrit-Change-Number: 9339
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:05:09 +
Gerrit-HasComments: Yes