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. 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 v Done 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 h Should we be generating the exception in the function call expr then? That seems weird. We don't have access to FunctionCallExpr inside the Function class. I updated the error message to include the types. 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? Done 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 co Replaced with a timestamp expression that warns. 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 c Added a case where it is invalid when Decimal v2 is enabled. I don't think we can get INVALID when decimal_v2 is disabled. Condensed the tests. 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? Done 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? Because I noticed these queries incorrectly have round() calls during the development of this patch. This is because in the past these used to be floats. We forgot to remove them. 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? As I mentioned in a different comment, I noticed that these were incorrect during the development of this patch 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 Done 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 Updated the test. This is like the base case. I am choosing the largest input that does not cause an overflow for both decimal v1 and decimal v2. 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 e Done 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 Done 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 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: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Comment-Date: Thu, 19 Apr 2018 01:07:55 +0000 Gerrit-HasComments: Yes
