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 <predicate sql> because the operands cannot be cast without loss of precision. Operand types: <t1 string> = <t2 string> 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? http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@a312 PS1, Line 312: Don't we get the warning behavior with decimal_v2? Even better, we could come up with a more reliable alternative test. http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java File fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java: http://gerrit.cloudera.org:8080/#/c/9930/1/fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java@172 PS1, Line 172: verifyDecimalType( What about cases where we expect INVALID? Might be good to have such test cases for both v1 and v2. Isn't the v1 and v2 behavior the same except for those edge cases? Might make sense to condense the tests more and avoid the copy+paste. http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-planner/queries/PlannerTest/join-order.test File testdata/workloads/functional-planner/queries/PlannerTest/join-order.test: http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-planner/queries/PlannerTest/join-order.test@2 PS1, Line 2: # Modifications: Added round() calls remove since that was fixed right? http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-planner/queries/PlannerTest/join-order.test@5 PS1, Line 5: sum(l_extendedprice * (1 - l_discount)) as revenue, for my understanding: why these changes? http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test File testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test: http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test@5 PS1, Line 5: sum(l_quantity) as sum_qty, why these changes? http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test File testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test: http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test@134 PS1, Line 134: AnalysisException: Unable to resolve the decimal types of the _impala_builtins.coalesce function arguments. You need to wrap the arguments in a CAST. move to an analyzer test http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test@324 PS1, Line 324: # Test AVG overflow due to DECIMAL_v2 Why not run this in V1 and V2? What's the purpose of this test (I get what the two tests below are doing)? http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test@533 PS1, Line 533: AnalysisException: Incompatible return types 'DECIMAL(38,0)' and 'DECIMAL(38,38)' of exprs 'CAST(123 AS DECIMAL(38,0))' and 'CAST(0.789 AS DECIMAL(38,38))'. ideally we should have these as analysis tests, we can still leave the v1 end-to-end tests if you prefer http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test@553 PS1, Line 553: InternalException: Unable to create a hash join on the following two decimal columns: DECIMAL(20,0) and DECIMAL(19,19). CAST the columns to the same decimal type. Should be a planner test http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-query/queries/QueryTest/insert_decimal.test File testdata/workloads/functional-query/queries/QueryTest/insert_decimal.test: http://gerrit.cloudera.org:8080/#/c/9930/1/testdata/workloads/functional-query/queries/QueryTest/insert_decimal.test@16 PS1, Line 16: AnalysisException: Target table '$DATABASE.t1' is incompatible with source expressions. Should be analyzer tests -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Comment-Date: Thu, 12 Apr 2018 04:52:58 +0000 Gerrit-HasComments: Yes
