vlsi commented on code in PR #5386:
URL: https://github.com/apache/calcite/pull/5386#discussion_r4457230342
##########
core/src/main/java/org/apache/calcite/util/ReflectUtil.java:
##########
@@ -590,6 +590,37 @@ public static boolean isParameterOptional(Method method,
int i) {
return false;
}
+ /** Derives whether the {@code i}th parameter of a method is nullable.
+ *
+ * <p>The algorithm is as follows:
+ * <ul>
+ * <li>If the parameter has a {@code @Nullable} annotation (from any
+ * well-known nullness library such as jspecify, Checker Framework,
+ * or javax.annotation), returns true.</li>
+ * <li>If the parameter has a {@link Parameter} annotation with
+ * {@code nullable = false}, returns false.</li>
+ * <li>Otherwise, returns whether the Java type is not a primitive
+ * (i.e., the default nullability derived from the type).</li>
+ * </ul>
+ */
+ public static boolean isParameterNullable(Method method, int i) {
+ final Annotation[] annotations = method.getParameterAnnotations()[i];
Review Comment:
`getParameterAnnotations()` returns declaration annotations only. jspecify
`@Nullable` and checker-qual `@Nullable` are `TYPE_USE` annotations, so this
loop never sees them: for `MyNullableParamFunction.eval`,
`getParameterAnnotations()[0]` is empty. Read
`method.getAnnotatedParameterTypes()[i].getAnnotations()` instead, and match
`org.jspecify.annotations.Nullable` and `NonNull` by fully qualified name. That
needs no dependency on jspecify and does not accept every annotation whose
simple name is `Nullable`.
##########
core/src/main/java/org/apache/calcite/util/ReflectUtil.java:
##########
@@ -590,6 +590,37 @@ public static boolean isParameterOptional(Method method,
int i) {
return false;
}
+ /** Derives whether the {@code i}th parameter of a method is nullable.
+ *
+ * <p>The algorithm is as follows:
+ * <ul>
+ * <li>If the parameter has a {@code @Nullable} annotation (from any
+ * well-known nullness library such as jspecify, Checker Framework,
+ * or javax.annotation), returns true.</li>
+ * <li>If the parameter has a {@link Parameter} annotation with
+ * {@code nullable = false}, returns false.</li>
+ * <li>Otherwise, returns whether the Java type is not a primitive
+ * (i.e., the default nullability derived from the type).</li>
+ * </ul>
+ */
+ public static boolean isParameterNullable(Method method, int i) {
+ final Annotation[] annotations = method.getParameterAnnotations()[i];
+ // Check for @Nullable annotation from any well-known library
+ for (Annotation annotation : annotations) {
+ if (annotation.annotationType().getSimpleName().equals("Nullable")) {
+ return true;
+ }
+ }
+ // Check @Parameter annotation for explicit nullable attribute
+ for (Annotation annotation : annotations) {
+ if (annotation.annotationType() == Parameter.class) {
+ return ((Parameter) annotation).nullable();
Review Comment:
This turns every primitive parameter with `@Parameter` into a nullable boxed
type: `@Parameter(name = "n") int n` in `MyLeftFunction` changes from `int NOT
NULL` to a nullable `Integer`. The same happens to `MyPlusFunction`,
`MyPlusInitFunction`, and `MazeTable.generate2`. Please add a test that a
primitive parameter with `@Parameter` stays `NOT NULL`.
##########
core/src/test/java/org/apache/calcite/test/UdfTest.java:
##########
@@ -666,6 +679,58 @@ private CalciteAssert.AssertThat withUdf() {
.returns("EXPR$0={a: 1, b: null, c: 3, d: null, e: null}\n");
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-6018";>[CALCITE-6018]
+ * Support nullable parameters in UDFs</a>.
+ *
+ * <p>Tests that {@code @Parameter(nullable = false)} makes a reference type
+ * parameter non-nullable. */
+ @Test void testNonNullParameter() throws Exception {
+ final CalciteAssert.AssertThat with = withUdf();
+ // Non-null parameter accepts non-null value
+ with.query("values \"adhoc\".my_nonnull_param('hello')")
+ .returns("EXPR$0=hello\n");
+
+ // Verify that the parameter type is non-nullable
+ Method method = Smalls.MyNonNullParamFunction.class.getMethod("eval",
String.class);
+ ScalarFunction func = ScalarFunctionImpl.create(method);
+ List<FunctionParameter> params = func.getParameters();
+ assertThat(params, hasSize(1));
+ org.apache.calcite.rel.type.RelDataTypeFactory typeFactory =
+ new org.apache.calcite.jdbc.JavaTypeFactoryImpl();
+ RelDataType paramType = params.get(0).getType(typeFactory);
+ assertThat("Parameter with @Parameter(nullable=false) should be
non-nullable",
+ paramType.isNullable(), is(false));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-6018";>[CALCITE-6018]
+ * Support nullable parameters in UDFs</a>.
+ *
+ * <p>Tests that {@code @Nullable} annotation on a parameter makes it
+ * explicitly nullable, and the function can handle null input. */
+ @Test void testNullableParameter() throws Exception {
Review Comment:
This test passes without the annotation, because `String` is nullable by
default and the `@Nullable` is never read. The test needs a case where the
annotation changes the result, such as a `@NullMarked` class with one
unannotated `String` parameter and one `@Nullable String` parameter.
##########
core/src/test/java/org/apache/calcite/test/UdfTest.java:
##########
@@ -666,6 +679,58 @@ private CalciteAssert.AssertThat withUdf() {
.returns("EXPR$0={a: 1, b: null, c: 3, d: null, e: null}\n");
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-6018";>[CALCITE-6018]
+ * Support nullable parameters in UDFs</a>.
+ *
+ * <p>Tests that {@code @Parameter(nullable = false)} makes a reference type
+ * parameter non-nullable. */
+ @Test void testNonNullParameter() throws Exception {
Review Comment:
Please add a case that passes `cast(null as varchar)` to `MY_NONNULL_PARAM`
and asserts whichever behavior the description settles on: a validation error
or a `NULL` result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
