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);
     }

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

@@ -3,13 +3,15 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Properties;
 import java.util.Scanner;
 import java.util.Set;
@@ -129,6 +131,20 @@ public int hashCode() {
     return toString().hashCode();
   }
 
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+    Resource resource = (Resource) o;
+    return Objects.equals(template, resource.template)
+        && Objects.equals(properties, resource.properties)
+        && Objects.equals(inputs, resource.inputs);
+  }
+
   @Override
   public String toString() {
     StringBuilder sb = new StringBuilder();
@@ -305,7 +321,7 @@ public SimpleTemplate(Environment env, String template) {
 
     @Override
     public String render(Resource resource) {
-      StringBuffer sb = new StringBuffer();
+      StringBuilder sb = new StringBuilder();
       Pattern p =
           Pattern.compile("([\\s\\-\\#]*)\\{\\{\\s*([\\w_\\-\\.]+)\\s*(:([\\w_\\-\\.]+))?\\s*((\\w+\\s*)*)\\s*\\}\\}");
       Matcher m = p.matcher(template);
@@ -318,9 +334,6 @@ public String render(Resource resource) {
         String defaultValue = m.group(4);
         String transform = m.group(5);
         String value = resource.getOrDefault(key, () -> env.getOrDefault(key, () -> defaultValue));
-        if (value == null) {
-          throw new IllegalArgumentException(template + " has no value for key " + key + ".");
-        }
         String transformedValue = applyTransform(value, transform);
         String quotedPrefix = Matcher.quoteReplacement(prefix);
         String quotedValue = Matcher.quoteReplacement(transformedValue);
@@ -387,7 +400,7 @@ public Collection<Template> find(Resource resource) throws IOException {
           e.hasMoreElements(); ) {
         InputStream in = e.nextElement().openStream();
         StringBuilder sb = new StringBuilder();
-        Scanner scanner = new Scanner(in);
+        Scanner scanner = new Scanner(in, StandardCharsets.UTF_8);
         scanner.useDelimiter("\n");
         while (scanner.hasNext()) {
           sb.append(scanner.next());

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

@@ -4,6 +4,7 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
 
 import org.apache.calcite.plan.RelOptUtil;
@@ -128,26 +129,11 @@ public void implement(SqlWriter w) {
   class QueryImplementor implements ScriptImplementor {
     private final RelNode relNode;
 
-    public QueryImplementor(RelNode relNode) {
-      this.relNode = relNode;
-    }
-
-    @Override
-    public void implement(SqlWriter w) {
-      RelToSqlConverter converter = new RelToSqlConverter(w.getDialect());
-      SqlImplementor.Result result = converter.visitRoot(relNode);
-      SqlSelect select = result.asSelect();
-      if (select.getSelectList() != null) {
-        select.setSelectList((SqlNodeList) select.getSelectList().accept(REMOVE_ROW_CONSTRUCTOR));
-      }
-      w.literal(select.toSqlString(w.getDialect()).getSql());
-    }
-
     // A `ROW(...)` operator which will unparse as just `(...)`.
-    private final SqlRowOperator IMPLIED_ROW_OPERATOR = new SqlRowOperator(""); // empty string name
+    private static final SqlRowOperator IMPLIED_ROW_OPERATOR = new SqlRowOperator(""); // empty string name
 
     // a shuttle that replaces `Row(...)` with just `(...)`
-    private final SqlShuttle REMOVE_ROW_CONSTRUCTOR = new SqlShuttle() {
+    private static final SqlShuttle REMOVE_ROW_CONSTRUCTOR = new SqlShuttle() {
       @Override
       public SqlNode visit(SqlCall call) {
         List<SqlNode> operands = call.getOperandList().stream().map(x -> x.accept(this)).collect(Collectors.toList());
@@ -159,6 +145,21 @@ public SqlNode visit(SqlCall call) {
         }
       }
     };
+
+    public QueryImplementor(RelNode relNode) {
+      this.relNode = relNode;
+    }
+
+    @Override
+    public void implement(SqlWriter w) {
+      RelToSqlConverter converter = new RelToSqlConverter(w.getDialect());
+      SqlImplementor.Result result = converter.visitRoot(relNode);
+      SqlSelect select = result.asSelect();
+      if (select.getSelectList() != null) {
+        select.setSelectList((SqlNodeList) Objects.requireNonNull(select.getSelectList().accept(REMOVE_ROW_CONSTRUCTOR)));
+      }
+      w.literal(select.toSqlString(w.getDialect()).getSql());
+    }
   }
 
   /**
@@ -297,7 +298,7 @@ public CompoundIdentifierImplementor(String database, String name) {
 
     @Override
     public void implement(SqlWriter w) {
-      SqlIdentifier identifier = new SqlIdentifier(Arrays.asList(new String[]{database, name}), SqlParserPos.ZERO);
+      SqlIdentifier identifier = new SqlIdentifier(Arrays.asList(database, name), SqlParserPos.ZERO);
       identifier.unparse(w, 0, 0);
     }
   }
@@ -346,9 +347,10 @@ private static SqlDataTypeSpec toSpec(RelDataType dataType) {
         return maybeNullable(dataType,
             new SqlDataTypeSpec(new SqlRowTypeNameSpec(SqlParserPos.ZERO, fieldNames, fieldTypes), SqlParserPos.ZERO));
       }
-      if (dataType.getComponentType() != null) {
+      RelDataType componentType = dataType.getComponentType();
+      if (componentType != null) {
         return maybeNullable(dataType, new SqlDataTypeSpec(new SqlCollectionTypeNameSpec(
-            new SqlBasicTypeNameSpec(dataType.getComponentType().getSqlTypeName(), SqlParserPos.ZERO),
+            new SqlBasicTypeNameSpec(componentType.getSqlTypeName(), SqlParserPos.ZERO),
             dataType.getSqlTypeName(), SqlParserPos.ZERO), SqlParserPos.ZERO));
       } else {
         return maybeNullable(dataType,

hoptimator-catalog/src/main/java/com/linkedin/hoptimator/catalog/builtin/DatagenSchemaFactory.java

@@ -44,6 +44,6 @@ public Schema create(SchemaPlus parentSchema, String name, Map<String, Object> o
         .with("fields.NAME.length", "5")
         .with("fields.CEO.length", "5")
         .config("COMPANY")));
-    return new DatabaseSchema(new Database(name, datagenTables));
+    return DatabaseSchema.create(new Database(name, datagenTables));
   }
 }