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 <jinc...@gmail.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
