Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4795: Allow fetching function obj from catalog using signature ......................................................................
Patch Set 1: (2 comments) Adding to Bharath's comments; please address those too. http://gerrit.cloudera.org:8080/#/c/7479/1/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: PS1, Line 463: TFunction tfn = objectDesc.getFn(); : Function fn = null; : if (tfn.isSetScalar_fn() || tfn.isSetAggregate_fn()) { : Function desc = Function.fromThrift(tfn); : fn = getFunction(desc, Function.CompareMode.IS_INDISTINGUISHABLE); : } else if (tfn.isSetSignature()) { : Db db = getDb(tfn.getName().getDb_name()); : fn = (db == null) ? null : db.getFunction(tfn.getSignature()); : } > nit: How about moving this to a new method, getFunction(TFunction fn, mode) Pulling into a separate fn with a clear interface in the javadoc makes sense. Not sure you need to expose 'mode' though. http://gerrit.cloudera.org:8080/#/c/7479/1/fe/src/main/java/org/apache/impala/catalog/Function.java File fe/src/main/java/org/apache/impala/catalog/Function.java: Line 360: throw new IllegalStateException("Expected function type to be either UDA or UDF."); This seems like a reasonable thing to do later, but I'm not sure if this will break anything else. It looks like there are a few callers to fromThrift(), and at least some of them make me nervous about changing this behavior: e.g. CatalogServiceCatalog.loadFunctionsFromDbParams(). That loads fns from a serialized copy that is stored in a table property. Unfortunately I don't think this is well tested. Not sure what kinds of serialized TFunctions exist in existing HMS instances. For now, just leave a TODO. -- To view, visit http://gerrit.cloudera.org:8080/7479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2cfad0213a79d39b77ad9aff701a93f93be4bf7f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
