From e6121d7831698ff3619d0cc71a9bb8cc9e0a9d96 Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Sat, 26 Sep 2020 16:23:44 +0300 Subject: [PATCH] [CALCITE-4284] ImmutableBeans: make reference properties non-nullable by default --- .../java/org/apache/calcite/plan/RelRule.java | 4 +- .../calcite/rel/externalize/RelDotWriter.java | 2 +- .../rules/AggregateReduceFunctionsRule.java | 2 +- .../apache/calcite/sql/parser/SqlParser.java | 12 ++-- .../calcite/sql/validate/SqlValidator.java | 4 +- .../calcite/sql2rel/SqlToRelConverter.java | 6 +- .../apache/calcite/util/ImmutableBeans.java | 29 +++----- .../test/AbstractMaterializedViewTest.java | 10 +-- .../calcite/util/ImmutableBeanTest.java | 70 ++++++++++--------- 9 files changed, 67 insertions(+), 72 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/plan/RelRule.java b/core/src/main/java/org/apache/calcite/plan/RelRule.java index 33388870fc78..b0f1ee717d01 100644 --- a/core/src/main/java/org/apache/calcite/plan/RelRule.java +++ b/core/src/main/java/org/apache/calcite/plan/RelRule.java @@ -144,10 +144,10 @@ default T as(Class 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 diff --git a/core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java b/core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java index 65b61bacac48..6ec4f9f8284d 100644 --- a/core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java +++ b/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 nodePredicate(); + @Nullable Predicate nodePredicate(); } } diff --git a/core/src/main/java/org/apache/calcite/rel/rules/AggregateReduceFunctionsRule.java b/core/src/main/java/org/apache/calcite/rel/rules/AggregateReduceFunctionsRule.java index b058868018cd..054b4b789cee 100644 --- a/core/src/main/java/org/apache/calcite/rel/rules/AggregateReduceFunctionsRule.java +++ b/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 actualFunctionsToReduce() { + default Set actualFunctionsToReduce() { final Set set = Util.first(functionsToReduce(), DEFAULT_FUNCTIONS_TO_REDUCE); set.forEach(AggregateReduceFunctionsRule::validateFunction); diff --git a/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java b/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java index e7c30f6fc58b..4a87ec33be41 100644 --- a/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java +++ b/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 charLiteralStyles(); /** Sets {@link #charLiteralStyles()}. */ Config withCharLiteralStyles(Set charLiteralStyles); - @ImmutableBeans.Property(required = true) + @ImmutableBeans.Property SqlParserImplFactory parserFactory(); /** Sets {@link #parserFactory()}. */ diff --git a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidator.java b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidator.java index a650c17397cb..0f4bb4eb8795 100644 --- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidator.java +++ b/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}. */ diff --git a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java index 7799f9cb66b9..b4d0afc5068d 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java @@ -6153,7 +6153,7 @@ 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()}. */ @@ -6161,7 +6161,7 @@ public interface Config { /** Returns a function that takes a {@link RelBuilder.Config} and returns * another. Default is the identity function. */ - @ImmutableBeans.Property(required = true) + @ImmutableBeans.Property UnaryOperator 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()}. */ diff --git a/core/src/main/java/org/apache/calcite/util/ImmutableBeans.java b/core/src/main/java/org/apache/calcite/util/ImmutableBeans.java index ce915951e7fa..9c6511ab501c 100644 --- a/core/src/main/java/org/apache/calcite/util/ImmutableBeans.java +++ b/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 Def makeDef(Class 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 Def makeDef(Class 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 Def makeDef(Class 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 { @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.METHOD) public @interface Property { - /** Whether the property is required. - * - *

Properties of type {@code int} and {@code boolean} are always - * required. - * - *

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; } diff --git a/core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java b/core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java index a6cee680aee1..0bc7b85d3a64 100644 --- a/core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java +++ b/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> getMaterializations(); @@ -243,8 +245,8 @@ default void noMat() { Sql withQuery(String query); @ImmutableBeans.Property - Function getChecker(); - Sql withChecker(Function checker); + @Nullable Function getChecker(); + Sql withChecker(@Nullable Function checker); @ImmutableBeans.Property AbstractMaterializedViewTest getTester(); diff --git a/core/src/test/java/org/apache/calcite/util/ImmutableBeanTest.java b/core/src/test/java/org/apache/calcite/util/ImmutableBeanTest.java index 4ecb69ac35f6..66819d3e2427 100644 --- a/core/src/test/java/org/apache/calcite/util/ImmutableBeanTest.java +++ b/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 list(); + @Nullable List list(); - CollectionBean withList(List list); + CollectionBean withList(@Nullable List list); @ImmutableBeans.Property(makeImmutable = true) - List immutableList(); + @Nullable List immutableList(); - CollectionBean withImmutableList(List list); + CollectionBean withImmutableList(@Nullable List list); @ImmutableBeans.Property(makeImmutable = false) - Set set(); + @Nullable Set set(); - CollectionBean withSet(Set set); + CollectionBean withSet(@Nullable Set set); @ImmutableBeans.Property(makeImmutable = true) - Set immutableSet(); + @Nullable Set immutableSet(); - CollectionBean withImmutableSet(Set set); + CollectionBean withImmutableSet(@Nullable Set set); @ImmutableBeans.Property(makeImmutable = false) - Map map(); + @Nullable Map map(); - CollectionBean withMap(Map map); + CollectionBean withMap(@Nullable Map map); @ImmutableBeans.Property(makeImmutable = true) - Map immutableMap(); + @Nullable Map immutableMap(); - CollectionBean withImmutableMap(Map map); + CollectionBean withImmutableMap(@Nullable Map map); } }