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: 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 think it's good to have explicit contexts with v1 and v2 so this patch will easily be portable to 2.x 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 the time the DB should be clear. 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 Also, let's move this to AnalyzeStmtsTest.TestUnion() where we test union compatibility 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) http://gerrit.cloudera.org:8080/#/c/9930/2/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/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3210 PS2, Line 3210: addTestTable("create table d.t2 (c decimal(38,1)) location '/'"); also add tests for double/decimal http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java: http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@97 PS2, Line 97: "t.double_col %s (select cast(d3 as double) from functional.decimal_tbl a)", Better to cast t.double_col to a decimal to perserve what this what test is supposed to cover (see L95) http://gerrit.cloudera.org:8080/#/c/9930/2/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/2/fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java@115 PS2, Line 115: // need 41 digits). Return the best we can do if decimal_v2 is disabled. Return a clipped decimal if decimal_v2 is disabled. (that's hardly "the best we can do" lol) http://gerrit.cloudera.org:8080/#/c/9930/2/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/2/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@343 PS2, Line 343: * The accepted format for error messages is 'not implemented: expected error message' Update comment http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@349 PS2, Line 349: if (!expectedPlan.get(0).toLowerCase().startsWith("not implemented") && It seems clearer to me to list the full Java exception name, like NotImplementedException or InternalException instead of a "custom" string http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@379 PS2, Line 379: private void handleInternalException(String query, String expectedErrorMsg, Code copy+paste can be avoided http://gerrit.cloudera.org:8080/#/c/9930/2/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@534 PS2, Line 534: } catch (NotImplementedException e) { How about we catch Exception e here, and then have a single function that checks the exception message. 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@5 PS1, Line 5: o_orderdate, > Because I noticed these queries incorrectly have round() calls during the d Good catch! -- 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: 2 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 23:05:33 +0000 Gerrit-HasComments: Yes
