[
https://issues.apache.org/jira/browse/CASSANDRA-8374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14541685#comment-14541685
]
Benjamin Lerer commented on CASSANDRA-8374:
-------------------------------------------
I have a few comments and few nits:
* Several unit tests do not pass:
{{UFTest.testDropKeyspaceContainingFunctionDropsPreparedStatementsWithDelayedValues}},
{{UFTest.testDropFunctionDropsPreparedStatementsWithDelayedValues}},
* I do not understand the reason why we cannot replace a function with
{{RETURNS NULL ON NULL INPUT}} by the same one with {{CALLED ON NULL}}. What if
the user made a mistake and want to correct it.
* If I am not mistaken, the following part of the code in
{{UDAggregate.compute}} can be simplified:
{code}
List<ByteBuffer> fArgs = Collections.singletonList(state);
if (!(stateFunction instanceof UDFunction) ||
((UDFunction)stateFunction).allowNullCheck(fArgs))
state = finalFunction.execute(protocolVersion, fArgs);
else
state = null;
return state;
{code}
As {{UDFunction.execute}} call {{allowNullCheck}} internally and return
{{null}} if it returns {{false}}, you could change it to:
{code}
List<ByteBuffer> fArgs = Collections.singletonList(state);
return finalFunction.execute(protocolVersion, fArgs);
{code}
* The following part of the code from {{UDAggregate.addInput}} will trigger 2
checks for null arguments (as {{allowNullCheck}} is also called internally by
the execute method) :
{code}
if (!(stateFunction instanceof UDFunction) ||
((UDFunction)stateFunction).allowNullCheck(fArgs))
state = stateFunction.execute(protocolVersion, fArgs);
{code}
* The method {{UDHelper.javaTypePrimitive}} should use {{if ... else}} to avoid
unecessary checks
For the Nits:
* The name of the new tests must be changed as they still reflect the initial
{{ALLOW NULL}} syntax
* I think that {{UDHelper.javaTypePrimitive}} and {{UDFunction.allowNullCheck}}
have really confusing names. the reason could be that they are both doing 2
tasks: checking {{calledOnNullInput}} and based on the result performing an
operation or not.
* The javadoc of {{UDFunction.generateExecuteMethod}} still refer to the
execute method
* In {{CQL.textile}} some of the markups do not render correctly. It is
apparently the case in different places in the doc. I am not sure why.
* The modifier is missing for {{ScalarFunction.isCalledOnNullInput}}
> Better support of null for UDF
> ------------------------------
>
> Key: CASSANDRA-8374
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8374
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Sylvain Lebresne
> Assignee: Robert Stupp
> Labels: client-impacting, cql3.3, docs-impacting, udf
> Fix For: 2.2 beta 1
>
> Attachments: 8374-3.txt, 8374-3.txt, 8473-1.txt, 8473-2.txt,
> 8473-4.txt
>
>
> Currently, every function needs to deal with it's argument potentially being
> {{null}}. There is very many case where that's just annoying, users should be
> able to define a function like:
> {noformat}
> CREATE FUNCTION addTwo(val int) RETURNS int LANGUAGE JAVA AS 'return val + 2;'
> {noformat}
> without having this crashing as soon as a column it's applied to doesn't a
> value for some rows (I'll note that this definition apparently cannot be
> compiled currently, which should be looked into).
> In fact, I think that by default methods shouldn't have to care about
> {{null}} values: if the value is {{null}}, we should not call the method at
> all and return {{null}}. There is still methods that may explicitely want to
> handle {{null}} (to return a default value for instance), so maybe we can add
> an {{ALLOW NULLS}} to the creation syntax.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
