Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP) ...................................................................... Patch Set 8: (12 comments) http://gerrit.cloudera.org:8080/#/c/9697/8/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/9697/8/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@40 PS8, Line 40: import org.apache.impala.thrift.TSymbolType; > revert imports? Done http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java File fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java: http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@206 PS8, Line 206: protected long GetLastModifiedTimeInternal() { > getLastModifiedTime()? changed it to start with a lower-case. http://gerrit.cloudera.org:8080/#/c/9697/8/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/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@391 PS8, Line 391: // Looks up the last time the function's source file was updated. Returns -1 if a > Comment is not accurate. We're looking up the mtime from the BE's library c Done http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@394 PS8, Line 394: public final long GetLastModifiedTime() { > getCachedLastModifiedTime Done http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@404 PS8, Line 404: protected long GetLastModifiedTimeInternal() { return Long.MAX_VALUE; } > Would be nice to have have the JNI invocation and exception handling in a s yup, simplified http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2171 PS8, Line 2171: testMTime("sleep(1)", false); > sleep() is an oddball, let's try a few more common built-ins including Expr Done http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2180 PS8, Line 2180: catalog_.addFunction(ScalarFunction.createForTesting("default", "udf_builtin_bug", > I don't think we've covered the UDA case added coverage for uda's. I think the way I dig the fn out is pretty kludgy for aggregates so let me know if there's a better way. http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py File tests/custom_cluster/test_coordinators.py: http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@137 PS8, Line 137: os.environ['IMPALA_HOME'], 'testdata/udfs/impala-hive-udfs.jar') > I think we generally indent 4 here (and other cases below) Done http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@145 PS8, Line 145: '/test-warehouse/{0}.db'.format(db_name)]) > Might need get_fs_path() here Done http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@203 PS8, Line 203: > Would be good to have a test that triggers the new error and check that run this proved to be difficult. my approach was to drop/create so as to modify the coord cache, then copy a new jar, then run a query. the reason this does not result in a bug is that add fn modifies the cache entry to refresh so that when the query is compiled, it picks up the new jar, and all works. that said, I'm working on adding tests with multiple coordinators and queries that don't run on all executors since there are cases there to double check. http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@206 PS8, Line 206: self.execute_query_expect_success(client, > add IF EXISTS (the database may not have been created) Done http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@208 PS8, Line 208: client.close() > client may not have been set yet Done -- To view, visit http://gerrit.cloudera.org:8080/9697 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6 Gerrit-Change-Number: 9697 Gerrit-PatchSet: 8 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 21 Mar 2018 05:40:43 +0000 Gerrit-HasComments: Yes
