[andygrove]

Which issue does this PR close?

N/A. This is dependency cleanup and does not fix a user-visible bug or add an enhancement.

Rationale for this change

Comet only used a small set of Guava utilities that have direct JDK equivalents. Removing those usages avoids carrying, shading, and relocating Guava and its failureaccess companion artifact in Comet's build and packaged JAR.

What changes are included in this PR?

• Replace Guava utilities with JDK equivalents for object hashing, stream copying and closing, unsigned-long test values, and classpath scanning.
• Remove the compile-scope Guava and failureaccess dependencies, Guava shade configuration, relocation rules, and duplicate-class exception. Hive's test-only catalog integration and Delta 4.x still need Guava at test runtime, so the root pom keeps guava.version and manages Guava at test scope, and the spark and delta-spark poms declare it at test scope without a version.
• Update contributor documentation examples to use java.util.Objects.

Spark's explicit Guava exclusion remains unchanged because it controls Spark's transitive dependency rather than Comet's direct dependency.

How are these changes tested?

• make core
• ./mvnw package -DskipTests (Spark 4.1 / Scala 2.13)
• ./mvnw -Pspark-3.5 package -DskipTests (Spark 3.5 / Scala 2.12)
• Focused plan-equality and shuffle suites: 6 passed, 1 Spark-4.2-only test canceled
• Parquet V1 suite with Spark 3.5: 79 passed, 2 canceled, 1 ignored
• Verified that the Maven dependency tree has no guava or failureaccess artifacts in the compile and runtime scopes. In the test scope it has guava and failureaccess, from the two test-scope declarations.
• CometPlanEqualitySuite on Spark 4.1 / Scala 2.13 and on Spark 3.5 / Scala 2.12, after merging main
• Verified the shaded JAR contains no Guava or relocated-Guava classes

[sunchao]

Reviewed the full 25-file diff from d0f367dc02520fe68f524db22083cc0af6e58bca to d3a8c67a8a756ee86a6ac580d9ed8870e08b33d6. The PR is not a draft. Found one introduced P2 issue. The snapshot and live discussion contain no existing reviews, comments, or review threads.

Summary

• Prior state and problem: Comet packaged and relocated Guava despite using only a few utility methods. This PR removes that production dependency while retaining Guava for Hive and Delta tests.
• Design approach: Replace Guava hashing, copying, cleanup, unsigned-value construction, and test class discovery with JDK APIs. Remove the corresponding dependency-management and shading configuration.
• Correctness: The hash replacements preserve every argument and use the same Arrays.hashCode algorithm. The unsigned-long expectations remain equivalent. Checked shuffle copying and cleanup against Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. Spark 4.1/4.2 already use transferTo for this bounded-copy operation. No production-result issue meeting the P1/P2 bar was identified.
• Compatibility analysis: The JDK replacements fit the project's Java 17 minimum. However, the new scanner fails under Scala 2.12 because Iterator.toSeq is lazy. This affects the supported Spark 3.4/3.5 test profiles and is the P2 finding below.
• Key design decisions: Keeping Guava at test scope preserves external test integrations without packaging it in Comet. Spark's existing dependency exclusion remains intact. Plan equality inputs, partitioning, and shuffle formats are unchanged.
• Implementation sketch: Production Scala changes replace imports and hash calls. Java changes replace stream utilities and remove testing annotations. Maven changes remove production Guava packaging. Test changes introduce directory/JAR scanning and equivalent unsigned-value expectations. Contributor examples follow the new hashing API.
• Performance: Removing the packaged dependency reduces bundled classes. The bounded-copy probes preserved byte counts and partition positions. No throughput benchmark was run, and no evidence-backed performance regression was identified.
• Design: The localized JDK substitutions are straightforward and avoid a replacement dependency. Try-with-resources simplifies bypass-writer cleanup while the surrounding failure path still aborts map output.
• Abstraction & complexity: The small cleanup helper preserves Guava's existing IOException handling. The test scanner is reasonably scoped, but its resource lifetime requires an explicitly strict collection before closing either resource.
• Behavioral changes worth calling out: Compared the affected paths with branch-1.1 at 89c729a9697467e8f8d89e3eadf1797a75e2328c. Removing packaged Guava is intended. The utility replacements preserve production semantics. Breaking Scala 2.12 class discovery is an unintended change.
• Suggested improvements: Materialize both scanner iterators with .toVector or .toList inside their try blocks, then verify CometPlanEqualitySuite under a Scala 2.12 profile. The strict-collection change passed the focused reproduction.

Routed skills: review-comet-pr and review-comet-shuffle-pr. Read AGENTS.md and the relevant contributor, shuffle, memory, development, and CI guidance.

Exact-head CI: 25 checks succeeded, 18 were skipped, and none failed or remained pending. Required Checks passed, including the Spark 4.1 expression, shuffle, execution, and scan suites, TPC-H/TPC-DS checks, native tests, and cross-profile compilation/lint checks. Extended Spark SQL, Iceberg, Delta, and macOS jobs were skipped. CI log downloads were blocked by network policy, so CI conclusions rely on check metadata.

Validation: The unchanged scanner helper failed for both directory and JAR inputs on JDK 17 with Scala 2.12.18, while Scala 2.13.17 passed. The Guava baseline and a disposable .toVector variant passed on Scala 2.12.18. Separate JDK 17/21 probes passed 256 hash comparisons, all 10 changed unsigned-long expectations, and nine bounded partition-copy cases. Full local Maven/native builds and extended integration suites were not run. The checkout remains unchanged.

[sunchao]

[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.

[andygrove]

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.

[comphead]

Thanks @andygrove. The Objects.hash and transferTo replacements look equivalent to me. Two things for the rebase, since main has moved:

• spark/src/main/spark-4.1+/org/apache/spark/sql/comet/CometArrowEvalPythonExec.scala (added by #6130 after this base) still imports com.google.common.base.Objects and calls Objects.hashCode(...). Git will not flag it since this PR does not touch the file, but with Guava at test scope I expect the default Spark 4.1 profile to fail to compile. I found no other com.google.common use on main outside the files this PR changes.
• #6784 adds CometTextNativeScanExec.scala with the same Guava Objects.hashCode, so whichever lands second needs a one-line change.

The description says the dependency tree has no guava or failureaccess, but two poms still declare Guava at test scope. Could it say that check was for compile and runtime scopes?

[comphead]

If new FileInputStream(file) throws, the second resource is never initialized, so outputChannel is not closed, where the previous code and Spark's BypassMergeSortShuffleWriter close it in a finally. Declaring the channel first keeps that and the original close order:

try (WritableByteChannelWrapper ignored = outputChannel;
    FileInputStream in = new FileInputStream(file)) {

The window is small since file.exists() was checked just before. I have not run it.

[andygrove]

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.

[comphead]

This re-pins Guava at 33.2.1-jre here and in contrib/delta-spark/pom.xml, while main is on 33.7.1-jre after #7259, so resolving the root pom.xml conflict by deleting the property would revert that bump. Could the root keep guava.version with its dependencyManagement entry moved to test scope? Then this pom could drop its local version, and the delta pom, which already uses ${guava.version}, would not need to change.

[andygrove]

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.

[andygrove]

Thanks @comphead. I merged main (1ddb4fe). You are right that CometArrowEvalPythonExec in spark-4.1+ still imported Guava's Objects, and a new operator that main added since used Objects.hashCode with three arguments, which java.util.Objects does not have. Both use Objects.hash now, and the default Spark 4.1 profile compiles. CometTextNativeScanExec is not on main yet, so whichever of that PR and this one lands second needs the one-line change. The description now says that the dependency check covers the compile and runtime scopes.