[
https://issues.apache.org/jira/browse/CASSANDRA-7813?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14182993#comment-14182993
]
Benjamin Lerer commented on CASSANDRA-7813:
-------------------------------------------
Here are my feedbacks:
* In {{FunctionName.isReservedKeyspace()}} you should use
{{Keyspace.SYSTEM_KS}} instead of "system" and do the comparison using
{{equalsIgnoreCase}} instead of {{equals}}. You should also moves the function
into {{Keyspace}} as it belongs more there. It will be nice also if once you
done that you could use the function in
{{ThriftValidation.validateKeyspaceNotSystem}}
* As we use FunctionName as a map key we should keep it immutable and create a
new instance if we want to change the keyspace, so the class should be final
and the fields also.
* In {{CreateFunctionStatement.prepareKeyspace}} you make sure that the
keyspace is set but then on {{checkAccess}} and {{validate}} you still assume
that it can be unset. So you can remove that code.
* In {{CreateFunctionStatement.announceMigration}} we now know the keyspace so
we should be able to use it instead of passing null to the functions.
* The check if the keyspace exist should be done in my opinion in
{{CreateFunctionStatement.prepareKeyspace}} and not in the {{MigrationManager}}
* The 3 previous feedbacks also apply to {{DropFunctionStatement}}
* The {{DropFunctionStatement.prepareKeyspace}} is copy past of the one of
{{CreateFunctionStatement}} you should change the error messages
* When the function is a native function it should be associated to the
{{system}} keyspace so it {{FunctionName.keyspace}} should be {{system}} not an
empty string.
* {code}
candidates = declared.get(name);
List<Function> candidatesCurrentKs = declared.get(new FunctionName(keyspace,
name.getName()));
if (candidates.isEmpty())
candidates = candidatesCurrentKs;
else if (!candidatesCurrentKs.isEmpty())
{
List<Function> tmp = new ArrayList<>(candidates.size() +
candidatesCurrentKs.size());
tmp.addAll(candidates);
tmp.addAll(candidatesCurrentKs);
candidates = tmp;
}{code} can be replaced by {code}candidates = new ArrayList<>();
candidates.addAll(declared.get(name));
candidates.addAll(declared.get(new FunctionName(keyspace,
name.getName())));{code}
* The changes are breaking AggregationTest
> Decide how to deal with conflict between native and user-defined functions
> --------------------------------------------------------------------------
>
> Key: CASSANDRA-7813
> URL: https://issues.apache.org/jira/browse/CASSANDRA-7813
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Sylvain Lebresne
> Assignee: Robert Stupp
> Labels: cql
> Fix For: 3.0
>
> Attachments: 7813.txt, 7813v2.txt, 7813v3.txt
>
>
> We have a bunch of native/hardcoded functions (now(), dateOf(), ...) and in
> 3.0, user will be able to define new functions. Now, there is a very high
> change that we will provide more native functions over-time (to be clear, I'm
> not particularly for adding native functions for allthethings just because we
> can, but it's clear that we should ultimately provide more than what we
> have). Which begs the question: how do we want to deal with the problem of
> adding a native function potentially breaking a previously defined
> user-defined function?
> A priori I see the following options (maybe there is more?):
> # don't do anything specific, hoping that it won't happen often and consider
> it a user problem if it does.
> # reserve a big number of names that we're hoping will cover all future need.
> # make native function and user-defined function syntactically distinct so it
> cannot happen.
> I'm not a huge fan of solution 1). Solution 2) is actually what we did for
> UDT but I think it's somewhat less practical here: there is so much types
> that it makes sense to provide natively and so it wasn't too hard to come up
> with a reasonably small list of types name to reserve just in case. This
> feels a lot harder for functions to me.
> Which leaves solution 3). Since we already have the concept of namespaces for
> functions, a simple idea would be to force user function to have namespace.
> We could even allow that namespace to be empty as long as we force the
> namespace separator (so we'd allow {{bar::foo}} and {{::foo}} for user
> functions, but *not* {{foo}} which would be reserved for native function).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
