[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
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 testdata/workloads/functional-query/queri
[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
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] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
Alex Behm 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)
Nice work, should be good to go soon!
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.
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
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.
The strict decimal is specifically for decimal->decimal casts. Please correct
here and elsewhere.
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'
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
what 'strictDecimal' is and how it's different/independent from strict.
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.
--
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 16:53:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE
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
