impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5529: Add additional function signatures for TRUNC()
Date Wed, 19 Jul 2017 15:49:52 GMT
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5529: Add additional function signatures for TRUNC()
......................................................................


Patch Set 1:

(1 comment)

> (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

Yes, AnalyzeExprsTest.java is one place you should add some tests. The other place would be
be/src/exprs/expr-test.cc. In both cases, you can follow the examples of the existing tests
for 'truncate' and 'dtrunc', and you should only need to add a few tests since the various
corner cases are already covered by the existing tests.

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.
So per the comments on the JIRA yesterday, we're not going to make any changes to input/output
types here, so these additional signatures aren't needed.

All you need to do is add 'trunc' as an alias for the existing 'truncate'/'dtrunc' functions,
as you've done on lines 278 and 399-406.

To be a little clearer - this functionality already exists, as you can pass something like
a TINYINT to the truncate function that takes a DOUBLE and our analyzer will automatically
take care of the needed casts (though you would have to explicitly cast the output from BIGINT
back down to TINYINT, since there's a potential loss of precision there).

Its true that this adds a little overhead, but its not huge, and we don't really expect people
to call these forms of 'trunc' much anyways since, as you note, they don't actually do anything
and just return their argument unchanged, so its not clear that the added complexity of having
all of these extra forms of 'trunc' is worth the performance gain.


-- 
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-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message