Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11359 )
Change subject: IMPALA-7203. Support UDFs in CatalogdMetaProvider ...................................................................... Patch Set 3: (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 Done http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2058 PS1, Line 2058: : } finally { : v > single line. Done 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 functio added to Function.fromThrift http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@955 PS1, Line 955: invalidateCacheForTable(obj.table.db_name, obj.table.tbl_name, invalidated); : > My opinion is that this should be moved into invalidateCacheForFunction() s hm, prefer to keep it consistent with the above one for TABLE/VIEW if that's alright. http://gerrit.cloudera.org:8080/#/c/11359/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@985 PS1, Line 985: > nit: make it varargs and avoid multiple calls above? Done 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? Done 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? Done 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? Done 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 Done 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/rea the contract for LocalCatalog is that it's all single-threaded. Where do we have multi-threaded access to LocalCatalog? -- 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: 3 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 04 Sep 2018 20:00:56 +0000 Gerrit-HasComments: Yes
