Fix remaining spotbugs and checkstyle complaints and enable PR blocking (#137)

* Fix remaining spotbugs issues and enable PR blocking

* Remaining checkstyle complaints

* Fixes after rebasing

Author: jogrogan

build.gradle

@@ -1,24 +1,24 @@
 plugins {
-  id("com.github.spotbugs") version "6.0.26"
+  id("com.github.spotbugs") version "6.1.10"
 }
 
 subprojects {
   apply plugin: 'checkstyle'
   apply plugin: 'com.github.spotbugs'
 
-  tasks.withType(JavaCompile) {
-    options.release = 8
+  tasks.withType(JavaCompile).configureEach {
+    options.release = 11
     options.compilerArgs << '-Xlint:deprecation'
     options.compilerArgs << '-Xlint:unchecked'
   }
-  tasks.withType(Javadoc) {
+  tasks.withType(Javadoc).configureEach {
     options.addStringOption('Xdoclint:none', '-quiet')
   }
 
   spotbugs {
-    ignoreFailures = true
+    ignoreFailures = false
     showProgress = true
-    reportsDir = file("$buildDir/spotbugs")
+    reportsDir = file("$rootProject.layout.buildDirectory/spotbugs")
     includeFilter = file("$rootDir/config/spotbugs/include.xml")
     excludeFilter = file("$rootDir/config/spotbugs/exclude.xml")
   }

config/checkstyle/checkstyle.xml

@@ -8,7 +8,7 @@
 LinkedIn Java style.
 -->
 <module name="Checker">
-  <property name="severity" value="warning"/>
+  <property name="severity" value="error"/>
   <property name="fileExtensions" value="java"/>
 
   <module name="TreeWalker">

config/spotbugs/exclude.xml

@@ -7,4 +7,21 @@
   <Match>
     <Class name="~.*models.*"/>
   </Match>
+
+  <!--
+    Allow a class to hold or return mutable objects. While this has obvious risks, it is much too
+    common a pattern to treat as a bug.
+  -->
+  <Match>
+    <Bug code="EI, EI2"/>
+  </Match>
+
+    <!--
+        Allow CT_CONSTRUCTOR_THROW in PipelineRules. The constructor pattern used by Calcite makes this
+        unavoidable for new rules.
+    -->
+  <Match>
+    <Class name="~.*PipelineRules.*"/>
+    <Bug code="CT"/>
+  </Match>
 </FindBugsFilter>

hoptimator-api/src/main/java/com/linkedin/hoptimator/Source.java

@@ -2,8 +2,6 @@
 
 import java.util.List;
 import java.util.Map;
-import java.util.stream.Collectors;
-
 
 public class Source implements Deployable {
 

hoptimator-api/src/main/java/com/linkedin/hoptimator/Validator.java

@@ -27,7 +27,7 @@ static void validateSubdomainName(String s, Issues issues) {
   }
 
   static <T, U> void validateUnique(Collection<T> collection, Function<T, U> accessor, Issues issues) {
-    Set<U> keys = new HashSet<U>();
+    Set<U> keys = new HashSet<>();
     for (T t : collection) {
       U u = accessor.apply(t);
       if (keys.contains(u)) {
@@ -149,17 +149,13 @@ private String format(int indentLevel) {
         return "";
       }
       StringBuilder sb = new StringBuilder();
-      for (int i = 0; i < indentLevel; i++) {
-        sb.append("  ");
-      }
+      sb.append("  ".repeat(Math.max(0, indentLevel)));
       if (context != null && !context.isEmpty()) {
         sb.append(context);
         sb.append(":\n");
       }
       for (String s : issues) {
-        for (int i = 0; i < indentLevel; i++) {
-          sb.append("  ");
-        }
+        sb.append("  ".repeat(Math.max(0, indentLevel)));
         sb.append("- ");
         sb.append(s);
         sb.append("\n");

hoptimator-api/src/main/java/com/linkedin/hoptimator/View.java

@@ -1,7 +1,6 @@
 package com.linkedin.hoptimator;
 
 import java.util.List;
-import java.util.stream.Collectors;
 
 
 public class View implements Deployable {

hoptimator-avro/src/main/java/com/linkedin/hoptimator/avro/AvroConverter.java

@@ -2,6 +2,8 @@
 
 import java.util.AbstractMap;
 import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import org.apache.avro.Schema;
@@ -44,10 +46,10 @@ public static Schema avro(String namespace, String name, RelDataType dataType) {
         case BOOLEAN:
           return createAvroTypeWithNullability(Schema.Type.BOOLEAN, dataType.isNullable());
         case ARRAY:
-          return createAvroSchemaWithNullability(Schema.createArray(avro(null, null, dataType.getComponentType())),
+          return createAvroSchemaWithNullability(Schema.createArray(avro(null, null, Objects.requireNonNull(dataType.getComponentType()))),
               dataType.isNullable());
         case MAP:
-          return createAvroSchemaWithNullability(Schema.createMap(avro(null, null, dataType.getValueType())),
+          return createAvroSchemaWithNullability(Schema.createMap(avro(null, null, Objects.requireNonNull(dataType.getValueType()))),
               dataType.isNullable());
         case UNKNOWN:
         case NULL:
@@ -118,8 +120,11 @@ public static RelDataType rel(Schema schema, RelDataTypeFactory typeFactory, boo
       case UNION:
         boolean isNullable = schema.isNullable();
         if (schema.isNullable() && schema.getTypes().size() == 2) {
-          Schema innerType = schema.getTypes().stream().filter(x -> x.getType() != Schema.Type.NULL).findFirst().get();
-          return typeFactory.createTypeWithNullability(rel(innerType, typeFactory, true), true);
+          Optional<Schema> innerType = schema.getTypes().stream().filter(x -> x.getType() != Schema.Type.NULL).findFirst();
+          if (innerType.isEmpty()) {
+            throw new IllegalArgumentException("Union schema must contain at least one non-null type");
+          }
+          return typeFactory.createTypeWithNullability(rel(innerType.get(), typeFactory, true), true);
         }
         // Since we collapse complex unions into separate fields, each of these fields needs to be nullable
         // as only one of the group will be present in any given record.

hoptimator-avro/src/main/java/com/linkedin/hoptimator/avro/AvroTableValidator.java

@@ -57,9 +57,9 @@ private void validate(SchemaPlus schema, Table table, Table originalTable, Issue
     RelDataType originalRowType = originalTable.getRowType(typeFactory);
     Schema avroSchema = AvroConverter.avro("ns", "n", rowType);
     Schema originalAvroSchema = AvroConverter.avro("ns", "n", originalRowType);
-    DatumWriter<Object> datumWriter = new GenericDatumWriter<Object>(originalAvroSchema);
+    DatumWriter<Object> datumWriter = new GenericDatumWriter<>(originalAvroSchema);
     try (OutputStream out = new ByteArrayOutputStream();
-        DataFileWriter<Object> dataFileWriter = new DataFileWriter<Object>(datumWriter)) {
+        DataFileWriter<Object> dataFileWriter = new DataFileWriter<>(datumWriter)) {
       dataFileWriter.create(originalAvroSchema, out);
       for (Object obj : new RandomData(avroSchema, 1)) {
         dataFileWriter.append(obj);

hoptimator-avro/src/test/java/com/linkedin/hoptimator/avro/AvroConverterTest.java

@@ -1,5 +1,7 @@
 package com.linkedin.hoptimator.avro;
 
+import java.util.Objects;
+
 import org.apache.avro.Schema;
 import org.apache.calcite.plan.RelOptUtil;
 import org.apache.calcite.rel.type.RelDataType;
@@ -16,21 +18,21 @@ public class AvroConverterTest {
 
   @Test
   public void convertsNestedSchemas() {
-    // CHECKSTYLE:OFF
     String schemaString =
-        "{\"type\":\"record\",\"name\":\"E\",\"namespace\":\"ns\",\"fields\":[{\"name\":\"h\",\"type\":[\"null\",{\"type\":\"record\",\"name\":\"H\",\"namespace\":\"ns\",\"fields\":[{\"name\":\"A\",\"type\":[\"null\",{\"type\":\"record\",\"name\":\"A\",\"fields\":[]}]}]}]}]}";
-    // CHECKSTYLE:ON
+        "{\"type\":\"record\",\"name\":\"E\",\"namespace\":\"ns\",\"fields\":["
+            + "{\"name\":\"h\",\"type\":[\"null\",{\"type\":\"record\",\"name\":\"H\",\"namespace\":\"ns\",\"fields\":["
+            + "{\"name\":\"A\",\"type\":[\"null\",{\"type\":\"record\",\"name\":\"A\",\"fields\":[]}]}]}]}]}";
 
     Schema avroSchema1 = (new Schema.Parser()).parse(schemaString);
     RelDataType rel1 = AvroConverter.rel(avroSchema1);
     assertEquals(rel1.toString(), rel1.getFieldCount(), avroSchema1.getFields().size());
     assertNotNull(rel1.toString(), rel1.getField("h", false, false));
-    RelDataType rel2 = rel1.getField("h", false, false).getType();
+    RelDataType rel2 = Objects.requireNonNull(rel1.getField("h", false, false)).getType();
     assertTrue(rel2.toString(), rel2.isNullable());
     Schema avroSchema2 = avroSchema1.getField("h").schema().getTypes().get(1);
     assertEquals(rel2.toString(), rel2.getFieldCount(), avroSchema2.getFields().size());
     assertNotNull(rel2.toString(), rel2.getField("A", false, false));
-    RelDataType rel3 = rel2.getField("A", false, false).getType();
+    RelDataType rel3 = Objects.requireNonNull(rel2.getField("A", false, false)).getType();
     assertTrue(rel3.toString(), rel3.isNullable());
     Schema avroSchema3 = avroSchema2.getField("A").schema().getTypes().get(1);
     assertEquals(rel3.toString(), rel3.getFieldCount(), avroSchema3.getFields().size());
@@ -48,16 +50,21 @@ public void convertsNestedSchemas() {
 
   @Test
   public void convertsNestedUnionSchemas() {
-    String schemaString = "{\"type\":\"record\",\"name\":\"record\",\"namespace\":\"ns\",\"fields\":[{\"name\":\"event\",\"type\":[{\"type\":\"record\",\"name\":\"record_event1\",\"fields\":[{\"name\":\"strField\",\"type\":\"string\"}]},{\"type\":\"record\",\"name\":\"record_event2\",\"fields\":[{\"name\":\"strField\",\"type\":\"string\"}]}]}]}";
+    String schemaString =
+        "{\"type\":\"record\",\"name\":\"record\",\"namespace\":\"ns\",\"fields\":["
+            + "{\"name\":\"event\",\"type\":[{\"type\":\"record\",\"name\":\"record_event1\",\"fields\":["
+            + "{\"name\":\"strField\",\"type\":\"string\"}]},{\"type\":\"record\",\"name\":\"record_event2\",\"fields\":["
+            + "{\"name\":\"strField\",\"type\":\"string\"}]}]}]}";
+
     Schema avroSchema1 = (new Schema.Parser()).parse(schemaString);
     RelDataType rel1 = AvroConverter.rel(avroSchema1);
     assertEquals(rel1.toString(), rel1.getFieldCount(), avroSchema1.getFields().size());
     assertNotNull(rel1.toString(), rel1.getField("event", false, false));
-    RelDataType rel2 = rel1.getField("event", false, false).getType();
+    RelDataType rel2 = Objects.requireNonNull(rel1.getField("event", false, false)).getType();
     assertTrue(rel2.isStruct());
     Schema avroSchema2 = avroSchema1.getField("event").schema();
     assertEquals(rel2.toString(), rel2.getFieldCount(), avroSchema2.getTypes().size());
-    RelDataType rel3 = rel2.getField("record_event1", false, false).getType();
+    RelDataType rel3 = Objects.requireNonNull(rel2.getField("record_event1", false, false)).getType();
     Schema avroSchema3 = avroSchema2.getTypes().get(0);
     assertEquals(rel3.toString(), rel3.getFieldCount(), avroSchema3.getFields().size());
     Schema avroSchema4 = AvroConverter.avro("NS", "R", rel1);

hoptimator-catalog/src/main/java/com/linkedin/hoptimator/catalog/DatabaseSchema.java

@@ -12,10 +12,15 @@ public class DatabaseSchema extends AbstractSchema {
   private final Database database;
   private final Map<String, Table> tableMap;
 
-  public DatabaseSchema(Database database) {
+  public DatabaseSchema(Database database, Map<String, Table> tableMap) {
     this.database = database;
+    this.tableMap = tableMap;
+  }
+
+  public static DatabaseSchema create(Database database) {
     try {
-      this.tableMap = database.tables().stream().collect(Collectors.toMap(x -> x, x -> new ProtoTable(x, database)));
+      Map<String, Table> tableMap = database.tables().stream().collect(Collectors.toMap(x -> x, x -> new ProtoTable(x, database)));
+      return new DatabaseSchema(database, tableMap);
     } catch (Exception e) {
       throw new RuntimeException(e);
     }