Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8398 )
Change subject: IMPALA-3436: Return a decimal when rounding a double ...................................................................... Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/8398/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8398/1//COMMIT_MSG@16 PS1, Line 16: This is implemented by introducing a "hack" where we make the parser How about we say rewrite or translation instead of hack? I don't think it's really a terrible hack - it's a minimal change. http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc File be/src/exprs/decimal-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc@111 PS1, Line 111: DCHECK(!scale.is_null); Add a FE test case in AnalyzeExprsTest.java that makes sure this is enforced. The existing round() functions that take a DECIMAL argument must have a non-NULL, constant second argument. New tests should be added to make sure that is also enforced for your new round() function. There's also a subtle change in behavior we should consider. The existing round() function accepts non-constant arguments, should the new round function() also allow that or not? http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc@116 PS1, Line 116: bool ovf = false; overflow http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions.h File be/src/exprs/decimal-functions.h: http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions.h@58 PS1, Line 58: FunctionContext*, const DoubleVal&, const IntVal&); add arg names for consistency http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@284 PS1, Line 284: [['round_v1','dround_v1'], Why keep the dround_v1 alias? http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@286 PS1, Line 286: [['round','dround'], 'DECIMAL', ['DOUBLE', 'INT'], 'impala::DecimalFunctions::RoundTo'], Can you organize these and comment on whether these are used in v1/v2 or both? Seems confusing now. http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@287 PS1, Line 287: [['round','dround','round_v1','dround_v1'], Why keep the dround_v1 alias? http://gerrit.cloudera.org:8080/#/c/8398/1/common/function-registry/impala_functions.py@403 PS1, Line 403: [['round','dround','round_v1','dround_v1'], Why keep the dround_v1 alias in these? http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/cup/sql-parser.cup@70 PS1, Line 70: private TQueryOptions queryOptions; add TODO to remove when decimal v1 is gone, maybe come up with a specific thing you can grep for later in the code, e.g. DECIMAL_V1 and use that consistently in the TODOs for removal http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@100 PS1, Line 100: FunctionCallExpr functionCallExpr = new FunctionCallExpr(fnName, params); newline http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@111 PS1, Line 111: // nullif(x, y) -> if(x DISTINCT FROM y, x, NULL) newline http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@385 PS1, Line 385: * This can only be called for functions that return wildcard decimals and the first Is this comment still accurate? http://gerrit.cloudera.org:8080/#/c/8398/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@407 PS1, Line 407: // The situtation where none of the parameters are decimal, but the return type is * Shrink comment to: None of the parameters are decimal but the return type is decimal. * It's not clear to me why we'd use (38,0) in this case, can you explain? For example, round(double, int) does not have decimal args. Shouldn't the return type be (38,X) where X is the value of the second arg if it is a constant? -- To view, visit http://gerrit.cloudera.org:8080/8398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6 Gerrit-Change-Number: 8398 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Comment-Date: Fri, 27 Oct 2017 21:53:47 +0000 Gerrit-HasComments: Yes
