Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/12481 )
Change subject: IMPALA-7368: Add initial support for DATE type ...................................................................... Patch Set 15: (5 comments) http://gerrit.cloudera.org:8080/#/c/12481/14/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java File fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java: http://gerrit.cloudera.org:8080/#/c/12481/14/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java@52 PS14, Line 52: public PartitionSpec(List<PartitionKeyValue> partitionSpec) { > Do we ever expect this to get called prior to analysis? Done http://gerrit.cloudera.org:8080/#/c/12481/14/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java@156 PS14, Line 156: } > same (Precondition on analysis if possible) Done http://gerrit.cloudera.org:8080/#/c/12481/11/fe/src/main/java/org/apache/impala/catalog/Function.java File fe/src/main/java/org/apache/impala/catalog/Function.java: http://gerrit.cloudera.org:8080/#/c/12481/11/fe/src/main/java/org/apache/impala/catalog/Function.java@191 PS11, Line 191: // Compares this to 'other' for 'mode'. > hm, I spent some time researching this across multiple databases: I agree that the function overload resolution algorithm needs to be redesigned but I don't think we should do it now as it would break backward compatibility. We can reconsider implicit casting rules and re-implement the resolution logic (preferably modelled after what Hive does) in a separate change when we are ready for a breaking change. What do you think? http://gerrit.cloudera.org:8080/#/c/12481/11/fe/src/main/java/org/apache/impala/catalog/Function.java@236 PS11, Line 236: private int calcSuperTypeOfMatchScore(Function other, boolean strict) { > I think we could simplify this code a lot by adding some utility function l Done 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; > it's interesting that our function resolution returns the first fit, from t That's correct, currently we never raise "ambiguous resolution" error. Adding an error to that effect sounds reasonable but I'm worried that it will introduce a breaking change. E.g. year('2011-01-01') resolves to year(TIMESTAMP) right now, but after introducing year(DATE) users will get an error. -- 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: 15 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:42:27 +0000 Gerrit-HasComments: Yes
