@sqlnullable annotation check

[MEDSMEDS]

1. If the implementation of udf function is sth like the following, it will incur NPE at run time.

@ScalarFunction("..")
@SqlType(StandardTypes.VARCHAR)
public static Slice Func(...)
{
    might return null
}

2. The stack looks like this:

   java.lang.NullPointerException: undefined
   at com.facebook.presto.common.type.AbstractVarcharType.writeSlice(AbstractVarcharType.java:140)
   at com.facebook.presto.$gen.PageProjectionWork_20210120_030458_6153.evaluate(Unknown Source)
   at com.facebook.presto.$gen.PageProjectionWork_20210120_030458_6153.process(Unknown Source)
   ...

3. the following code in ParametricScalarImplementation.java handle error cases only for boxed/primitive type, but not for other type that will accept null pointer.

   if (Primitives.isWrapperType(actualReturnType)) {
   checkArgument(nullable, "Method [%s] has wrapper return type %s but is missing @SqlNullable", method, actualReturnType.getSimpleName());
   }
   else if (actualReturnType.isPrimitive()) {
   checkArgument(!nullable, "Method [%s] annotated with @SqlNullable has primitive return type %s", method, actualReturnType.getSimpleName());
   }

4. So in BytecodeUtils.java the null ptr is not handled properly because the isNullable() is false, the wasNullVariable logic will not be included in codegen.

if (function.isNullable()) {
    switch (bestChoice.getReturnPlaceConvention()) {
        case STACK:
            block.append(unboxPrimitiveIfNecessary(scope, returnType));
            break;
        case PROVIDED_BLOCKBUILDER:
            // no-op
            break;
        default:
            throw new UnsupportedOperationException(format("Unsupported return place convention: %s", bestChoice.getReturnPlaceConvention()));
    }
}

5. Finally the generateWrite() will generate code that call the following method without check.

   String methodName = "write" + Primitives.wrap(valueJavaType).getSimpleName();

@Override
public void writeSlice(BlockBuilder blockBuilder, Slice value)
{
    writeSlice(blockBuilder, value, 0, value.length()); // call length() on null ptr!!!
}

So is it possible to widen the check in point 3 to eliminate the problem? UDF incurred bug pops up very frequent and we hope to detect problems as much as possible during plugin installation stage. The annotation based UDF programming is great but the code is too flexible sometimes.

Thanks!

[findepi]

The issues are not the best way to get help with UDFs. Did you try #dev channel on our slack?

[MEDSMEDS]

OK. will redirect to slack.

[jinyangli34]

Hitting the same issue. NPE will happen if @SqlNullable annotation is missing. The slack history is gone. Anyone knows what's the discussion conclusion?

[jinyangli34]

Also noticed there is CI check error when using @SqlNullable annotation on primitive type:

"Method [%s] annotated with @SqlNullable has primitive return type %s"

But no such check for non-primitive types. Would require to use either SqlNullable or SqlNonNull on non-primitive types help? (with a new annotation)