Alex Behm has posted comments on this change. Change subject: IMPALA-3437: DECIMAL_V2: avoid implicit decimal->double conversion ......................................................................
Patch Set 5: (11 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 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 rationale for their casting behavior. In particular, does decimal still have a higher processing cost? This way it becomes clear that the old behavior was intended to maximize performance and the new behavior is intended to provide accurate/consistent behavior (but does it take a perf hit for that?) Line 529: protected void convertNumericLiteralsFromDecimal(Analyzer analyzer) I can't wait for this function to be deleted. 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 DECIMAL_V2 setting. Line 1319: * Test expressions that resolve to the same type with either DECIMAL_V2 value. resolve to -> return (in the FE 'resolve' has a specific meaning for SlotRefs and TableRefs which we should not confuse here) Line 1331: Type.DOUBLE, ScalarType.createDecimalType(5, 1)); Why is the result not DECIMAL(2,1)? Decimal type seems weird, but not the focus of this patch. Line 1342: // DECIMAL_V2: floating point + decimal expr = decimal This does not seem to explain what happens for "floating point + numeric literal" like in the DECIMAL_V1 case. What happens in cases where the numeric literal is a float because it does not fit into our max decimal? Needs test. Might be clearer if you list: DECIMAL_V2: float + decimal literal = decimal DECIMAL_V2: float + float literal = float Line 1386: // Multiplying a floating point type with any other type always results in a double. Why? Seems inconsistent. Line 1409: checkDecimalReturnType("select round(1.2345, 2) * pow(10, 10)", Type.DOUBLE); add same test with "+" Line 1426: // Test behavior of compound expressions with a single column and many literals. column -> slot ref (and elsewhere) Line 1465: checkDecimalReturnType("select 1.123e-2", ScalarType.createDecimalType(5, 5)); What about literals that don't fit into a decimal? -- 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
