Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan ...................................................................... Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h File be/src/runtime/lib-cache.h: http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h@79 PS8, Line 79: /// exp_mtime differs from the mtime on the file system. > If an error is returned because the mtime differs, will the cache and the l I think you might have missed this comment. http://gerrit.cloudera.org:8080/#/c/9697/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/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@391 PS11, Line 391: protected TSymbolLookupParams getLookupParams() { return null; } abstract? http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@393 PS11, Line 393: // Looks up the last time the function's source file was updated as recorded in its What does it return if there was an error? http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@394 PS11, Line 394: // lib-cache entry. Returns -1 if a modified time is not applicable. not necessary obvious that the lib cache is in the BE, so better "in its backend lib-cache entry" http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@399 PS11, Line 399: Preconditions.checkNotNull(lookup); combine with previous line: TSymbolLookupParams lookup = Preconditios.checkNotNull(getLookupParams()); http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@407 PS11, Line 407: // There was an error looking up the lib. A max mtime will result in an error. The last part is really nebulous to me. How will a max mtime result in an error? What kind of error? http://gerrit.cloudera.org:8080/#/c/9697/11/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/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2162 PS11, Line 2162: // HACK: aggregates contain slot-refs in their child expressions. they are Cleaner solution: * call stmt.materializeRequiredSlots(stmt.getAnalyzer()); * check stmt.getAggInfo().getAggregateExprs().get(0) as well as stmt.getAggInfo().getMergeAggInfo().getAggregateExprs().get(0) http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2186 PS11, Line 2186: testMTime("avg(int_col) from functional.alltypes", false); I think you can move the "from functional.alltypes" part into L2159, we have similar patterns elsewhere 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@203 PS8, Line 203: > this proved to be difficult. my approach was to drop/create so as to modify thanks for trying, let me think whether I can come up with a sensible approach -- 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: 11 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 18:42:48 +0000 Gerrit-HasComments: Yes
