[SPARK-24529][BUILD][TEST-MAVEN] Add spotbugs into maven build process
authorKazuaki Ishizaki <ishizaki@jp.ibm.com>
Thu, 12 Jul 2018 01:52:23 +0000 (09:52 +0800)
committerhyukjinkwon <gurwls223@apache.org>
Thu, 12 Jul 2018 01:52:23 +0000 (09:52 +0800)
## What changes were proposed in this pull request?

This PR enables a Java bytecode check tool [spotbugs](https://spotbugs.github.io/) to avoid possible integer overflow at multiplication. When an violation is detected, the build process is stopped.
Due to the tool limitation, some other checks will be enabled. In this PR, [these patterns](http://spotbugs-in-kengo-toda.readthedocs.io/en/lqc-list-detectors/detectors.html#findpuzzlers) in `FindPuzzlers` can be detected.

This check is enabled at `compile` phase. Thus, `mvn compile` or `mvn package` launches this check.

## How was this patch tested?

Existing UTs

Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com>

Closes #21542 from kiszk/SPARK-24529.

core/src/main/scala/org/apache/spark/util/collection/ExternalSorter.scala
mllib/src/main/scala/org/apache/spark/ml/image/HadoopUtils.scala
pom.xml
resource-managers/kubernetes/core/pom.xml
resource-managers/kubernetes/integration-tests/pom.xml
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

index 176f84f..b159200 100644 (file)
@@ -368,8 +368,8 @@ private[spark] class ExternalSorter[K, V, C](
     val bufferedIters = iterators.filter(_.hasNext).map(_.buffered)
     type Iter = BufferedIterator[Product2[K, C]]
     val heap = new mutable.PriorityQueue[Iter]()(new Ordering[Iter] {
-      // Use the reverse of comparator.compare because PriorityQueue dequeues the max
-      override def compare(x: Iter, y: Iter): Int = -comparator.compare(x.head._1, y.head._1)
+      // Use the reverse order because PriorityQueue dequeues the max
+      override def compare(x: Iter, y: Iter): Int = comparator.compare(y.head._1, x.head._1)
     })
     heap.enqueue(bufferedIters: _*)  // Will contain only the iterators with hasNext = true
     new Iterator[Product2[K, C]] {
index 8c975a2..f1579ec 100644 (file)
@@ -42,9 +42,11 @@ private object RecursiveFlag {
     val old = Option(hadoopConf.get(flagName))
     hadoopConf.set(flagName, value.toString)
     try f finally {
-      old match {
-        case Some(v) => hadoopConf.set(flagName, v)
-        case None => hadoopConf.unset(flagName)
+      // avoid false positive of DLS_DEAD_LOCAL_STORE_IN_RETURN by SpotBugs
+      if (old.isDefined) {
+        hadoopConf.set(flagName, old.get)
+      } else {
+        hadoopConf.unset(flagName)
       }
     }
   }
diff --git a/pom.xml b/pom.xml
index cd567e2..6dee6fc 100644 (file)
--- a/pom.xml
+++ b/pom.xml
           </execution>
         </executions>
       </plugin>
+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <version>3.1.3</version>
+        <configuration>
+          <classFilesDirectory>${basedir}/target/scala-${scala.binary.version}/classes</classFilesDirectory>
+          <testClassFilesDirectory>${basedir}/target/scala-${scala.binary.version}/test-classes</testClassFilesDirectory>
+          <effort>Max</effort>
+          <threshold>Low</threshold>
+          <xmlOutput>true</xmlOutput>
+          <visitors>FindPuzzlers</visitors>
+          <fork>false</fork>
+        </configuration>
+        <executions>
+          <execution>
+            <goals>
+              <goal>check</goal>
+            </goals>
+            <phase>compile</phase>
+          </execution>
+        </executions>
+      </plugin>
     </plugins>
   </build>
 
index a6dd47a..920f0f6 100644 (file)
     </dependency>
 
     <dependency>
+      <groupId>org.apache.spark</groupId>
+      <artifactId>spark-tags_${scala.binary.version}</artifactId>
+      <type>test-jar</type>
+    </dependency>
+
+    <dependency>
       <groupId>io.fabric8</groupId>
       <artifactId>kubernetes-client</artifactId>
       <version>${kubernetes.client.version}</version>
index 6a2fff8..29334cc 100644 (file)
       <artifactId>kubernetes-client</artifactId>
       <version>${kubernetes-client.version}</version>
     </dependency>
+    <dependency>
+      <groupId>org.apache.spark</groupId>
+      <artifactId>spark-tags_${scala.binary.version}</artifactId>
+      <type>test-jar</type>
+    </dependency>
   </dependencies>
 
   <build>
index e217d37..b8f2aa3 100644 (file)
@@ -993,7 +993,7 @@ trait ArraySortLike extends ExpectsInputTypes {
         } else if (o2 == null) {
           nullOrder
         } else {
-          -ordering.compare(o1, o2)
+          ordering.compare(o2, o1)
         }
       }
     }
index e6377b7..30ce9e4 100644 (file)
@@ -294,7 +294,7 @@ object CaseWhen {
       case cond :: value :: Nil => Some((cond, value))
       case value :: Nil => None
     }.toArray.toSeq  // force materialization to make the seq serializable
-    val elseValue = if (branches.size % 2 == 1) Some(branches.last) else None
+    val elseValue = if (branches.size % 2 != 0) Some(branches.last) else None
     CaseWhen(cases, elseValue)
   }
 }
@@ -309,7 +309,7 @@ object CaseKeyWhen {
       case Seq(cond, value) => Some((EqualTo(key, cond), value))
       case Seq(value) => None
     }.toArray.toSeq  // force materialization to make the seq serializable
-    val elseValue = if (branches.size % 2 == 1) Some(branches.last) else None
+    val elseValue = if (branches.size % 2 != 0) Some(branches.last) else None
     CaseWhen(cases, elseValue)
   }
 }
index 383ebde..f398b47 100644 (file)
@@ -1507,7 +1507,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
         case "TIMESTAMP" =>
           Literal(Timestamp.valueOf(value))
         case "X" =>
-          val padding = if (value.length % 2 == 1) "0" else ""
+          val padding = if (value.length % 2 != 0) "0" else ""
           Literal(DatatypeConverter.parseHexBinary(padding + value))
         case other =>
           throw new ParseException(s"Literals of type '$other' are currently not supported.", ctx)