Kim Jin Chul has posted comments on this change. Change subject: IMPALA-5529: Add additional function signatures for TRUNC() ......................................................................
Patch Set 1: (4 comments) Hi, Would you please review my code with comments? Thanks. I would like to add some unit tests into java/org/apache/impala/analysis/AnalyzeExprsTest.java. Do you think this is a right place? Please let me know the location if you have E2E test set. Best regards, Jinchul http://gerrit.cloudera.org:8080/#/c/7450/1/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: PS1, Line 278: [['trunc'], 'BIGINT', ['BIGINT'], ''], : [['trunc'], 'INT', ['INT'], ''], : [['trunc'], 'SMALLINT', ['SMALLINT'], ''], : [['trunc'], 'TINYINT', ['TINYINT'], ''], 1) I believe there is nothing to do. It just returns an input value. I think we don't have to call backend function such as Truncate/Round, so I would like to do nothing here as coalesce's implementation. By the way, my implementation seems to be insufficient because backend throws "Function trunc is not implemented". Would you please let me know if there is any special handling? 2) What do you think about the specified types such as a series of INT type? I guess we need to support more premitive types to work the function properly. PS1, Line 282: 'BIGINT', ['DOUBLE'] In the description of the JIRA, the reporter wrote that double precision of the argument should be double precision, but I am wondering it is expected behavior because of "DECIMAL(*, *) = TRUNC(DECIMAL(*,*) [, *INT])" which means return type should be same of the first argument type. If the return type should be DOUBLE, I guess we need to add implicit conversion. PS1, Line 284: 'DOUBLE', ['DOUBLE', 'INT'] 1) I think "DOUBLE = TRUNC(DOUBLE, *INT)" is required. Do you agree on this? 2) The author before my patch intended implicit type conversion because there aren't SMALLINT, TINYINT and so on. I think implicit conversion has the disadvantages: * unnecessary type conversion cost * (maybe) forget the original type http://gerrit.cloudera.org:8080/#/c/7450/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 392: fnName_.getFunction().equalsIgnoreCase("trunc") || This is required to determine result type. -- To view, visit http://gerrit.cloudera.org:8080/7450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I856da9f817b948de3c72af60a0742b128398b4cf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-HasComments: Yes
