build: Enforce Java 8 API floor on presto-spark module closure#27785
Open
kevintang2022 wants to merge 1 commit into
Open
build: Enforce Java 8 API floor on presto-spark module closure#27785kevintang2022 wants to merge 1 commit into
kevintang2022 wants to merge 1 commit into
Conversation
Contributor
Reviewer's GuideAdds an Animal Sniffer–based Java 8 API compatibility check for presto-spark and its dependency closure, and fixes a single Java 9-only Math.floorDiv call to use the Java 8 overload instead. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jja725
previously approved these changes
May 13, 2026
Contributor
jja725
left a comment
There was a problem hiding this comment.
Thanks for this compile time check
jja725
reviewed
May 13, 2026
|
|
||
| <!-- | ||
| Modules consumed by presto-spark must remain runnable on a Java 8 JVM | ||
| (Sapphire / Presto-on-Spark 2.x runs Spark 2.4 driver and executors |
Contributor
There was a problem hiding this comment.
Sapphire seems to be meta specific so should not in OSS
Contributor
Author
There was a problem hiding this comment.
Yes i will remove the comments
Summary: Adds `animal-sniffer-maven-plugin` to `presto-spark` and the modules it transitively depends on, configured against the Java 8 signature DB. The plugin scans bytecode at the `process-classes` phase and fails the build on references to JDK APIs that do not exist in Java 8. Also fixes one pre-existing leak surfaced by the new check: `TimestampOperators.java:140` was calling `Math.floorDiv(long, int)` (added in Java 9). Cast the second argument to `long` to use the Java 8 `(long, long)` overload. `presto-spark` is consumed by Apache Spark 2.x deployments, which require Java 8. There is no compile-time enforcement of this floor today: `<project.build.targetJdk>8</project.build.targetJdk>` in the affected modules is a hint read by maven-pmd-plugin, not by `javac`, and `<air.check.skip-modernizer>true</air.check.skip-modernizer>` disables the only existing related check. As a result, contributors can introduce Java 9+ APIs (e.g. `Optional.or(Supplier)`, `Math.floorDiv(long, int)`, `Stream.toList()`) that compile and pass tests on the Java 17 CI matrix but throw `NoSuchMethodError` at runtime on a Java 8 driver/executor. - No public API changes. - New `process-classes` check fails the build on Java 9+ API references in opted-in modules. - Polymorphic-signature `MethodHandle.invoke`/`invokeExact` calls are globally ignored — animal-sniffer cannot statically resolve them, and they are not real violations. Differential Revision: D104913960
e3e5e6c to
5e095b1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Description
Adds
animal-sniffer-maven-plugintopresto-sparkand the modules it transitively depends on, configured against the Java 8 signature DB. The plugin scans bytecode at theprocess-classesphase and fails the build on references to JDK APIs that do not exist in Java 8.Also fixes one pre-existing leak surfaced by the new check:
TimestampOperators.java:140was callingMath.floorDiv(long, int)(added in Java 9). Cast the second argument tolongto use the Java 8(long, long)overload.Motivation and Context
presto-sparkis consumed by Apache Spark 2.x deployments, which require Java 8. There is no compile-time enforcement of this floor today:<project.build.targetJdk>8</project.build.targetJdk>in the affected modules is a hint read by maven-pmd-plugin, not byjavac, and<air.check.skip-modernizer>true</air.check.skip-modernizer>disables the only existing related check. As a result, contributors can introduce Java 9+ APIs (e.g.Optional.or(Supplier),Math.floorDiv(long, int),Stream.toList()) that compile and pass tests on the Java 17 CI matrix but throwNoSuchMethodErrorat runtime on a Java 8 driver/executor.Impact
process-classescheck fails the build on Java 9+ API references in opted-in modules.MethodHandle.invoke/invokeExactcalls are globally ignored — animal-sniffer cannot statically resolve them, and they are not real violations.Differential Revision: D104913960
Release Notes