sunchao commented on code in PR #7000:
URL: https://github.com/apache/datafusion-comet/pull/7000#discussion_r4315351433
##########
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:
[P2] Materialize the class names before closing the scanner. On Scala 2.12,
used by the Spark 3.4/3.5 profiles, `Iterator.toSeq` returns a lazy `Stream`.
Consequently, `stream.close()` runs before the remaining directory entries are
consumed, and the structural equality test throws `IllegalStateException`
instead of discovering and checking the production classes. The JAR branch at
line 115 has the same problem and throws `IllegalStateException: zip file
closed`. This breaks `CometPlanEqualitySuite` on those profiles despite the
green Scala 2.13 CI run. Use `.toVector` or `.toList` in both branches so
enumeration completes inside the `try` block.
Evidence: Extracted the unchanged `productionClasses` helper from this head,
compiled it with Scala 2.12.18 on JDK 17, and invoked it through a
URLClassLoader on a directory containing three fixture classes and an
equivalent JAR. Forcing the returned sequence with `.map(_.getName).sorted`
failed at `FileTreeIterator.hasNext` for the directory and `ZipFile.ensureOpen`
with `zip file closed` for the JAR. Both inputs passed with Scala 2.13.17. The
previous Guava scanner and a disposable variant replacing both `.toSeq` calls
with `.toVector` passed on Scala 2.12.18. Scala 2.12.17 and 2.12.18 sources
both define `TraversableOnce.toSeq` as `toStream`.
--
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]
