Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12481 )
Change subject: IMPALA-7368: Add initial support for DATE type ...................................................................... Patch Set 11: (2 comments) http://gerrit.cloudera.org:8080/#/c/12481/14/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/12481/14/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@351 PS14, Line 351: return "AVG requires a numeric, timestamp or date parameter: " + toSql(); Needs update http://gerrit.cloudera.org:8080/#/c/12481/11/fe/src/main/java/org/apache/impala/util/FunctionUtils.java File fe/src/main/java/org/apache/impala/util/FunctionUtils.java: http://gerrit.cloudera.org:8080/#/c/12481/11/fe/src/main/java/org/apache/impala/util/FunctionUtils.java@243 PS11, Line 243: return max_func; > That's correct, currently we never raise "ambiguous resolution" error. I don't think this behaviour was a well thought-out decision. The earliest versions of this that I saw was very hacky and actually returned an arbitrary function when there were multiple valid overloads - whichever was listed first in the catalog. I changed that to be a bit more intelligent about choosing functions that don't result in loss of information and also and checking functions in a deterministic order. It's not the way I would have designed it but it was the least disruptive way to evolve it. Introducing an error where there wasn't one before is pretty risky because it will probably break existing queries. Your example seems highly likely to be used in practice. There are some cases where it might not be a breaking change to raise an error, because they didn't occur before, e.g. if one of the inputs is a DATE. But not sure how interesting those cases are. I tend to think that something like this is probably the least of the evils, but the complexity of the behaviour is not ideal. -- To view, visit http://gerrit.cloudera.org:8080/12481 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea8155ef09557e0afa2f8b2d0b2dc9d0896dc30f Gerrit-Change-Number: 12481 Gerrit-PatchSet: 11 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 27 Mar 2019 17:59:06 +0000 Gerrit-HasComments: Yes
