You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/04/03 02:11:10 UTC
[4/9] impala git commit: IMPALA-6724: Allow creating/dropping
functions with the same name as built-ins
IMPALA-6724: Allow creating/dropping functions with the same name as built-ins
This patch removes restriction on creating a function with the same name
as the built-in function. The reason for lifting the restriction is to
avoid a name clash when introducing new built-in functions. The patch
also fixes some inconsistent behavior when creating or dropping a function
when the name specified is fully-qualified or not.
Refer to the below tables for more information.
Create function:
+---------+-------------+-------------------------+-------------------------------+-------------------------------+
| FQ Name | Built-in DB | Function Name | Existing Behavior | New Behavior |
+---------+-------------+-------------------------+-------------------------------+-------------------------------+
| Yes | Yes | Same as built-in | Same name exception | Cannot modify system database |
| Yes | Yes | Different than built-in | Cannot modify system database | Cannot modify system database |
| Yes | No | Same as built-in | Function created | Function created |
| Yes | No | Different than built-in | Function created | Function created |
| No | Yes | Same as built-in | Same name exception | Cannot modify system database |
| No | Yes | Different than built-in | Cannot modify system database | Cannot modify system database |
| No | No | Same as built-in | Same name exception | Function created |
| No | No | Different than built-in | Function created | Function created |
+---------+-------------+-------------------------+-------------------------------+-------------------------------+
Drop function:
+---------+-------------+-------------------------+-------------------------------+-------------------------------+
| FQ Name | Built-in DB | Function Name | Existing Behavior | New Behavior |
+---------+-------------+-------------------------+-------------------------------+-------------------------------+
| Yes | Yes | Same as built-in | Cannot modify system database | Cannot modify system database |
| Yes | Yes | Different than built-in | Cannot modify system database | Cannot modify system database |
| Yes | No | Same as built-in | Function dropped | Function dropped |
| Yes | No | Different than built-in | Function dropped | Function dropped |
| No | Yes | Same as built-in | Cannot modify system database | Cannot modify system database |
| No | Yes | Different than built-in | Cannot modify system database | Cannot modify system database |
| No | No | Same as built-in | Cannot modify system database | Function dropped |
| No | No | Different than built-in | Function dropped | Function dropped |
+---------+-------------+-------------------------+-------------------------------+-------------------------------+
Select function (no new behavior):
+---------+-------------+-------------------------+--------------------------------------------------------+
| FQ Name | Built-in DB | Function Name | Behavior |
+---------+-------------+-------------------------+--------------------------------------------------------+
| Yes | Yes | Same as built-in | Function in the specified database (built-in) executed |
| Yes | Yes | Different than built-in | Unknown function exception |
| Yes | No | Same as built-in | Function in the specified database executed |
| Yes | No | Different than built-in | Function in the specified database executed |
| No | Yes | Same as built-in | Built-in function executed |
| No | Yes | Different than built-in | Unknown function exception |
| No | No | Same as built-in | Built-in function executed |
| No | No | Different than built-in | Function in the current database executed |
+---------+-------------+-------------------------+--------------------------------------------------------+
Testing:
- Ran front-end tests
- Added end-to-end DDL function tests
Cherry-picks: not for 2.x
Change-Id: Ic30df56ac276970116715c14454a5a2477b185fa
Reviewed-on: http://gerrit.cloudera.org:8080/9800
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/08d386f0
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/08d386f0
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/08d386f0
Branch: refs/heads/master
Commit: 08d386f0fc62e2991bcc046dde8574f25d421580
Parents: c3ab276
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Mon Mar 26 12:17:52 2018 -0500
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Mon Apr 2 21:12:31 2018 +0000
----------------------------------------------------------------------
.../impala/analysis/CreateFunctionStmtBase.java | 8 +--
.../impala/analysis/DropFunctionStmt.java | 2 +-
.../apache/impala/analysis/FunctionName.java | 35 +++++++---
.../apache/impala/analysis/AnalyzeDDLTest.java | 30 ++++++--
.../impala/analysis/AnalyzeStmtsTest.java | 57 ++++++++++++++++
.../impala/analysis/AuthorizationTest.java | 17 ++++-
.../queries/QueryTest/functions-ddl.test | 72 ++++++++++++++++++++
7 files changed, 195 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/08d386f0/fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java b/fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
index d5ab52a..9e070b9 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
@@ -134,7 +134,7 @@ public abstract class CreateFunctionStmtBase extends StatementBase {
@Override
public void analyze(Analyzer analyzer) throws AnalysisException {
// Validate function name is legal
- fnName_.analyze(analyzer);
+ fnName_.analyze(analyzer, false);
if (hasSignature()) {
// Validate function arguments and return type.
@@ -149,12 +149,6 @@ public abstract class CreateFunctionStmtBase extends StatementBase {
analyzer.registerPrivReq(new PrivilegeRequest(
new AuthorizeableFn(fn_.dbName(), fn_.signatureString()), Privilege.CREATE));
- Db builtinsDb = analyzer.getCatalog().getDb(Catalog.BUILTINS_DB);
- if (builtinsDb.containsFunction(fn_.getName())) {
- throw new AnalysisException("Function cannot have the same name as a builtin: " +
- fn_.getFunctionName().getFunction());
- }
-
db_ = analyzer.getDb(fn_.dbName(), true);
Function existingFn = db_.getFunction(fn_, Function.CompareMode.IS_INDISTINGUISHABLE);
if (existingFn != null && !ifNotExists_) {
http://git-wip-us.apache.org/repos/asf/impala/blob/08d386f0/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java b/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
index 1a533d1..b82b9d2 100644
--- a/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
@@ -74,7 +74,7 @@ public class DropFunctionStmt extends StatementBase {
@Override
public void analyze(Analyzer analyzer) throws AnalysisException {
- fnName_.analyze(analyzer);
+ fnName_.analyze(analyzer, false);
if (hasSignature()) {
fnArgs_.analyze(analyzer);
http://git-wip-us.apache.org/repos/asf/impala/blob/08d386f0/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/FunctionName.java b/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
index 00d702c..0ce8735 100644
--- a/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
+++ b/fe/src/main/java/org/apache/impala/analysis/FunctionName.java
@@ -85,6 +85,24 @@ public class FunctionName {
}
public void analyze(Analyzer analyzer) throws AnalysisException {
+ analyze(analyzer, true);
+ }
+
+ /**
+ * Path resolution happens as follows.
+ *
+ * Fully-qualified function name:
+ * - Set the database name to the database name specified.
+ *
+ * Non-fully-qualified function name:
+ * - When preferBuiltinsDb is true:
+ * - If the function name specified has the same name as a built-in function,
+ * set the database name to _impala_builtins.
+ * - Else, set the database name to the current session DB name.
+ * - When preferBuiltinsDb is false: set the database name to current session DB name.
+ */
+ public void analyze(Analyzer analyzer, boolean preferBuiltinsDb)
+ throws AnalysisException {
if (isAnalyzed_) return;
analyzeFnNamePath();
if (fn_.isEmpty()) throw new AnalysisException("Function name cannot be empty.");
@@ -100,20 +118,15 @@ public class FunctionName {
}
// Resolve the database for this function.
+ Db builtinDb = analyzer.getCatalog().getBuiltinsDb();
if (!isFullyQualified()) {
- Db builtinDb = analyzer.getCatalog().getBuiltinsDb();
- if (builtinDb.containsFunction(fn_)) {
- // If it isn't fully qualified and is the same name as a builtin, use
- // the builtin.
+ db_ = analyzer.getDefaultDb();
+ if (preferBuiltinsDb && builtinDb.containsFunction(fn_)) {
db_ = Catalog.BUILTINS_DB;
- isBuiltin_ = true;
- } else {
- db_ = analyzer.getDefaultDb();
- isBuiltin_ = false;
}
- } else {
- isBuiltin_ = db_.equals(Catalog.BUILTINS_DB);
}
+ Preconditions.checkNotNull(db_);
+ isBuiltin_ = db_.equals(Catalog.BUILTINS_DB) && builtinDb.containsFunction(fn_);
isAnalyzed_ = true;
}
@@ -124,7 +137,7 @@ public class FunctionName {
String.format("Invalid function name: '%s'. Expected [dbname].funcname.",
Joiner.on(".").join(fnNamePath_)));
} else if (fnNamePath_.size() > 1) {
- db_ = fnNamePath_.get(0);
+ db_ = fnNamePath_.get(0).toLowerCase();
fn_ = fnNamePath_.get(1).toLowerCase();
} else {
Preconditions.checkState(fnNamePath_.size() == 1);
http://git-wip-us.apache.org/repos/asf/impala/blob/08d386f0/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
index 8f9e384..c2bae5c 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
@@ -3114,12 +3114,32 @@ public class AnalyzeDDLTest extends FrontendTestBase {
"Could not find function FakePrepare(impala_udf::FunctionContext*, "+
"impala_udf::FunctionContext::FunctionStateScope) in: ");
- // TODO: https://issues.apache.org/jira/browse/IMPALA-6724
// Try to create a function with the same name as a builtin
- // AnalysisError("create function sin(double) RETURNS double" + udfSuffix,
- // "Function cannot have the same name as a builtin: sin");
- // AnalysisError("create function sin() RETURNS double" + udfSuffix,
- // "Function cannot have the same name as a builtin: sin");
+ AnalyzesOk("create function sin(double) RETURNS double" + udfSuffix);
+ AnalyzesOk("create function sin() RETURNS double" + udfSuffix);
+ // Try to create a function in the system database.
+ AnalysisError("create function _impala_builtins.sin(double) returns double" +
+ udfSuffix, "Cannot modify system database.");
+ AnalysisError("create function sin(double) returns double" + udfSuffix,
+ createAnalysisCtx("_impala_builtins"), "Cannot modify system database.");
+ AnalysisError("create function _impala_builtins.f(double) returns double" +
+ udfSuffix, "Cannot modify system database.");
+
+ // Try to drop a function with the same name as builtin.
+ addTestFunction("sin", Lists.newArrayList(Type.DOUBLE), false);
+ // default.sin(double) function exists.
+ AnalyzesOk("drop function sin(double)");
+ // default.cos(double) does not exist.
+ AnalysisError("drop function cos(double)", "Function does not exist: cos(DOUBLE)");
+ AnalysisError("drop function _impala_builtins.sin(double)",
+ "Cannot modify system database.");
+
+ // Try to select a function with the same name as builtin.
+ // This will call _impala_builtins.sin(1).
+ AnalyzesOk("select sin(1)");
+ AnalyzesOk("select _impala_builtins.sin(1)");
+ AnalyzesOk("select default.sin(1)");
+ AnalysisError("select functional.sin(1)", "functional.sin() unknown");
// Try to create with a bad location
AnalysisError("create function foo() RETURNS int LOCATION 'bad-location' SYMBOL='c'",
http://git-wip-us.apache.org/repos/asf/impala/blob/08d386f0/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
index 4720abc..10a3e9d 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
@@ -18,18 +18,21 @@
package org.apache.impala.analysis;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import java.lang.reflect.Field;
import java.util.List;
+import org.apache.impala.analysis.AnalysisContext.AnalysisResult;
import org.apache.impala.catalog.Column;
import org.apache.impala.catalog.PrimitiveType;
import org.apache.impala.catalog.ScalarType;
import org.apache.impala.catalog.Table;
import org.apache.impala.catalog.Type;
import org.apache.impala.common.AnalysisException;
+import org.apache.impala.common.ImpalaException;
import org.apache.impala.common.RuntimeEnv;
import org.junit.Assert;
import org.junit.Test;
@@ -3727,4 +3730,58 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
AnalyzesOk("set foo=true");
AnalyzesOk("set");
}
+
+ @Test
+ public void TestFunctionPaths() throws ImpalaException {
+ // The statement here does not matter since we just need to get a dummy analyzer
+ // to be able to call FuntionName.analyze(Analyzer, boolean) method.
+ Analyzer dummyAnalyzer = ((StatementBase) AnalyzesOk("select 1")).getAnalyzer();
+ FunctionName fnName = new FunctionName(null, "sin");
+ fnName.analyze(dummyAnalyzer, false);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName(null, "f");
+ fnName.analyze(dummyAnalyzer, false);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName("db", "sin");
+ fnName.analyze(dummyAnalyzer, false);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName("db", "f");
+ fnName.analyze(dummyAnalyzer, false);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName("_impala_builtins", "sin");
+ fnName.analyze(dummyAnalyzer, false);
+ assertTrue(fnName.isBuiltin());
+
+ fnName = new FunctionName("_impala_builtins", "f");
+ fnName.analyze(dummyAnalyzer, false);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName(null, "sin");
+ fnName.analyze(dummyAnalyzer, true);
+ assertTrue(fnName.isBuiltin());
+
+ fnName = new FunctionName(null, "f");
+ fnName.analyze(dummyAnalyzer, true);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName("db", "sin");
+ fnName.analyze(dummyAnalyzer, false);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName("db", "f");
+ fnName.analyze(dummyAnalyzer, true);
+ assertFalse(fnName.isBuiltin());
+
+ fnName = new FunctionName("_impala_builtins", "sin");
+ fnName.analyze(dummyAnalyzer, false);
+ assertTrue(fnName.isBuiltin());
+
+ fnName = new FunctionName("_impala_builtins", "f");
+ fnName.analyze(dummyAnalyzer, false);
+ assertFalse(fnName.isBuiltin());
+ }
}
http://git-wip-us.apache.org/repos/asf/impala/blob/08d386f0/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
index d43ebe3..d59f55f 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
@@ -2328,7 +2328,21 @@ public class AuthorizationTest extends FrontendTestBase {
// the same name as the built-in function.
AuthzError(ctx, "create function sin(double) returns double location " +
"'/test-warehouse/libTestUdfs.so' symbol='NoArgs'",
+ "User '%s' does not have privileges to CREATE/DROP functions in: " +
+ "default.sin(DOUBLE)");
+ // User tries to create a function in the system database.
+ AuthzError(ctx, "create function _impala_builtins.sin(double) returns double " +
+ "location '/test-warehouse/libTestUdfs.so' symbol='NoArgs'",
"Cannot modify system database.");
+ AuthzError(ctx, "create function _impala_builtins.f(double) returns double " +
+ "location '/test-warehouse/libTestUdfs.so' symbol='NoArgs'",
+ "Cannot modify system database.");
+ // User tries to drop a function with the same name as builtin.
+ AuthzError(ctx, "drop function _impala_builtins.sin(double)",
+ "Cannot modify system database.");
+ AuthzError(ctx, "drop function sin(double)",
+ "User '%s' does not have privileges to CREATE/DROP functions in: " +
+ "default.sin(DOUBLE)");
// TODO: Add test support for dynamically changing privileges for
// file-based policy.
@@ -2354,8 +2368,7 @@ public class AuthorizationTest extends FrontendTestBase {
AuthzError("create function _impala_builtins.f() returns int location " +
"'/test-warehouse/libTestUdfs.so' symbol='NoArgs'",
"Cannot modify system database.");
- AuthzError("drop function if exists pi()",
- "Cannot modify system database.");
+ AuthzOk("drop function if exists pi()");
// Add default.f(), tpch.f()
ctx_.catalog.addFunction(ScalarFunction.createForTesting("default", "f",
http://git-wip-us.apache.org/repos/asf/impala/blob/08d386f0/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test b/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
index 22d7122..d41d9da 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
@@ -295,3 +295,75 @@ show analytic functions in _impala_builtins;
---- TYPES
STRING, STRING, STRING, STRING
====
+---- QUERY
+# Create a function that has the same name as built-in function without FQN.
+CREATE FUNCTION sin(int) RETURNS int
+LOCATION '$FILESYSTEM_PREFIX/test-warehouse/libTestUdfs.so' SYMBOL='Fn'
+====
+---- QUERY
+# Make sure the custom sin function was created successfully.
+SHOW FUNCTIONS IN $DATABASE;
+---- RESULTS: VERIFY_IS_SUBSET
+'INT','sin(INT)','NATIVE','true'
+---- TYPES
+STRING, STRING, STRING, STRING
+====
+---- QUERY
+# Query using custom and built-in sin functions.
+SELECT $DATABASE.sin(0), sin(0);
+---- RESULTS:
+NULL,0
+---- TYPES
+INT, DOUBLE
+====
+---- QUERY
+# Drop a function that has the same name as built-in function without FQN.
+DROP FUNCTION sin(int);
+====
+---- QUERY
+# Make sure the custom sin function was dropped successfully.
+SHOW FUNCTIONS IN $DATABASE;
+---- LABELS
+return type, signature, binary type, is persistent
+---- RESULTS: VERIFY_IS_NOT_IN
+'INT','sin(INT)','NATIVE','true'
+---- TYPES
+STRING, STRING, STRING, STRING
+====
+---- QUERY
+# Create a function that has the same name as built-in function with FQN.
+CREATE FUNCTION $DATABASE.sin(int) RETURNS int
+LOCATION '$FILESYSTEM_PREFIX/test-warehouse/libTestUdfs.so' SYMBOL='Fn'
+====
+---- QUERY
+# Make sure the custom sin function was created successfully.
+SHOW FUNCTIONS IN $DATABASE;
+---- LABELS
+return type, signature, binary type, is persistent
+---- RESULTS: VERIFY_IS_SUBSET
+'INT','sin(INT)','NATIVE','true'
+---- TYPES
+STRING, STRING, STRING, STRING
+====
+---- QUERY
+# Query using custom sin and built-in sin functions.
+SELECT $DATABASE.sin(0), sin(0);
+---- RESULTS:
+NULL,0
+---- TYPES
+INT, DOUBLE
+====
+---- QUERY
+# Drop a function that has the same name as built-in function with FQN.
+DROP FUNCTION $DATABASE.sin(int);
+====
+---- QUERY
+# Make sure the custom sin function was dropped successfully.
+SHOW FUNCTIONS IN $DATABASE;
+---- LABELS
+return type, signature, binary type, is persistent
+---- RESULTS: VERIFY_IS_NOT_IN
+'INT','sin(INT)','NATIVE','true'
+---- TYPES
+STRING, STRING, STRING, STRING
+=====
\ No newline at end of file