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 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 Done 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 Done 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. Done 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 Done 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 NotImplem Done 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 Done 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 c 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: 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: Fri, 20 Apr 2018 01:18:06 +0000 Gerrit-HasComments: Yes
