Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12001 )
Change subject: IMPALA-7902: NumericLiteral fixes, refactoring ...................................................................... Patch Set 10: Code-Review+1 (3 comments) The code flow seems fine to me, specially around NumericLiteral's life-cycle of implicit and explicit types. I'm not familiar with the intricacies of type overflows and how they are handled. So I'm not confident enough to +2 this. Since Tim already took a look, I think you can upgrade to a +2 once the nits are fixed. http://gerrit.cloudera.org:8080/#/c/12001/10/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java File fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java: http://gerrit.cloudera.org:8080/#/c/12001/10/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java@234 PS10, Line 234: catch (SqlCastException e) { : return null; : } I see that you are trying to preserve the earlier behavior here. But doesn't it make sense to propagate the SqlCastException for overflow? Just curious (I'm not sure why we returned null earlier) http://gerrit.cloudera.org:8080/#/c/12001/10/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java File fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java: http://gerrit.cloudera.org:8080/#/c/12001/10/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@52 PS10, Line 52: > nit: ? http://gerrit.cloudera.org:8080/#/c/12001/10/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@113 PS10, Line 113: /** nit: use // style comments for vars -- To view, visit http://gerrit.cloudera.org:8080/12001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I484600747b2871d3a6fe9153751973af9a8534f2 Gerrit-Change-Number: 12001 Gerrit-PatchSet: 10 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 13 Dec 2018 00:17:37 +0000 Gerrit-HasComments: Yes
