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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
