Skip to content
Closed
Show file tree
Hide file tree
Changes from 10 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 @@ -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
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?

// 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]] {
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
22 changes: 22 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2606,6 +2606,28 @@
</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>
<executions>
<execution>
<goals>
<goal>check</goal>
</goals>
<phase>compile</phase>
</execution>
</executions>
</plugin>
</plugins>
</build>

Expand Down
6 changes: 6 additions & 0 deletions resource-managers/kubernetes/core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@
<scope>test</scope>
</dependency>

<dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is derived from the previous PR

<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>
Expand Down
5 changes: 5 additions & 0 deletions resource-managers/kubernetes/integration-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@
<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>
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,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)
}
}
Expand All @@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down