Taras Bobrovytsky 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 Done 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 enforce Added a test. I think we should remain consistent with other decimal round functions. They do not allow a non-constant round arguments, so I think it makes sense to not allow it here as well. http://gerrit.cloudera.org:8080/#/c/8398/1/be/src/exprs/decimal-functions-ir.cc@116 PS1, Line 116: bool ovf = false; > overflow Done 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 Done 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? Because what if someone typed in "select dround(x)". We need to be able to differentiate between round and dround in error messages. Are you suggesting to get rid of the "dround" alias completely? 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 bo Done 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? Same reason as above. 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? Answered above. 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 t Done 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 Done 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 Done 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? Fixed the comment. 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: Done -- 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-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Comment-Date: Tue, 07 Nov 2017 00:35:00 +0000 Gerrit-HasComments: Yes
