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

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

2018-04-24 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins 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:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2357/


--
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: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 24 Apr 2018 22:34:28 +
Gerrit-HasComments: No

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

2018-04-24 Thread 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

2018-04-24 Thread 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

2018-04-24 Thread 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

2018-04-23 Thread Alex Behm (Code Review)

Alex Behm 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 5: Code-Review+2

Changes look fine to me.

In the future, we should perhaps reconsider adding float/double suffixes to 
clean up all these casts.


--
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: 5
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 24 Apr 2018 04:09:24 +
Gerrit-HasComments: No

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

2018-04-23 Thread 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

2018-04-23 Thread 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

2018-04-20 Thread Alex Behm (Code Review)

Alex Behm 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:

(5 comments)

Should probable run this on exhaustive too to make sure we're not missing any 
broken place.

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 rely on 
creating a decimal type larger than we can support. Are you sure this passes 
all tests?


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@1840
PS2, Line 1840: AnalyzesOk("select concat('a', 'b', 'c', 'd')");
> How do we avoid printing the function name? It gets printed when we call fn
Right now you are doing FunctionName.toString(). Instead you can do: 
FunctionName.getFunction().toString()


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


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 "functional" 
so having it is confusing.


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 and 
not the full package name also. Since we use this for checking expected error 
messages, we know what specific exception we are looking for (do no need for 
package info).



--
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: Fri, 20 Apr 2018 16:22:06 +
Gerrit-HasComments: Yes

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

2018-04-19 Thread 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] IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE

2018-04-19 Thread 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

2018-04-19 Thread Alex Behm (Code Review)

Alex Behm 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:

(22 comments)

Looks pretty good to me

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


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?


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@440
PS1, Line 440: for (int i = 0; i < children_.size(); ++i) {
> Not a good idea because if ignoreWildcardDecimals is true we don't want to 
It's confusing to me because this check is inside the loop, but 
resolvedWildcardType and ignoreWildcardDecimals are not changed in the loop, so 
it seems more logical to move this check outside.

Would it not work to check:
if (resolvedWildcardType != null && resolvedWildcardType.isInvalid() && 
!ignoreWildcardDecimals) {
  // error
}


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.


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()


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 
because we require no clipping to happen. You can just return the appropriate 
type directly.


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


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


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


http://gerrit.cloudera.org:8080/#/c/9930/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/9930/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2570
PS1, Line 2570: testDecimalExpr(String.format("%s(1.23)", alias),
> Should we be generating the exception in the function call expr then? That
Makes sense, printing the toSql() will be more tricky. Let's defer.


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:

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

2018-04-18 Thread 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

2018-04-18 Thread 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-6518,IMPALA-6340: Check that decimal types are compatible in FE

2018-04-11 Thread Alex Behm (Code Review)

Alex Behm 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:

(23 comments)

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)


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?


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


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?


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 caller 
has more context to produce a better error message (like you've already shown 
in this change), so probably better to move this check to the caller.

You can remove the throws declaration from this function then.


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' should 
possibly be combined. According to the function comment 'strict' should not 
allow loss of precision, so decimalV2 should be true?


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 
inside here, since we are not actually clipping.

Alternatively, we should comment what the decimalV2 argument does here (I still 
prefer 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


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


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?


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@89
PS1, Line 89:   "Unable to create a hash join on the following two 
decimal " +
Also print the join predicate in the error message, otherwise it might be very 
hard for users to find the right place to CAST.

Suggestion:

Unable to create a hash join with equi-join predicate  because 
the operands cannot be cast without loss of precision. Operand types:  = 


http://gerrit.cloudera.org:8080/#/c/9930/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/9930/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2570
PS1, Line 2570: "Unable to resolve the decimal types of the 
_impala_builtins.coalesce " +
Can we also print the FunctionCallExpr.toSql() at this point? It might be hard 
for users to figure out where it's going wrong in a huge query.


http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2573
PS1, Line 2573: testDecimalExpr("coalesce(cast(0.789 as decimal(19, 19)), " 
+
duplicate test?

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

2018-04-11 Thread Alex Behm (Code Review)

Alex Behm 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:

(4 comments)

High-level questions/comments. I'm still going through the details.

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 is 
that we don't support RANGE offset boundaries today, so this code change is not 
incompatible.

See AnalyzeExprsTest.TestAnalyticExprs()


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 
directly:

boolean isDecimalV2() { return getQueryOptions().isDecimal_v2(); }


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?


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

(camel-case in Java land)



--
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: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Comment-Date: Thu, 12 Apr 2018 00:19:22 +
Gerrit-HasComments: Yes

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

2018-04-10 Thread Alex Behm (Code Review)

Alex Behm 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:

These choices are mostly modeled after Netezza which is conservative. We felt 
that the conservative behavior makes most sense for the cases described in the 
commit msg. Netezza/Impala will not implicitly truncate/round there.


--
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: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Comment-Date: Tue, 10 Apr 2018 23:19:55 +
Gerrit-HasComments: No

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

2018-04-06 Thread Dan Hecht (Code Review)

Dan Hecht 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:

Are these choices in behavior modeled after another system?


--
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: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Comment-Date: Fri, 06 Apr 2018 16:58:50 +
Gerrit-HasComments: No

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

2018-04-04 Thread 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-6518,IMPALA-6340: Check that decimal types are compatible in FE

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

18 matches

Site Navigation

Mail list logo

Footer information