impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3437: DECIMAL V2: avoid implicit decimal->double conversion
Date Wed, 13 Sep 2017 17:12:25 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double conversion
......................................................................


Patch Set 5:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 513:    * Converts numeric literal in the expr tree rooted at this expr to return floating
> literals
Done


Line 516:    * Decimal has a higher processing cost than floating point and we should not
pay
> Please add subsections "DECIMAL_V1" and "DECIMAL_V2" to explain the rationa
Done. I didn't expend many characters on the DECIMAL_V2 section but I think it's sufficient
to explain the existence of the two code paths (since the comment will go away when DECIMAL_V1
goes away).


http://gerrit.cloudera.org:8080/#/c/7916/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

Line 1306:    * Test expressions that resolve to different types depend on the DECIMAL_V2
setting.
> Test expressions that return different decimal types depending on the DECIM
Reworded slightly since some don't return decimal types, just involve decimal somehow.


Line 1319:    * Test expressions that resolve to the same type with either DECIMAL_V2 value.
> resolve to -> return
Done


Line 1331:         Type.DOUBLE, ScalarType.createDecimalType(5, 1));
> Why is the result not DECIMAL(2,1)?
1 -> interpreted as tinyint -> decimal(3, 0)
1.1 -> interpreted as decimal(2,1)

Then adding decimal(3, 0) and decimal(2, 1) generally results in decimal(5, 1).

This makes sense to me. It's not perfect but it's consistent.

To get the tighter bound on the decimal type you'd need to interpret literals without decimal
points as decimal literals (or do something even more complicated in the type system).


Line 1342:     // DECIMAL_V2: floating point + decimal expr = decimal
> This does not seem to explain what happens for "floating point + numeric li
I don't think the original comment conveyed my intent well. 
The comment is intended to describe the general rules that apply for DECIMAL_V1 and DECIMAL_V2.
DECIMAL_V1's rules treat all numeric literals the same way in this context, whereas DECIMAL_V2's
rules treat all decimal exprs the same way.

The R.H.S. expressions in these cases are decimal literals, so they are in the intersection
of decimal exprs and numeric literals.


Line 1386:     // Multiplying a floating point type with any other type always results in
a double.
> Why? Seems inconsistent.
That's the current behaviour and it does treat literals consistently with other exprs. I think
the idea is that multiplication is more likely to overflow fixed-point values. The code in
getArithmeticResultType() is:

      // For multiplications involving at least one floating point type we cast decimal to
      // double in order to prevent decimals from overflowing.
      if (op == ArithmeticExpr.Operator.MULTIPLY &&
          (t1.isFloatingPointType() || t2.isFloatingPointType())) {
        return Type.DOUBLE;
      }


Line 1409:     checkDecimalReturnType("select round(1.2345, 2) * pow(10, 10)", Type.DOUBLE);
> add same test with "+"
Done. Also updated the comment because it was a little unclear that it was testing a very
specific pattern instead of general expressions with decimal + double.


Line 1426:     // Test behavior of compound expressions with a single column and many literals.
> column -> slot ref
Done


Line 1465:     checkDecimalReturnType("select 1.123e-2", ScalarType.createDecimalType(5, 5));
> What about literals that don't fit into a decimal?
Added some more tests along those lines.


-- 
To view, visit http://gerrit.cloudera.org:8080/7916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie419a75784eec2294947103e6e1465dfadfc29da
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Greg Rahn <grahn@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message