andygrove commented on code in PR #7000:
URL: https://github.com/apache/datafusion-comet/pull/7000#discussion_r4530683934
##########
spark/src/test/scala/org/apache/spark/sql/comet/CometPlanEqualitySuite.scala:
##########
@@ -76,6 +78,48 @@ class CometPlanEqualitySuite extends AnyFunSuite {
.map(_.name.decodedName.toString)
.toSet
+ private def productionClasses(
+ loader: ClassLoader,
+ location: URL,
+ packageName: String): Seq[Class[_]] = {
+ val packagePath = packageName.replace('.', '/') + "/"
+ val root = Paths.get(location.toURI)
+ val classNames = if (Files.isDirectory(root)) {
+ val stream = Files.walk(root.resolve(packagePath))
+ try {
+ stream
+ .iterator()
+ .asScala
+ .filter(path => Files.isRegularFile(path) &&
path.toString.endsWith(".class"))
+ .map { path =>
+ root
+ .relativize(path)
+ .toString
+ .stripSuffix(".class")
+ .replace(File.separatorChar, '.')
+ }
+ .toSeq
Review Comment:
Fixed. Both branches of `productionClasses` use `.toVector`, so the
enumeration finishes inside the `try`. I ran `CometPlanEqualitySuite` on Spark
3.5 / Scala 2.12 as well as on the default Spark 4.1 / Scala 2.13 profile, and
it passes on both.
##########
spark/src/main/java/org/apache/spark/sql/comet/execution/shuffle/CometBypassMergeSortShuffleWriter.java:
##########
@@ -364,36 +362,19 @@ private long[]
writePartitionedData(ShuffleMapOutputWriter mapOutputWriter) thro
private void writePartitionedDataWithChannel(File file,
WritableByteChannelWrapper outputChannel)
throws IOException {
- boolean copyThrewException = true;
- try {
- FileInputStream in = new FileInputStream(file);
- try (FileChannel inputChannel = in.getChannel()) {
- Utils.copyFileStreamNIO(inputChannel, outputChannel.channel(), 0L,
inputChannel.size());
- copyThrewException = false;
- } finally {
- Closeables.close(in, copyThrewException);
- }
- } finally {
- Closeables.close(outputChannel, copyThrewException);
+ try (FileInputStream in = new FileInputStream(file);
+ WritableByteChannelWrapper ignored = outputChannel) {
Review Comment:
Good catch. The channel is declared first now, so it is closed when opening
the file fails, and it is still closed after the input.
##########
spark/pom.xml:
##########
@@ -36,6 +36,9 @@ under the License.
<properties>
<!-- The only published module: the parent pom skips deploying every
module -->
<maven.deploy.skip>false</maven.deploy.skip>
+ <!-- Hive's test-only catalog integration still uses Guava at runtime.
Keep the version local
+ so Guava is not exposed by or packaged into the published Comet
artifact. -->
+ <guava.version>33.2.1-jre</guava.version>
Review Comment:
Done. The root pom keeps `guava.version` at `33.7.1-jre`, and its
`dependencyManagement` entry for Guava is at test scope. The `spark` and
`delta-spark` poms have no version of their own, so the #7259 bump is kept. I
checked the dependency tree: there is no Guava in the compile and runtime
scopes, and the test scope has `33.7.1-jre`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
