[CALCITE-4199] review-only: nullability annotations

.editorconfig

@@ -20,6 +20,9 @@ ij_java_use_single_class_imports = true
 max_line_length = 100
 ij_any_wrap_long_lines = true
 
+[*.astub]
+indent_size = 2
+
 [*.java]
 # Doc: https://youtrack.jetbrains.com/issue/IDEA-170643#focus=streamItem-27-3708697.0-0
 # $ means "static"

.github/workflows/main.yml

@@ -175,6 +175,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 :linq4j:classes :core: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
@@ -194,6 +211,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.",
@@ -553,6 +557,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> {
@@ -586,6 +627,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") {

cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java

@@ -37,6 +37,8 @@
 import org.apache.calcite.util.TimestampString;
 import org.apache.calcite.util.Util;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -85,7 +87,7 @@ public CassandraFilter(
     assert getConvention() == child.getConvention();
   }
 
-  @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+  @Override public @Nullable RelOptCost computeSelfCost(RelOptPlanner planner,
       RelMetadataQuery mq) {
     return super.computeSelfCost(planner, mq).multiplyBy(0.1);
   }

cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraLimit.java

@@ -27,6 +27,8 @@
 import org.apache.calcite.rex.RexLiteral;
 import org.apache.calcite.rex.RexNode;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.util.List;
 
 /**
@@ -44,7 +46,7 @@ public CassandraLimit(RelOptCluster cluster, RelTraitSet traitSet,
     assert getConvention() == input.getConvention();
   }
 
-  @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+  @Override public @Nullable RelOptCost computeSelfCost(RelOptPlanner planner,
       RelMetadataQuery mq) {
     // We do this so we get the limit for free
     return planner.getCostFactory().makeZeroCost();

cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraProject.java

@@ -29,6 +29,8 @@
 
 import com.google.common.collect.ImmutableList;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -51,7 +53,7 @@ public CassandraProject(RelOptCluster cluster, RelTraitSet traitSet,
         rowType);
   }
 
-  @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+  @Override public @Nullable RelOptCost computeSelfCost(RelOptPlanner planner,
       RelMetadataQuery mq) {
     return super.computeSelfCost(planner, mq).multiplyBy(0.1);
   }

cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraSort.java

@@ -28,6 +28,8 @@
 import org.apache.calcite.rel.type.RelDataTypeField;
 import org.apache.calcite.rex.RexNode;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.util.ArrayList;
 import java.util.List;
 
@@ -44,7 +46,7 @@ public CassandraSort(RelOptCluster cluster, RelTraitSet traitSet,
     assert getConvention() == child.getConvention();
   }
 
-  @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+  @Override public @Nullable RelOptCost computeSelfCost(RelOptPlanner planner,
       RelMetadataQuery mq) {
     RelOptCost cost = super.computeSelfCost(planner, mq);
     if (!collation.getFieldCollations().isEmpty()) {

cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraToEnumerableConverter.java

@@ -40,6 +40,8 @@
 import org.apache.calcite.util.Pair;
 import org.apache.calcite.util.Util;
 
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.util.AbstractList;
 import java.util.ArrayList;
 import java.util.List;
@@ -63,7 +65,7 @@ protected CassandraToEnumerableConverter(
         getCluster(), traitSet, sole(inputs));
   }
 
-  @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+  @Override public @Nullable RelOptCost computeSelfCost(RelOptPlanner planner,
       RelMetadataQuery mq) {
     return super.computeSelfCost(planner, mq).multiplyBy(.1);
   }

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/codegen/templates/Parser.jj

@@ -269,8 +269,7 @@ void debug_message1() {
 }
 
 JAVACODE String unquotedIdentifier() {
-    return SqlParserUtil.strip(getToken(0).image, null, null, null,
-        unquotedCasing);
+    return SqlParserUtil.toCase(getToken(0).image, unquotedCasing);
 }
 
 /**
@@ -4387,7 +4386,8 @@ SqlNode StringLiteral() :
 |
     <BIG_QUERY_DOUBLE_QUOTED_STRING>
     {
-        p = SqlParserUtil.strip(getToken(0).image, DQ, DQ, "\\\"", Casing.UNCHANGED);
+        p = SqlParserUtil.stripQuotes(getToken(0).image, DQ, DQ, "\\\"",
+            Casing.UNCHANGED);
         try {
             return SqlLiteral.createCharString(p, charSet, getPos());
         } catch (java.nio.charset.UnsupportedCharsetException e) {
@@ -4398,7 +4398,8 @@ SqlNode StringLiteral() :
 |
     <BIG_QUERY_QUOTED_STRING>
     {
-        p = SqlParserUtil.strip(getToken(0).image, "'", "'", "\\'", Casing.UNCHANGED);
+        p = SqlParserUtil.stripQuotes(getToken(0).image, "'", "'", "\\'",
+            Casing.UNCHANGED);
         try {
             return SqlLiteral.createCharString(p, charSet, getPos());
         } catch (java.nio.charset.UnsupportedCharsetException e) {
@@ -4881,19 +4882,19 @@ void IdentifierSegment(List<String> names, List<SqlParserPos> positions) :
         }
     |
         <QUOTED_IDENTIFIER> {
-            id = SqlParserUtil.strip(getToken(0).image, DQ, DQ, DQDQ,
+            id = SqlParserUtil.stripQuotes(getToken(0).image, DQ, DQ, DQDQ,
                 quotedCasing);
             pos = getPos().withQuoting(true);
         }
     |
         <BACK_QUOTED_IDENTIFIER> {
-            id = SqlParserUtil.strip(getToken(0).image, "`", "`", "``",
+            id = SqlParserUtil.stripQuotes(getToken(0).image, "`", "`", "``",
                 quotedCasing);
             pos = getPos().withQuoting(true);
         }
     |
         <BRACKET_QUOTED_IDENTIFIER> {
-            id = SqlParserUtil.strip(getToken(0).image, "[", "]", "]]",
+            id = SqlParserUtil.stripQuotes(getToken(0).image, "[", "]", "]]",
                 quotedCasing);
             pos = getPos().withQuoting(true);
         }
@@ -4902,7 +4903,7 @@ void IdentifierSegment(List<String> names, List<SqlParserPos> positions) :
             span = span();
             String image = getToken(0).image;
             image = image.substring(image.indexOf('"'));
-            image = SqlParserUtil.strip(image, DQ, DQ, DQDQ, quotedCasing);
+            image = SqlParserUtil.stripQuotes(image, DQ, DQ, DQDQ, quotedCasing);
         }
         [
             <UESCAPE> <QUOTED_STRING> {

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 {