Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12001 )
Change subject: IMPALA-7902: NumericLiteral fixes, refactoring ...................................................................... Patch Set 11: (3 comments) Made requested changes. However, the pre-review build failed. Please hold off the next review cycle until I fix the issue. 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' Yes, trying to make changes one step at a time. As it turns out, we don't want to fail the query. Consider the following: FALSE AND CAST(20 AS DECIMAL(1.0)) As we process the tree, we'll visit FALSE, then the CAST, which we'll try to rewrite and fail. Then, we'll evaluate the whole expression. Because of short-circuit evals, the entire expression folds to FALSE, though it contains an invalid subexpression. So, some work would be needed to mark each node, and fail the query only if the final, rewritten, expression tree has at last one failed node. 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: t > nit: ? Actually, removed this paragraph. Sadly, the BE will happily cast 1000 to SMALLINT and end up with a truncated value... The cases that fail occur for other literals, so I'll move this paragraph to another spot in the code in another patch. http://gerrit.cloudera.org:8080/#/c/12001/10/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@113 PS10, Line 113: explicitType_ = type_; > nit: use // style comments for vars Sure. I'm used to eclipse: hover over a variable and Eclipse will display the Javadoc for it, but won't display plain-old comments. -- 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: 11 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 17:11:28 +0000 Gerrit-HasComments: Yes
