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 4: (5 comments) Should probable run this on exhaustive too to make sure we're not missing any broken place. http://gerrit.cloudera.org:8080/#/c/9930/4/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/4/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@119 PS4, Line 119: Preconditions.checkState(precision <= MAX_PRECISION); Pretty sure these should break a few tests. I believe in some places we rely on creating a decimal type larger than we can support. Are you sure this passes all tests? 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@1840 PS2, Line 1840: AnalyzesOk("select concat('a', 'b', 'c', 'd')"); > How do we avoid printing the function name? It gets printed when we call fn Right now you are doing FunctionName.toString(). Instead you can do: FunctionName.getFunction().toString() http://gerrit.cloudera.org:8080/#/c/9930/4/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/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2570 PS4, Line 2570: AnalysisContext decimalV1Ctx = createAnalysisCtx(Catalog.DEFAULT_DB); you can remove the Catalog.DEFAULT_DB arg http://gerrit.cloudera.org:8080/#/c/9930/4/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/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2654 PS4, Line 2654: AnalysisContext decimalV1Ctx = createAnalysisCtx("functional"); Use createAnalysisCtx() without an arg. There's no significance to "functional" so having it is confusing. http://gerrit.cloudera.org:8080/#/c/9930/4/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/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@348 PS4, Line 348: if (!expectedPlan.get(0).contains("NotImplementedException") && Do startsWith() instead of contains() and expect only the exception name and not the full package name also. Since we use this for checking expected error messages, we know what specific exception we are looking for (do no need for package info). -- 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: 4 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 16:22:06 +0000 Gerrit-HasComments: Yes
