Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/9800 )
Change subject: IMPALA-6724: Incorrect exception handling in create/drop function statements ...................................................................... Patch Set 6: (6 comments) The approach looks fine to me. Just a bunch of nitpicks and I feel we need e-e tests to make sure fn resolution works as expected. http://gerrit.cloudera.org:8080/#/c/9800/5/fe/src/main/java/org/apache/impala/analysis/FunctionName.java File fe/src/main/java/org/apache/impala/analysis/FunctionName.java: http://gerrit.cloudera.org:8080/#/c/9800/5/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@104 PS5, Line 104: * set the database name to _impala_builtins. > Tim brought up a security concern which I think is a valid concern. As an a I see your point. I guess it can be argued both ways, so I'm fine either way. http://gerrit.cloudera.org:8080/#/c/9800/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java File fe/src/main/java/org/apache/impala/analysis/FunctionName.java: http://gerrit.cloudera.org:8080/#/c/9800/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@87 PS6, Line 87: /** : * This method runs the overloaded analyze method with useDefaultDb set to false. : */ Kinda obvious, can be removed I think. http://gerrit.cloudera.org:8080/#/c/9800/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@95 PS6, Line 95: when analyze is called nit: redundant http://gerrit.cloudera.org:8080/#/c/9800/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@95 PS6, Line 95: Some rules regarding how the database name is set nit: I think these are the exhaustive rules. So "some rules" is misleading. Also, maybe just say "Function resolution happens as follows", it implicitly means that we are choosing the parent db. http://gerrit.cloudera.org:8080/#/c/9800/6/fe/src/main/java/org/apache/impala/analysis/FunctionName.java@142 PS6, Line 142: isAnalyzed_ = true; Add a preconditions.checknotnull(db_) after L141? http://gerrit.cloudera.org:8080/#/c/9800/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/9800/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@3123 PS6, Line 3123: // Try to select a function with the same name as builtin. I think we need tests to make sure we are resolving functions correctly. An e-e test similar to this where we override a builtin with UDF and run a select with it and double checking the output. (multiple cases like FQ / non FQ etc.) -- To view, visit http://gerrit.cloudera.org:8080/9800 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic30df56ac276970116715c14454a5a2477b185fa Gerrit-Change-Number: 9800 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 28 Mar 2018 06:40:08 +0000 Gerrit-HasComments: Yes
