Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11359 )
Change subject: IMPALA-7203. Support UDFs in CatalogdMetaProvider ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@479 PS1, Line 479: TFunction minFunc = new TFunction(); : minFunc.setName(obj.fn.getName()); : min.setFn(minFunc) I think this can be min.setFn(new TFunction(obj.fn.getName()) http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2058 PS1, Line 2058: { : thriftFuncs.add(f.toThrift()); : } single line. http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@762 PS1, Line 762: funcs.add(Function.fromThrift(thriftFunc)); Is it worth adding a Preconditions check that checks that extracted function have all the optional fields set? http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@955 PS1, Line 955: invalidateCacheForDb(obj.fn.name.db_name, DbCacheKey.DbInfoType.FUNCTION_NAMES, : invalidated); My opinion is that this should be moved into invalidateCacheForFunction() so that there is a single place that invalidates both. What do you think? http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@985 PS1, Line 985: type, nit: make it varargs and avoid multiple calls above? http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java: http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@179 PS1, Line 179: Populate the 'functions_' map if not already populated. : * This handles loading persistent native functions, since they're already : * present in the DB-level metadata, and loading the list of Java function : * names. The Java function signatures themselves are lazy-loaded as necessary : * by loadFunction(...). Update? http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@188 PS1, Line 188: // Load the Java functions names. We don't load the actual metadata : // for them unless they get looked up. Update? http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@196 PS1, Line 196: from metadata provider Remove? http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@200 PS1, Line 200: for (String fn : funcNames) { : functions_.put(fn, null); : } Single line http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@229 PS1, Line 229: functions_.put(functionName, overloads); Should we make this map thread-safe or something? (now that we can load/read from multiple places in parallel) -- To view, visit http://gerrit.cloudera.org:8080/11359 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifef8ece9f214dca9441833b00f65c7c152d0ab53 Gerrit-Change-Number: 11359 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 30 Aug 2018 23:20:22 +0000 Gerrit-HasComments: Yes
