[CALCITE-4199] Add nullability annotations to the methods and fields, ensure consistency with checkerframework

.github/workflows/main.yml

@@ -147,6 +147,23 @@ jobs:
           job-id: errprone
           arguments: --scan --no-parallel --no-daemon -PenableErrorprone classes
 
+  linux-checkerframework:
+    name: 'CheckerFramework (JDK 11)'
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          fetch-depth: 50
+      - name: 'Set up JDK 11'
+        uses: actions/setup-java@v1
+        with:
+          java-version: 11
+      - name: 'Run CheckerFramework'
+        uses: burrunan/gradle-cache-action@v1
+        with:
+          job-id: checkerframework-jdk11
+          arguments: --scan --no-parallel --no-daemon -PenableCheckerframework classes
+
   linux-slow:
     # Run slow tests when the commit is on master or it is requested explicitly by adding an
     # appropriate label in the PR
@@ -166,6 +183,7 @@ jobs:
         with:
           job-id: jdk8
           arguments: --scan --no-parallel --no-daemon testSlow
+
   linux-druid:
     if: github.event.action != 'labeled'
     name: 'Linux (JDK 8) Druid Tests'

bom/build.gradle.kts

@@ -53,12 +53,14 @@ dependencies {
         // dependency on it during compilation
         apiv("com.alibaba.database:innodb-java-reader")
         apiv("com.beust:jcommander")
+        apiv("org.checkerframework:checker-qual", "checkerframework")
         apiv("com.datastax.cassandra:cassandra-driver-core")
         apiv("com.esri.geometry:esri-geometry-api")
         apiv("com.fasterxml.jackson.core:jackson-databind")
         apiv("com.github.kstyrc:embedded-redis")
         apiv("com.github.stephenc.jcip:jcip-annotations")
-        apiv("com.google.code.findbugs:jsr305", "findbugs.jsr305")
+        apiv("com.google.errorprone:error_prone_annotations", "errorprone")
+        apiv("com.google.errorprone:error_prone_type_annotations", "errorprone")
         apiv("com.google.guava:guava")
         apiv("com.google.protobuf:protobuf-java", "protobuf")
         apiv("com.google.uzaygezen:uzaygezen-core", "uzaygezen")

build.gradle.kts

@@ -37,6 +37,7 @@ plugins {
     // Verification
     checkstyle
     calcite.buildext
+    id("org.checkerframework") apply false
     id("com.github.autostyle")
     id("org.nosphere.apache.rat")
     id("com.github.spotbugs")
@@ -65,6 +66,7 @@ val lastEditYear by extra(lastEditYear())
 
 // Do not enable spotbugs by default. Execute it only when -Pspotbugs is present
 val enableSpotBugs = props.bool("spotbugs")
+val enableCheckerframework by props()
 val enableErrorprone by props()
 val skipCheckstyle by props()
 val skipAutostyle by props()
@@ -454,6 +456,8 @@ allprojects {
                         replace("junit5: Assert.fail", "org.junit.Assert.fail", "org.junit.jupiter.api.Assertions.fail")
                     }
                     replaceRegex("side by side comments", "(\n\\s*+[*]*+/\n)(/[/*])", "\$1\n\$2")
+                    replaceRegex("jsr305 nullable -> checkerframework", "javax\\.annotation\\.Nullable", "org.checkerframework.checker.nullness.qual.Nullable")
+                    replaceRegex("jsr305 nonnull -> checkerframework", "javax\\.annotation\\.Nonnull", "org.checkerframework.checker.nullness.qual.NonNull")
                     importOrder(
                         "org.apache.calcite.",
                         "org.apache.",
@@ -552,6 +556,43 @@ allprojects {
                 }
             }
         }
+        if (enableCheckerframework) {
+            apply(plugin = "org.checkerframework")
+            dependencies {
+                "checkerFramework"("org.checkerframework:checker:${"checkerframework".v}")
+                // CheckerFramework annotations might be used in the code as follows:
+                // dependencies {
+                //     "compileOnly"("org.checkerframework:checker-qual")
+                //     "testCompileOnly"("org.checkerframework:checker-qual")
+                // }
+                if (JavaVersion.current() == JavaVersion.VERSION_1_8) {
+                    // only needed for JDK 8
+                    "checkerFrameworkAnnotatedJDK"("org.checkerframework:jdk8")
+                }
+            }
+            configure<org.checkerframework.gradle.plugin.CheckerFrameworkExtension> {
+                skipVersionCheck = true
+                // See https://checkerframework.org/manual/#introduction
+                checkers.add("org.checkerframework.checker.nullness.NullnessChecker")
+                // Below checkers take significant time and they do not provide much value :-/
+                // checkers.add("org.checkerframework.checker.optional.OptionalChecker")
+                // checkers.add("org.checkerframework.checker.regex.RegexChecker")
+                // https://checkerframework.org/manual/#creating-debugging-options-progress
+                // extraJavacArgs.add("-Afilenames")
+                extraJavacArgs.addAll(listOf("-Xmaxerrs", "10000"))
+                // Consider Java assert statements for nullness and other checks
+                extraJavacArgs.add("-AassumeAssertionsAreEnabled")
+                // https://checkerframework.org/manual/#stub-using
+                extraJavacArgs.add("-Astubs=" +
+                        fileTree("$rootDir/src/main/config/checkerframework") {
+                            include("**/*.astub")
+                        }.asPath
+                )
+                if (project.path == ":core") {
+                    extraJavacArgs.add("-AskipDefs=^org\\.apache\\.calcite\\.sql\\.parser\\.impl\\.")
+                }
+            }
+        }
 
         tasks {
             configureEach<Jar> {
@@ -585,6 +626,9 @@ allprojects {
                 if (werror) {
                     options.compilerArgs.add("-Werror")
                 }
+                if (enableCheckerframework) {
+                    options.forkOptions.memoryMaximumSize = "2g"
+                }
             }
             configureEach<Test> {
                 outputs.cacheIf("test results depend on the database configuration, so we souldn't cache it") {

core/build.gradle.kts

@@ -47,7 +47,7 @@ dependencies {
     implementation("com.fasterxml.jackson.core:jackson-core")
     implementation("com.fasterxml.jackson.core:jackson-databind")
     implementation("com.fasterxml.jackson.dataformat:jackson-dataformat-yaml")
-    implementation("com.google.code.findbugs:jsr305"/* optional*/)
+    implementation("com.google.errorprone:error_prone_annotations")
     implementation("com.google.guava:guava")
     implementation("com.google.uzaygezen:uzaygezen-core")
     implementation("com.jayway.jsonpath:json-path")
@@ -56,6 +56,7 @@ dependencies {
     implementation("net.hydromatic:aggdesigner-algorithm")
     implementation("org.apache.commons:commons-dbcp2")
     implementation("org.apache.commons:commons-lang3")
+    implementation("org.checkerframework:checker-qual")
     implementation("commons-io:commons-io")
     implementation("org.codehaus.janino:commons-compiler")
     implementation("org.codehaus.janino:janino")

core/src/main/java/org/apache/calcite/DataContext.java

@@ -25,6 +25,8 @@
 
 import com.google.common.base.CaseFormat;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.lang.reflect.Modifier;
@@ -42,17 +44,17 @@ public interface DataContext {
   /**
    * Returns a sub-schema with a given name, or null.
    */
-  SchemaPlus getRootSchema();
+  @Nullable SchemaPlus getRootSchema();
 
   /**
    * Returns the type factory.
    */
-  JavaTypeFactory getTypeFactory();
+  @Nullable JavaTypeFactory getTypeFactory();
 
   /**
    * Returns the query provider.
    */
-  QueryProvider getQueryProvider();
+  @Nullable QueryProvider getQueryProvider();
 
   /**
    * Returns a context variable.
@@ -62,7 +64,7 @@ public interface DataContext {
    *
    * @param name Name of variable
    */
-  Object get(String name);
+  @Nullable Object get(String name);
 
   /** Variable that may be asked for in a call to {@link DataContext#get}. */
   enum Variable {

core/src/main/java/org/apache/calcite/adapter/clone/ArrayTable.java

@@ -41,6 +41,8 @@
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.lang.reflect.Array;
 import java.lang.reflect.Type;
 import java.util.AbstractList;
@@ -82,9 +84,9 @@ class ArrayTable extends AbstractQueryableTable implements ScannableTable {
     return Statistics.of(content.size, keys, content.collations);
   }
 
-  @Override public Enumerable<Object[]> scan(DataContext root) {
-    return new AbstractEnumerable<Object[]>() {
-      @Override public Enumerator<Object[]> enumerator() {
+  @Override public Enumerable<@Nullable Object[]> scan(DataContext root) {
+    return new AbstractEnumerable<@Nullable Object[]>() {
+      @Override public Enumerator<@Nullable Object[]> enumerator() {
         final Content content = supplier.get();
         return content.arrayEnumerator();
       }
@@ -225,7 +227,7 @@ public static List asList(final Representation representation,
       // Cache size. It might be expensive to compute.
       final int size = representation.size(dataSet);
       return new AbstractList() {
-        @Override public Object get(int index) {
+        @Override public @Nullable Object get(int index) {
           return representation.getObject(dataSet, index);
         }
 
@@ -243,9 +245,9 @@ public interface Representation {
 
     /** Converts a value set into a compact representation. If
      * {@code sources} is not null, permutes. */
-    Object freeze(ColumnLoader.ValueSet valueSet, int[] sources);
+    Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources);
 
-    Object getObject(Object dataSet, int ordinal);
+    @Nullable Object getObject(Object dataSet, int ordinal);
     int getInt(Object dataSet, int ordinal);
 
     /** Creates a data set that is the same as a given data set
@@ -277,7 +279,7 @@ public static class ObjectArray implements Representation {
       return RepresentationType.OBJECT_ARRAY;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       // We assume the values have been canonized.
       final List<Comparable> list = permuteList(valueSet.values, sources);
       return list.toArray(new Comparable[0]);
@@ -334,7 +336,7 @@ public static class PrimitiveArray implements Representation {
       return RepresentationType.PRIMITIVE_ARRAY;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       //noinspection unchecked
       return primitive.toArray2(
           permuteList((List) valueSet.values, sources));
@@ -344,7 +346,7 @@ public static class PrimitiveArray implements Representation {
       return primitive.permute(dataSet, sources);
     }
 
-    @Override public Object getObject(Object dataSet, int ordinal) {
+    @Override public @Nullable Object getObject(Object dataSet, int ordinal) {
       return p.arrayItem(dataSet, ordinal);
     }
 
@@ -375,7 +377,7 @@ public static class PrimitiveDictionary implements Representation {
       return RepresentationType.PRIMITIVE_DICTIONARY;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       throw new UnsupportedOperationException(); // TODO:
     }
 
@@ -423,10 +425,10 @@ public static class ObjectDictionary implements Representation {
       return RepresentationType.OBJECT_DICTIONARY;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       final int n = valueSet.map.keySet().size();
       int extra = valueSet.containsNull ? 1 : 0;
-      Comparable[] codeValues =
+      @Nullable Comparable[] codeValues =
           valueSet.map.keySet().toArray(new Comparable[n + extra]);
       Arrays.sort(codeValues, 0, n);
       ColumnLoader.ValueSet codeValueSet =
@@ -486,7 +488,7 @@ public static class StringDictionary implements Representation {
       return RepresentationType.STRING_DICTIONARY;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       throw new UnsupportedOperationException(); // TODO:
     }
 
@@ -524,7 +526,7 @@ public static class ByteStringDictionary implements Representation {
       return RepresentationType.BYTE_STRING_DICTIONARY;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       throw new UnsupportedOperationException(); // TODO:
     }
 
@@ -565,7 +567,7 @@ public static class Constant implements Representation {
       return RepresentationType.CONSTANT;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       final int size = valueSet.values.size();
       return Pair.of(size == 0 ? null : valueSet.values.get(0), size);
     }
@@ -626,7 +628,7 @@ public static class BitSlicedPrimitiveArray implements Representation {
       return RepresentationType.BIT_SLICED_PRIMITIVE_ARRAY;
     }
 
-    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int[] sources) {
+    @Override public Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources) {
       final int chunksPerWord = 64 / bitCount;
       final List<Comparable> valueList =
           permuteList(valueSet.values, sources);
@@ -783,7 +785,7 @@ public static void orLong(
   }
 
   private static <E> List<E> permuteList(
-      final List<E> list, final int[] sources) {
+      final List<E> list, final int @Nullable [] sources) {
     if (sources == null) {
       return list;
     }
@@ -828,13 +830,13 @@ public <T> Enumerator<T> enumerator() {
       }
     }
 
-    public Enumerator<Object[]> arrayEnumerator() {
+    public Enumerator<@Nullable Object[]> arrayEnumerator() {
       return new ArrayEnumerator(size, columns);
     }
 
     /** Enumerator over a table with a single column; each element
      * returned is an object. */
-    private static class ObjectEnumerator implements Enumerator<Object> {
+    private static class ObjectEnumerator implements Enumerator<@Nullable Object> {
       final int rowCount;
       final Object dataSet;
       final Representation representation;
@@ -846,7 +848,7 @@ private static class ObjectEnumerator implements Enumerator<Object> {
         this.representation = column.representation;
       }
 
-      @Override public Object current() {
+      @Override public @Nullable Object current() {
         return representation.getObject(dataSet, i);
       }
 
@@ -864,7 +866,7 @@ private static class ObjectEnumerator implements Enumerator<Object> {
 
     /** Enumerator over a table with more than one column; each element
      * returned is an array. */
-    private static class ArrayEnumerator implements Enumerator<Object[]> {
+    private static class ArrayEnumerator implements Enumerator<@Nullable Object[]> {
       final int rowCount;
       final List<Column> columns;
       int i = -1;
@@ -874,8 +876,8 @@ private static class ArrayEnumerator implements Enumerator<Object[]> {
         this.columns = columns;
       }
 
-      @Override public Object[] current() {
-        Object[] objects = new Object[columns.size()];
+      @Override public @Nullable Object[] current() {
+        @Nullable Object[] objects = new Object[columns.size()];
         for (int j = 0; j < objects.length; j++) {
           final Column pair = columns.get(j);
           objects[j] = pair.representation.getObject(pair.dataSet, i);

core/src/main/java/org/apache/calcite/adapter/clone/CloneSchema.java

@@ -37,6 +37,8 @@
 import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableList;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.lang.reflect.Type;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -90,15 +92,15 @@ private Table createCloneTable(QueryProvider queryProvider,
   @Deprecated // to be removed before 2.0
   public static <T> Table createCloneTable(final JavaTypeFactory typeFactory,
       final RelProtoDataType protoRowType,
-      final List<ColumnMetaData.Rep> repList,
+      final @Nullable List<ColumnMetaData.Rep> repList,
       final Enumerable<T> source) {
     return createCloneTable(typeFactory, protoRowType, ImmutableList.of(),
         repList, source);
   }
 
   public static <T> Table createCloneTable(final JavaTypeFactory typeFactory,
       final RelProtoDataType protoRowType, final List<RelCollation> collations,
-      final List<ColumnMetaData.Rep> repList, final Enumerable<T> source) {
+      final @Nullable List<ColumnMetaData.Rep> repList, final Enumerable<T> source) {
     final Type elementType;
     if (source instanceof QueryableTable) {
       elementType = ((QueryableTable) source).getElementType();