Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9930 )
Change subject: WIP: IMPALA-6518,IMPALA-6340: Check that decimal types are compatible in FE ...................................................................... Patch Set 8: (6 comments) Nice work, should be good to go soon! http://gerrit.cloudera.org:8080/#/c/9930/8/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/8/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java@377 PS8, Line 377: Analyzer analyzer, AnalyticWindow.Boundary boundary) throws AnalysisException { No need to pass analyzer. http://gerrit.cloudera.org:8080/#/c/9930/8/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/8/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@217 PS8, Line 217: Preconditions.checkState(!t0.isDecimal() && !t1.isDecimal()); remove http://gerrit.cloudera.org:8080/#/c/9930/8/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/8/fe/src/main/java/org/apache/impala/analysis/Expr.java@429 PS8, Line 429: * If strictDecimal is true, the function will throw an exception if it is not possible This isn't accurate because we will still allow decimal->double conversions. The strict decimal is specifically for decimal->decimal casts. Please correct here and elsewhere. http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/9930/8/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@178 PS8, Line 178: * message. Needs comment for 'strictDecimal' http://gerrit.cloudera.org:8080/#/c/9930/8/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/8/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@433 PS8, Line 433: * is INVALID_TYPE. Especially these very visible and heavily used functions need a comment for what 'strictDecimal' is and how it's different/independent from strict. http://gerrit.cloudera.org:8080/#/c/9930/8/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/8/fe/src/main/java/org/apache/impala/catalog/Type.java@293 PS8, Line 293: * If strictDecimal is true, only consider casts that result in no loss of information This is a nice description! I suggest replicating it in more places. -- 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: 8 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Comment-Date: Thu, 26 Apr 2018 16:53:33 +0000 Gerrit-HasComments: Yes
