[xuzifu666]

jira: https://issues.apache.org/jira/browse/CALCITE-6018

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
70.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[vlsi]

I'd drop Parameter.nullable() and read nullness from type-use annotations only, specifically jspecify. Nullness is a property of the parameter type, so the type system is the right place for it. An attribute on @Parameter duplicates it, can contradict it (@Nullable @Parameter(nullable = false)), cannot describe nested types such as List<@Nullable String>, and adds a public API to linq4j that we would have to keep.

Proposed rule: honor jspecify @Nullable and @NonNull only. An unannotated reference type is non-null only inside null-marked scope: the innermost @NullMarked or @NullUnmarked on the method, enclosing classes, package, or module decides. Elsewhere it keeps today's nullable default, so UDFs without jspecify annotations keep their types. Primitives stay NOT NULL.

The PR should also say what a non-null parameter means at the SQL level. Today a NULL argument for a parameter whose type is NOT NULL is accepted and the call returns NULL, so the declared type is not enforced. Either the validator rejects such a call, or the description states that only the reported parameter type changes.

[vlsi]

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.

[xuzifu666]

Done. Parameter.nullable() is removed entirely and linq4j is back to its original state. The public 4-arg ParameterListBuilder.add() overload was also demoted to private, so no new public API is exposed anywhere.

Nullness is now read from type-use annotations only, specifically jspecify:

1. ReflectUtil.isParameterNullable reads method.getAnnotatedParameterTypes()[i].getAnnotations() (you're right that getParameterAnnotations() never sees these TYPE_USE annotations) and matches org.jspecify.annotations.Nullable / NonNull by fully qualified name, so no dependency on jspecify and no simple-name matching of arbitrary Nullable annotations.
2. An unannotated reference type is non-null only inside a null-marked scope: the innermost @NullMarked / @NullUnmarked on the method, an enclosing class, the package, or the module decides. Elsewhere it keeps the historical nullable default, so UDFs without jspecify annotations keep their types.
3. Primitives are never nullable (this also fixes the bug you spotted at line 617, where @parameter int n became a nullable Integer; there is now a test asserting a primitive parameter with @parameter stays NOT NULL).

[vlsi]

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.

[xuzifu666]

I settled on the second option: the change only affects the parameter type reported through FunctionParameter.getType(RelDataTypeFactory). The validator does not reject a call that passes NULL to a parameter whose type is NOT NULL — the call is accepted and returns NULL. This is stated in the isParameterNullable javadoc and the PR description, and covered by a new test asserting values "adhoc".my_nonnull_param(cast(null as varchar)) returns NULL.

[vlsi]

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.

[xuzifu666]

done as suggested.

[vlsi]

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.

[xuzifu666]

Done. I added a test which asserting the NULL result.

[xuzifu666]

I'd drop Parameter.nullable() and read nullness from type-use annotations only, specifically jspecify. Nullness is a property of the parameter type, so the type system is the right place for it. An attribute on @Parameter duplicates it, can contradict it (@Nullable @Parameter(nullable = false)), cannot describe nested types such as List<@Nullable String>, and adds a public API to linq4j that we would have to keep.

Proposed rule: honor jspecify @Nullable and @NonNull only. An unannotated reference type is non-null only inside null-marked scope: the innermost @NullMarked or @NullUnmarked on the method, enclosing classes, package, or module decides. Elsewhere it keeps today's nullable default, so UDFs without jspecify annotations keep their types. Primitives stay NOT NULL.

The PR should also say what a non-null parameter means at the SQL level. Today a NULL argument for a parameter whose type is NOT NULL is accepted and the call returns NULL, so the declared type is not enforced. Either the validator rejects such a call, or the description states that only the reported parameter type changes.

Thanks for the review @vlsi and all points addressed, PTAL~

[mihaibudiu]

@vlsi if you are satisfied please approve

[vlsi]

I do not think disabling --release is the way to go.

[mihaibudiu]

I don't think this issue should change anything in the build system.