impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kim Jin Chul (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()
Date Tue, 18 Jul 2017 16:39:06 GMT
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 <jinchul@gmail.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Greg Rahn <grahn@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <jinchul@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message