Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12001 )
Change subject: IMPALA-7902: NumericLiteral fixes, refactoring ...................................................................... Patch Set 7: (10 comments) Did a pass over this. Didn't fully digest the test changes but I'm glad there are that many new tests. http://gerrit.cloudera.org:8080/#/c/12001/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12001/7//COMMIT_MSG@33 PS7, Line 33: IMPALA-7891: Analyzer does not detect numeric overflow in CAST This behaviour of ignoring overflow doesn't change does it? http://gerrit.cloudera.org:8080/#/c/12001/7/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/12001/7/fe/src/main/java/org/apache/impala/analysis/Expr.java@377 PS7, Line 377: public Type getExplicitType() { return type_; } Do we even need this on the Expr base class? It seems like it's only actually used for NumericLiteral. Or is part of your plan to use this in a follow-up change? It would be nice if we could avoid having two different concepts of type exposed (and have the implicit vs explicit distinction contained in NumericLiteral) but maybe I'm missing the reason why it's necessary. http://gerrit.cloudera.org:8080/#/c/12001/7/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/7/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java@66 PS7, Line 66: extra space http://gerrit.cloudera.org:8080/#/c/12001/7/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/7/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@53 PS7, Line 53: * The literal also has an "implicit type" which starts the same as the Are "natural type" and "implicit type" the same thing? http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@131 PS7, Line 131: Double double? I don't think this needs to be nullable, right? http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@283 PS7, Line 283: isFloat I find these function names potentially confusing. Something like isInFloatRange() might be less ambiguous. http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@332 PS7, Line 332: if (value.floatValue() == d) { It looks like you're preserving some existing behaviour, but I'm not sure that the behaviour is worth preserving - it's going to be unpredictable from the point of view of users that this small set of values gets cast as FLOAT instead of DOUBLE. I think in most cases you'd prefer the higher precision of DOUBLE for follow-on calculations anyway. I guess it looks like mostly we promote to DOUBLE anyway as soon as we do an operation on it. E.g. getting different types for similar-looking literals is confusing: [localhost:21000] default> select typeof(0.000000000000000000000000000000000000000000001401298464324817); Query: select typeof(0.000000000000000000000000000000000000000000001401298464324817) Query submitted at: 2018-12-03 11:53:35 (Coordinator: http://tarmstrong-box:25000) Query progress can be monitored at: http://tarmstrong-box:25000/query_plan?query_id=e7476ee9632866bd:d562a34600000000 +-------------------------------+ | typeof(1.401298464324817e-45) | +-------------------------------+ | FLOAT | +-------------------------------+ Fetched 1 row(s) in 0.11s [localhost:21000] default> select typeof(0.000000000000000000000000000000000000000000001401298464324816); Query: select typeof(0.000000000000000000000000000000000000000000001401298464324816) Query submitted at: 2018-12-03 11:53:38 (Coordinator: http://tarmstrong-box:25000) Query progress can be monitored at: http://tarmstrong-box:25000/query_plan?query_id=d6437a5c8dfe404d:ba012a500000000 +-------------------------------+ | typeof(1.401298464324816e-45) | +-------------------------------+ | DOUBLE | +-------------------------------+ Fetched 1 row(s) in 0.11s http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/main/java/org/apache/impala/common/SqlCastException.java File fe/src/main/java/org/apache/impala/common/SqlCastException.java: http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/main/java/org/apache/impala/common/SqlCastException.java@26 PS7, Line 26: planer typo: planner http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@492 PS7, Line 492: nit: Blank line seems unnecessary http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java File fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java: http://gerrit.cloudera.org:8080/#/c/12001/7/fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java@445 PS7, Line 445: public void testSwapSign() { I think it would help to have a very brief comment for some of these tests. Some of them are arguably obvious, but e.g. for this one it would be helpful to mention why integers are specificaly tested. Test swapSign() for integral types, where the type may need to be promoted. -- 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: 7 Gerrit-Owner: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 03 Dec 2018 22:58:49 +0000 Gerrit-HasComments: Yes
