Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ private[spark] class ExternalSorter[K, V, C](
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we keep this comment? we still use reverse order here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe: Use the reverse order because PriorityQueue dequeues the max.

override def compare(x: Iter, y: Iter): Int = -comparator.compare(x.head._1, y.head._1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the comment as we don't use the reverse of comparator.compare now?

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]] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it a real problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a real problem. However, I cannot find to disable only this check in findPuzzles.

if (old.isDefined) {
hadoopConf.set(flagName, old.get)
} else {
hadoopConf.unset(flagName)
}
}
}
Expand Down
29 changes: 29 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2606,6 +2606,35 @@
</execution>
</executions>
</plugin>
<plugin>
<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs-maven-plugin</artifactId>
<version>3.1.3</version>
<configuration>
<classFilesDirectory>${basedir}/target/scala-2.11/classes</classFilesDirectory>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also want to apply it to 2.12 later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, me too. Good catch.

<testClassFilesDirectory>${basedir}/target/scala-2.11/test-classes</testClassFilesDirectory>
<effort>Max</effort>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiszk, btw do you roughly know how much time this PR increases in the build?

Copy link
Member Author

@kiszk kiszk Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see large difference of maven's user time with and w/o Max

with Max

user	17m18.628s

without Max

user	17m3.724s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, how much does it increase the Maven build time by this whole PR then roughly?

<threshold>Low</threshold>
<xmlOutput>true</xmlOutput>
<visitors>FindPuzzlers</visitors>
<fork>false</fork>
</configuration>
<dependencies>
<dependency>
<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs</artifactId>
<version>3.1.3</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the spotbugs 3.1.3 plugin not already have this dependency? I would have assumed that's the default and we don't want to manage it separately. If we do maybe make a property for this version to make sure it doesn't vary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let us drop this part.

</dependency>
</dependencies>
<executions>
<execution>
<goals>
<goal>check</goal>
</goals>
<phase>compile</phase>
</execution>
</executions>
</plugin>
</plugins>
</build>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ trait ArraySortLike extends ExpectsInputTypes {
} else if (o2 == null) {
nullOrder
} else {
-ordering.compare(o1, o2)
ordering.compare(o2, o1)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,8 @@ 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
// avoid false positive of IM_BAD_CHECK_FOR_ODD by SpotBugs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that this comment might be a bit confusing to readers. Is it good without this comment?

val elseValue = if (branches.size % 2 != 0) Some(branches.last) else None
CaseWhen(cases, elseValue)
}
}
Expand All @@ -309,7 +310,8 @@ 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
// avoid false positive of IM_BAD_CHECK_FOR_ODD by SpotBugs
val elseValue = if (branches.size % 2 != 0) Some(branches.last) else None
CaseWhen(cases, elseValue)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1507,7 +1507,8 @@ 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 ""
// avoid false positive of IM_BAD_CHECK_FOR_ODD by SpotBugs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does it mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means x % 2 == does not work correctly if x is negative.

In this case, we know value.length is always non-negative . The current SpotBugs cannot detect this pre-condition. I cannot find to disable only this check in findPuzzles, too.

Therefore, this is a workaround to avoid the false positive violation.

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)
Expand Down