[CALCITE-4284] ImmutableBeans: make reference properties non-nullable by default

Author: vlsi

core/src/main/java/org/apache/calcite/plan/RelRule.java

@@ -144,10 +144,10 @@ default <T extends Object> T as(Class<T> class_) {
 
     /** Description of the rule instance. */
     @ImmutableBeans.Property
-    String description();
+    @Nullable String description();
 
     /** Sets {@link #description()}. */
-    Config withDescription(String description);
+    Config withDescription(@Nullable String description);
 
     /** Creates the operands for the rule instance. */
     @ImmutableBeans.Property

core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java

@@ -287,6 +287,6 @@ public interface WriteOption {
      * Predicate for nodes that need to be highlighted.
      */
     @ImmutableBeans.Property
-    Predicate<RelNode> nodePredicate();
+    @Nullable Predicate<RelNode> nodePredicate();
   }
 }

core/src/main/java/org/apache/calcite/rel/rules/AggregateReduceFunctionsRule.java

@@ -881,7 +881,7 @@ public interface Config extends RelRule.Config {
 
     /** Returns the validated set of functions to reduce, or the default set
      * if not specified. */
-    @NonNull default Set<SqlKind> actualFunctionsToReduce() {
+    default Set<SqlKind> actualFunctionsToReduce() {
       final Set<SqlKind> set =
           Util.first(functionsToReduce(), DEFAULT_FUNCTIONS_TO_REDUCE);
       set.forEach(AggregateReduceFunctionsRule::validateFunction);

core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java

@@ -257,19 +257,19 @@ public interface Config {
     /** Sets {@link #identifierMaxLength()}. */
     Config withIdentifierMaxLength(int identifierMaxLength);
 
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     Casing quotedCasing();
 
     /** Sets {@link #quotedCasing()}. */
     Config withQuotedCasing(Casing casing);
 
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     Casing unquotedCasing();
 
     /** Sets {@link #unquotedCasing()}. */
     Config withUnquotedCasing(Casing casing);
 
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     Quoting quoting();
 
     /** Sets {@link #quoting()}. */
@@ -282,7 +282,7 @@ public interface Config {
     /** Sets {@link #caseSensitive()}. */
     Config withCaseSensitive(boolean caseSensitive);
 
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     SqlConformance conformance();
 
     /** Sets {@link #conformance()}. */
@@ -292,13 +292,13 @@ public interface Config {
     boolean allowBangEqual();
 
     /** Returns which character literal styles are supported. */
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     Set<CharLiteralStyle> charLiteralStyles();
 
     /** Sets {@link #charLiteralStyles()}. */
     Config withCharLiteralStyles(Set<CharLiteralStyle> charLiteralStyles);
 
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     SqlParserImplFactory parserFactory();
 
     /** Sets {@link #parserFactory()}. */

core/src/main/java/org/apache/calcite/sql/validate/SqlValidator.java

@@ -877,7 +877,7 @@ public interface Config {
 
     /** Returns the type coercion rules for explicit type coercion. */
     @ImmutableBeans.Property
-    SqlTypeCoercionRule typeCoercionRules();
+    @Nullable SqlTypeCoercionRule typeCoercionRules();
 
     /**
      * Sets the {@link SqlTypeCoercionRule} instance which defines the type conversion matrix
@@ -889,7 +889,7 @@ public interface Config {
      * @param rules The {@link SqlTypeCoercionRule} instance,
      *              see its documentation for how to customize the rules
      */
-    Config withTypeCoercionRules(SqlTypeCoercionRule rules);
+    Config withTypeCoercionRules(@Nullable SqlTypeCoercionRule rules);
 
     /** Returns the dialect of SQL (SQL:2003, etc.) this validator recognizes.
      * Default is {@link SqlConformanceEnum#DEFAULT}. */

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

@@ -6153,15 +6153,15 @@ public interface Config {
 
     /** Returns the factory to create {@link RelBuilder}, never null. Default is
      * {@link RelFactories#LOGICAL_BUILDER}. */
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     RelBuilderFactory getRelBuilderFactory();
 
     /** Sets {@link #getRelBuilderFactory()}. */
     Config withRelBuilderFactory(RelBuilderFactory factory);
 
     /** Returns a function that takes a {@link RelBuilder.Config} and returns
      * another. Default is the identity function. */
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     UnaryOperator<RelBuilder.Config> getRelBuilderConfigTransform();
 
     /** Sets {@link #getRelBuilderConfigTransform()}.
@@ -6180,7 +6180,7 @@ default Config addRelBuilderConfigTransform(
     /** Returns the hint strategies used to decide how the hints are propagated to
      * the relational expressions. Default is
      * {@link HintStrategyTable#EMPTY}. */
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     HintStrategyTable getHintStrategyTable();
 
     /** Sets {@link #getHintStrategyTable()}. */

core/src/main/java/org/apache/calcite/util/ImmutableBeans.java

@@ -34,6 +34,7 @@
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 import java.lang.invoke.MethodHandle;
+import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
@@ -118,8 +119,6 @@ private static <T extends Object> Def<T> makeDef(Class<T> beanClass) {
       if (property == null) {
         continue;
       }
-      final boolean hasNonnull =
-          hasAnnotation(method, "org.checkerframework.checker.nullness.qual.NonNull");
       final Mode mode;
       final Object defaultValue = getDefault(method);
       final String methodName = method.getName();
@@ -142,9 +141,10 @@ private static <T extends Object> Def<T> makeDef(Class<T> beanClass) {
         throw new IllegalArgumentException("method '" + methodName
             + "' has too many parameters");
       }
-      final boolean required = property.required()
-          || propertyType.isPrimitive()
-          || hasNonnull;
+      final boolean required = propertyType.isPrimitive()
+          || !hasAnnotation(
+              method.getAnnotatedReturnType(),
+          "org.checkerframework.checker.nullness.qual.Nullable");
       if (required) {
         requiredPropertyNames.add(propertyName);
       }
@@ -165,7 +165,8 @@ private static <T extends Object> Def<T> makeDef(Class<T> beanClass) {
             return v;
           }
           if (required && defaultValue == null) {
-            throw new IllegalArgumentException("property '" + propertyName
+            throw new IllegalArgumentException("property '" + beanClass.getName()
+                + "#" + propertyName
                 + "' is required and has no default value");
           }
           return defaultValue2;
@@ -340,9 +341,9 @@ private static Object value(boolean copy, Object o) {
 
   /** Looks for an annotation by class name.
    * Useful if you don't want to depend on the class
-   * (e.g. "org.checkerframework.checker.nullness.qual.NonNull") at compile time. */
-  private static boolean hasAnnotation(Method method, String className) {
-    for (Annotation annotation : method.getDeclaredAnnotations()) {
+   * (e.g. "org.checkerframework.checker.nullness.qual.Nullable") at compile time. */
+  private static boolean hasAnnotation(AnnotatedElement element, String className) {
+    for (Annotation annotation : element.getDeclaredAnnotations()) {
       if (annotation.annotationType().getName().equals(className)) {
         return true;
       }
@@ -419,16 +420,6 @@ private interface Handler<T extends Object> {
   @Retention(RetentionPolicy.RUNTIME)
   @Target(ElementType.METHOD)
   public @interface Property {
-    /** Whether the property is required.
-     *
-     * <p>Properties of type {@code int} and {@code boolean} are always
-     * required.
-     *
-     * <p>If a property is required, it cannot be set to null.
-     * If it has no default value, calling "get" will give a runtime exception.
-     */
-    boolean required() default false;
-
     /** Whether to make immutable copies of property values. */
     boolean makeImmutable() default true;
   }

core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java

@@ -54,6 +54,8 @@
 
 import com.google.common.collect.ImmutableList;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.util.ArrayList;
 import java.util.List;
 import java.util.function.Function;
@@ -231,8 +233,8 @@ default void noMat() {
     }
 
     @ImmutableBeans.Property
-    CalciteAssert.SchemaSpec getDefaultSchemaSpec();
-    Sql withDefaultSchemaSpec(CalciteAssert.SchemaSpec spec);
+    CalciteAssert.@Nullable SchemaSpec getDefaultSchemaSpec();
+    Sql withDefaultSchemaSpec(CalciteAssert.@Nullable SchemaSpec spec);
 
     @ImmutableBeans.Property
     List<Pair<String, String>> getMaterializations();
@@ -243,8 +245,8 @@ default void noMat() {
     Sql withQuery(String query);
 
     @ImmutableBeans.Property
-    Function<String, Boolean> getChecker();
-    Sql withChecker(Function<String, Boolean> checker);
+    @Nullable Function<String, Boolean> getChecker();
+    Sql withChecker(@Nullable Function<String, Boolean> checker);
 
     @ImmutableBeans.Property
     AbstractMaterializedViewTest getTester();

core/src/test/java/org/apache/calcite/util/ImmutableBeanTest.java

@@ -20,7 +20,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 
-import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
 import org.hamcrest.Matcher;
 import org.junit.jupiter.api.Test;
 
@@ -81,7 +81,8 @@ class ImmutableBeanTest {
       throw new AssertionError("expected error, got " + v);
     } catch (IllegalArgumentException e) {
       assertThat(e.getMessage(),
-          is("property 'IntSansDefault' is required and has no default value"));
+          is("property 'org.apache.calcite.util.ImmutableBeanTest$Bean2#IntSansDefault'"
+              + " is required and has no default value"));
     }
     assertThat(b.withIntSansDefault(4).getIntSansDefault(), is(4));
 
@@ -96,8 +97,8 @@ class ImmutableBeanTest {
       throw new AssertionError("expected error, got " + v);
     } catch (IllegalArgumentException e) {
       assertThat(e.getMessage(),
-          is("property 'BooleanSansDefault' is required and has no default "
-              + "value"));
+          is("property 'org.apache.calcite.util.ImmutableBeanTest$Bean2#BooleanSansDefault'"
+              + " is required and has no default value"));
     }
     assertThat(b.withBooleanSansDefault(false).isBooleanSansDefault(),
         is(false));
@@ -115,8 +116,8 @@ class ImmutableBeanTest {
       throw new AssertionError("expected error, got " + v);
     } catch (IllegalArgumentException e) {
       assertThat(e.getMessage(),
-          is("property 'StringSansDefault' is required and has no default "
-              + "value"));
+          is("property 'org.apache.calcite.util.ImmutableBeanTest$Bean2#StringSansDefault'"
+              + " is required and has no default value"));
     }
     assertThat(b.withStringSansDefault("a").getStringSansDefault(), is("a"));
 
@@ -126,7 +127,8 @@ class ImmutableBeanTest {
       throw new AssertionError("expected error, got " + v);
     } catch (IllegalArgumentException e) {
       assertThat(e.getMessage(),
-          is("property 'NonnullString' is required and has no default value"));
+          is("property 'org.apache.calcite.util.ImmutableBeanTest$Bean2#NonnullString'"
+              + " is required and has no default value"));
     }
     assertThat(b.withNonnullString("a").getNonnullString(), is("a"));
 
@@ -564,42 +566,42 @@ interface Bean2 {
     boolean isBooleanSansDefault();
     Bean2 withBooleanSansDefault(boolean x);
 
-    @ImmutableBeans.Property(required = true)
+    @ImmutableBeans.Property
     String getStringSansDefault();
     Bean2 withStringSansDefault(String x);
 
     @ImmutableBeans.Property
-    String getOptionalString();
-    Bean2 withOptionalString(String s);
+    @Nullable String getOptionalString();
+    Bean2 withOptionalString(@Nullable String s);
 
-    /** Property is required because it has 'Nonnull' annotation. */
+    /** Property is required because its return type does not have Nullable annotation. */
     @ImmutableBeans.Property
-    @NonNull String getNonnullString();
+    String getNonnullString();
     Bean2 withNonnullString(String s);
 
     @ImmutableBeans.Property
     @ImmutableBeans.StringDefault("abc")
-    @NonNull String getStringWithDefault();
-    Bean2 withStringWithDefault(String s);
+    String getStringWithDefault();
+    Bean2 withStringWithDefault(@Nullable String s);
 
     @ImmutableBeans.Property
     @ImmutableBeans.NullDefault
-    String getStringWithNullDefault();
-    Bean2 withStringWithNullDefault(String s);
+    @Nullable String getStringWithNullDefault();
+    Bean2 withStringWithNullDefault(@Nullable String s);
 
     @ImmutableBeans.Property
     @ImmutableBeans.EnumDefault("RED")
-    @NonNull Color getColorWithDefault();
-    Bean2 withColorWithDefault(Color color);
+    Color getColorWithDefault();
+    Bean2 withColorWithDefault(@Nullable Color color);
 
     @ImmutableBeans.Property
     @ImmutableBeans.NullDefault
-    Color getColorWithNullDefault();
-    Bean2 withColorWithNullDefault(Color color);
+    @Nullable Color getColorWithNullDefault();
+    Bean2 withColorWithNullDefault(@Nullable Color color);
 
     @ImmutableBeans.Property()
-    Color getColorOptional();
-    Bean2 withColorOptional(Color color);
+    @Nullable Color getColorOptional();
+    Bean2 withColorOptional(@Nullable Color color);
   }
 
   /** Red, blue, green. */
@@ -638,33 +640,33 @@ public interface SubBean extends MyBean {
   /** A bean that has collection-valued properties. */
   public interface CollectionBean {
     @ImmutableBeans.Property(makeImmutable = false)
-    List<String> list();
+    @Nullable List<String> list();
 
-    CollectionBean withList(List<String> list);
+    CollectionBean withList(@Nullable List<String> list);
 
     @ImmutableBeans.Property(makeImmutable = true)
-    List<String> immutableList();
+    @Nullable List<String> immutableList();
 
-    CollectionBean withImmutableList(List<String> list);
+    CollectionBean withImmutableList(@Nullable List<String> list);
 
     @ImmutableBeans.Property(makeImmutable = false)
-    Set<String> set();
+    @Nullable Set<String> set();
 
-    CollectionBean withSet(Set<String> set);
+    CollectionBean withSet(@Nullable Set<String> set);
 
     @ImmutableBeans.Property(makeImmutable = true)
-    Set<String> immutableSet();
+    @Nullable Set<String> immutableSet();
 
-    CollectionBean withImmutableSet(Set<String> set);
+    CollectionBean withImmutableSet(@Nullable Set<String> set);
 
     @ImmutableBeans.Property(makeImmutable = false)
-    Map<String, Integer> map();
+    @Nullable Map<String, Integer> map();
 
-    CollectionBean withMap(Map<String, Integer> map);
+    CollectionBean withMap(@Nullable Map<String, Integer> map);
 
     @ImmutableBeans.Property(makeImmutable = true)
-    Map<String, Integer> immutableMap();
+    @Nullable Map<String, Integer> immutableMap();
 
-    CollectionBean withImmutableMap(Map<String, Integer> map);
+    CollectionBean withImmutableMap(@Nullable Map<String, Integer> map);
   }
 }