Repository: cassandra Updated Branches: refs/heads/trunk ed0532c64 -> a88a78380
Require arg types to disambiguate UDF drops Patch by Robert Stupp; reviewed by Tyler Hobbs for CASSANDRA-7812 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/a88a7838 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/a88a7838 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/a88a7838 Branch: refs/heads/trunk Commit: a88a78380ea26a25457a58a115195eb944bf0420 Parents: ed0532c Author: Robert Stupp <sn...@snazy.de> Authored: Wed Sep 3 10:26:30 2014 -0500 Committer: Tyler Hobbs <ty...@datastax.com> Committed: Wed Sep 3 10:26:30 2014 -0500 ---------------------------------------------------------------------- CHANGES.txt | 1 + src/java/org/apache/cassandra/cql3/Cql.g | 13 +++- .../cassandra/cql3/functions/UDFunction.java | 12 ++-- .../cql3/statements/DropFunctionStatement.java | 68 +++++++++++++++++--- .../cassandra/service/MigrationManager.java | 10 ++- test/unit/org/apache/cassandra/cql3/UFTest.java | 23 ++++++- 6 files changed, 105 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/a88a7838/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index babfa0e..281b63b 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0 + * Require arg types to disambiguate UDF drops (CASSANDRA-7812) * Do anticompaction in groups (CASSANDRA-6851) * Verify that UDF class methods are static (CASSANDRA-7781) * Support pure user-defined functions (CASSANDRA-7395, 7740) http://git-wip-us.apache.org/repos/asf/cassandra/blob/a88a7838/src/java/org/apache/cassandra/cql3/Cql.g ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/Cql.g b/src/java/org/apache/cassandra/cql3/Cql.g index af2c239..8c40885 100644 --- a/src/java/org/apache/cassandra/cql3/Cql.g +++ b/src/java/org/apache/cassandra/cql3/Cql.g @@ -527,11 +527,22 @@ createFunctionStatement returns [CreateFunctionStatement expr] dropFunctionStatement returns [DropFunctionStatement expr] @init { boolean ifExists = false; + List<CQL3Type.Raw> argsTypes = new ArrayList<>(); + boolean argsPresent = false; } : K_DROP K_FUNCTION (K_IF K_EXISTS { ifExists = true; } )? fn=functionName - { $expr = new DropFunctionStatement(fn, ifExists); } + ( + '(' + ( + v=comparatorType { argsTypes.add(v); } + ( ',' v=comparatorType { argsTypes.add(v); } )* + )? + ')' + { argsPresent = true; } + )? + { $expr = new DropFunctionStatement(fn, argsTypes, argsPresent, ifExists); } ; /** http://git-wip-us.apache.org/repos/asf/cassandra/blob/a88a7838/src/java/org/apache/cassandra/cql3/functions/UDFunction.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/functions/UDFunction.java b/src/java/org/apache/cassandra/cql3/functions/UDFunction.java index 558fb72..aeefe41 100644 --- a/src/java/org/apache/cassandra/cql3/functions/UDFunction.java +++ b/src/java/org/apache/cassandra/cql3/functions/UDFunction.java @@ -138,11 +138,15 @@ public abstract class UDFunction extends AbstractFunction return new Mutation(Keyspace.SYSTEM_KS, kv.decompose(name.namespace, name.name)); } - // TODO: we should allow removing just one function, not all functions having a given name - public static Mutation dropFromSchema(long timestamp, FunctionName fun) + public Mutation toSchemaDrop(long timestamp) { - Mutation mutation = makeSchemaMutation(fun); - mutation.delete(SystemKeyspace.SCHEMA_FUNCTIONS_CF, timestamp); + Mutation mutation = makeSchemaMutation(name); + ColumnFamily cf = mutation.addOrGet(SystemKeyspace.SCHEMA_FUNCTIONS_CF); + + Composite prefix = CFMetaData.SchemaFunctionsCf.comparator.make(computeSignature(argTypes)); + int ldt = (int) (System.currentTimeMillis() / 1000); + cf.addAtom(new RangeTombstone(prefix, prefix.end(), timestamp, ldt)); + return mutation; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/a88a7838/src/java/org/apache/cassandra/cql3/statements/DropFunctionStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/DropFunctionStatement.java b/src/java/org/apache/cassandra/cql3/statements/DropFunctionStatement.java index 4c963a8..78c8607 100644 --- a/src/java/org/apache/cassandra/cql3/statements/DropFunctionStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/DropFunctionStatement.java @@ -17,10 +17,13 @@ */ package org.apache.cassandra.cql3.statements; +import java.util.ArrayList; import java.util.List; import org.apache.cassandra.auth.Permission; +import org.apache.cassandra.cql3.CQL3Type; import org.apache.cassandra.cql3.functions.*; +import org.apache.cassandra.db.marshal.AbstractType; import org.apache.cassandra.exceptions.InvalidRequestException; import org.apache.cassandra.exceptions.RequestValidationException; import org.apache.cassandra.exceptions.UnauthorizedException; @@ -35,10 +38,17 @@ public final class DropFunctionStatement extends SchemaAlteringStatement { private final FunctionName functionName; private final boolean ifExists; + private final List<CQL3Type.Raw> argRawTypes; + private final boolean argsPresent; - public DropFunctionStatement(FunctionName functionName, boolean ifExists) + public DropFunctionStatement(FunctionName functionName, + List<CQL3Type.Raw> argRawTypes, + boolean argsPresent, + boolean ifExists) { this.functionName = functionName; + this.argRawTypes = argRawTypes; + this.argsPresent = argsPresent; this.ifExists = ifExists; } @@ -68,18 +78,58 @@ public final class DropFunctionStatement extends SchemaAlteringStatement public boolean announceMigration(boolean isLocalOnly) throws RequestValidationException { List<Function> olds = Functions.find(functionName); - if (olds == null || olds.isEmpty()) + + if (!argsPresent && olds != null && olds.size() > 1) + throw new InvalidRequestException(String.format("'DROP FUNCTION %s' matches multiple function definitions; " + + "specify the argument types by issuing a statement like " + + "'DROP FUNCTION %s (type, type, ...)'. Hint: use cqlsh " + + "'DESCRIBE FUNCTION %s' command to find all overloads", + functionName, functionName, functionName)); + + List<AbstractType<?>> argTypes = new ArrayList<>(argRawTypes.size()); + for (CQL3Type.Raw rawType : argRawTypes) + { + // We have no proper keyspace to give, which means that this will break (NPE currently) + // for UDT: #7791 is open to fix this + argTypes.add(rawType.prepare(null).getType()); + } + + Function old; + if (argsPresent) + { + old = Functions.find(functionName, argTypes); + if (old == null) + { + if (ifExists) + return false; + // just build a nicer error message + StringBuilder sb = new StringBuilder(); + for (CQL3Type.Raw rawType : argRawTypes) + { + if (sb.length() > 0) + sb.append(", "); + sb.append(rawType); + } + throw new InvalidRequestException(String.format("Cannot drop non existing function '%s(%s)'", + functionName, sb)); + } + } + else { - if (ifExists) - return false; - throw new InvalidRequestException(String.format("Cannot drop non existing function '%s'", functionName)); + if (olds == null || olds.isEmpty()) + { + if (ifExists) + return false; + throw new InvalidRequestException(String.format("Cannot drop non existing function '%s'", functionName)); + } + old = olds.get(0); } - for (Function f : olds) - if (f.isNative()) - throw new InvalidRequestException(String.format("Cannot drop function '%s' because it has native overloads", functionName)); + if (old.isNative()) + throw new InvalidRequestException(String.format("Cannot drop function '%s' because it is a " + + "native (built-in) function", functionName)); - MigrationManager.announceFunctionDrop(functionName, isLocalOnly); + MigrationManager.announceFunctionDrop((UDFunction)old, isLocalOnly); return true; } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/a88a7838/src/java/org/apache/cassandra/service/MigrationManager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/MigrationManager.java b/src/java/org/apache/cassandra/service/MigrationManager.java index b0cf79d..4f03483 100644 --- a/src/java/org/apache/cassandra/service/MigrationManager.java +++ b/src/java/org/apache/cassandra/service/MigrationManager.java @@ -38,14 +38,11 @@ import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.config.KSMetaData; import org.apache.cassandra.config.UTMetaData; import org.apache.cassandra.config.Schema; -import org.apache.cassandra.cql3.functions.FunctionName; import org.apache.cassandra.cql3.functions.UDFunction; import org.apache.cassandra.db.*; import org.apache.cassandra.db.marshal.UserType; import org.apache.cassandra.exceptions.AlreadyExistsException; import org.apache.cassandra.exceptions.ConfigurationException; -import org.apache.cassandra.exceptions.InvalidRequestException; -import org.apache.cassandra.exceptions.SyntaxException; import org.apache.cassandra.gms.*; import org.apache.cassandra.io.IVersionedSerializer; import org.apache.cassandra.io.util.DataOutputPlus; @@ -374,10 +371,11 @@ public class MigrationManager announce(addSerializedKeyspace(UTMetaData.dropFromSchema(droppedType, FBUtilities.timestampMicros()), droppedType.keyspace), announceLocally); } - public static void announceFunctionDrop(FunctionName fun, boolean announceLocally) + public static void announceFunctionDrop(UDFunction udf, boolean announceLocally) { - logger.info(String.format("Drop Function '%s'", fun)); - announce(UDFunction.dropFromSchema(FBUtilities.timestampMicros(), fun), announceLocally); + Mutation mutation = udf.toSchemaDrop(FBUtilities.timestampMicros()); + logger.info(String.format("Drop Function overload '%s' args '%s'", udf.name(), udf.argTypes())); + announce(mutation, announceLocally); } public static void announceNewFunction(UDFunction udf, boolean announceLocally) http://git-wip-us.apache.org/repos/asf/cassandra/blob/a88a7838/test/unit/org/apache/cassandra/cql3/UFTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/UFTest.java b/test/unit/org/apache/cassandra/cql3/UFTest.java index 84161b2..60a7a68 100644 --- a/test/unit/org/apache/cassandra/cql3/UFTest.java +++ b/test/unit/org/apache/cassandra/cql3/UFTest.java @@ -19,8 +19,6 @@ package org.apache.cassandra.cql3; import org.junit.Test; -import org.apache.cassandra.exceptions.InvalidRequestException; - public class UFTest extends CQLTester { public static Double sin(Double val) @@ -141,6 +139,10 @@ public class UFTest extends CQLTester assertInvalid("DROP FUNCTION foo::sin"); // but don't complain with "IF EXISTS" execute("DROP FUNCTION IF EXISTS foo::sin"); + + // can't drop native functions + assertInvalid("DROP FUNCTION dateof"); + assertInvalid("DROP FUNCTION uuid"); } @Test @@ -186,5 +188,22 @@ public class UFTest extends CQLTester assertEmpty(execute("SELECT v FROM %s WHERE k = overloaded((ascii)?)", "foo")); // And since varchar == text, this should work too assertEmpty(execute("SELECT v FROM %s WHERE k = overloaded((varchar)?)", "foo")); + + // no such functions exist... + assertInvalid("DROP FUNCTION overloaded(boolean)"); + assertInvalid("DROP FUNCTION overloaded(bigint)"); + + // 'overloaded' has multiple overloads - so it has to fail (CASSANDRA-7812) + assertInvalid("DROP FUNCTION overloaded"); + execute("DROP FUNCTION overloaded(varchar)"); + assertInvalid("SELECT v FROM %s WHERE k = overloaded((text)?)", "foo"); + execute("DROP FUNCTION overloaded(text, text)"); + assertInvalid("SELECT v FROM %s WHERE k = overloaded((text)?,(text)?)", "foo", "bar"); + execute("DROP FUNCTION overloaded(ascii)"); + assertInvalid("SELECT v FROM %s WHERE k = overloaded((ascii)?)", "foo"); + // single-int-overload must still work + assertRows(execute("SELECT v FROM %s WHERE k = overloaded((int)?)", 3), row(1)); + // overloaded has just one overload now - so the following DROP FUNCTION is not ambigious (CASSANDRA-7812) + execute("DROP FUNCTION overloaded"); } }
