[MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules#12543
Open
LuciferYang wants to merge 2 commits into
Open
[MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules#12543LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
Now that Gluten supports only Spark 3.3+, drop a handful of comment and code leftovers referring to Spark 3.1 / 3.2: * `gluten-substrait/.../WriteFilesExecTransformer.scala`: comment updated from "Spark3.2/3.3/3.4" to "Spark 3.3/3.4", with an extra sentence clarifying that Spark 3.5 strips these fields natively via SPARK-43123. Manual cleanup is still needed for the 3.3/3.4 branches. * `gluten-arrow/.../ArrowColumnarBatch.scala`: rewrite the class docstring. The old claim "Spark-3.2 declares ColumnarBatch as final" is factually wrong on every currently supported branch (`public class ColumnarBatch implements AutoCloseable` on 3.3/3.4/3.5/4.0/4.1). Replaced with a factual note that this is a historical fork kept standalone to preserve the writable-column contract. * `gluten-celeborn/.../CelebornShuffleManager.java`: drop the "Added in SPARK-32055, for Spark 3.1 and above" comment. That condition holds on every currently supported Spark version, so the note has no signal. * `backends-velox/.../velox/VeloxFormatWriterInjects.scala`: drop the "Do NOT add override keyword for compatibility on spark 3.1" comment and add the `override` keyword to the anonymous `OutputWriter.path()`. Spark's `OutputWriter.path(): String` is abstract on every supported branch, so `override` is legal there and now matches the neighbouring `override def close()`. * `tools/gluten-it/common/.../SparkJvmOptions.java`: replace the reflection-based lookup of `org.apache.spark.launcher.JavaModuleOptions` with a direct `JavaModuleOptions.defaultModuleOptions()` call. The class is `public` with a public static `defaultModuleOptions()` method, documented `@since 3.3.0`, and reachable via the `spark-launcher` transitive dep of `spark-core` on every supported branch. The Spark 3.2 `ClassNotFoundException` fallback was already removed by apache#12525; this eliminates the remaining reflection scaffolding. No behavior change on any supported Spark version.
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Cleans up remaining Spark 3.1/3.2 compatibility leftovers (mostly comments + small refactors) across non-ClickHouse modules, aligning the codebase with the currently supported Spark versions (3.3+).
Changes:
- Simplifies
SparkJvmOptionsby replacing reflection with a directJavaModuleOptions.defaultModuleOptions()call. - Removes outdated Spark-version compatibility comments and updates documentation comments to reflect current Spark behavior.
- Adds an
overridemodifier where it is now valid/expected on supported Spark branches.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/gluten-it/common/src/main/java/org/apache/gluten/integration/SparkJvmOptions.java | Drops reflection and directly calls JavaModuleOptions.defaultModuleOptions(). |
| gluten-substrait/src/main/scala/org/apache/gluten/execution/WriteFilesExecTransformer.scala | Updates Spark-version comment and adds Spark 3.5 context for internal metadata stripping. |
| gluten-celeborn/src/main/java/org/apache/spark/shuffle/gluten/celeborn/CelebornShuffleManager.java | Removes a now-uninformative Spark-version comment. |
| gluten-arrow/src/main/scala/org/apache/gluten/vectorized/ArrowColumnarBatch.scala | Rewrites class Scaladoc to remove outdated/factually incorrect Spark 3.2 claim. |
| backends-velox/src/main/scala/org/apache/spark/sql/execution/datasources/velox/VeloxFormatWriterInjects.scala | Adds override to OutputWriter.path() and removes obsolete Spark 3.1 compatibility note. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
23
to
25
| public static String read() { | ||
| try { | ||
| final Class<?> clazz = Class.forName(MODULE_OPTIONS_CLASS_NAME); | ||
| final Method method = clazz.getMethod("defaultModuleOptions"); | ||
| return (String) method.invoke(null); | ||
| } catch (ClassNotFoundException | ||
| | NoSuchMethodException | ||
| | InvocationTargetException | ||
| | IllegalAccessException e) { | ||
| throw new RuntimeException( | ||
| "Failed to read Spark JVM module options via " | ||
| + MODULE_OPTIONS_CLASS_NAME | ||
| + "#defaultModuleOptions", | ||
| e); | ||
| } | ||
| return JavaModuleOptions.defaultModuleOptions(); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Thanks for the note. I don't think a guard is worth adding here:
spark-launcheris acompile-scope dep ofspark-coreon every currently supported branch (Spark 3.3 through 4.1), soJavaModuleOptionswill always be on the classpath at runtime.- The only caller is
tools/gluten-it/sbin/gluten-it.sh, which runsSparkJvmOptionsagainst thepackage/target/libdirectory containing the full Gluten + Spark jars — the same classpath every Gluten job uses.
A guard would trade a standard JVM linkage error for a slightly nicer message in a scenario that has no realistic way to occur; every other Gluten module already calls Spark APIs directly and would fail earlier under a hypothetical missing-spark-core setup. I'd rather match that convention.
| } | ||
|
|
||
| // To be compatible with Spark3.2/3.3/3.4, we do cleanup spark internal metadata manually. | ||
| // To be compatible with Spark 3.3/3.4, we do cleanup spark internal metadata manually. |
Contributor
Author
There was a problem hiding this comment.
Good catch, fixed in bf250e9 — "we do cleanup spark..." → "we clean up Spark...".
LuciferYang
marked this pull request as draft
July 17, 2026 06:20
Address Copilot review feedback: "we do cleanup spark internal metadata" reads awkwardly. Change to "we clean up Spark internal metadata". No code change.
|
Run Gluten Clickhouse CI on x86 |
This was referenced Jul 17, 2026
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.
What changes were proposed in this pull request?
Follow-up to #12525 (which cleaned up doc / velox / SparkJvmOptions leftovers) and #12541 (CH-side workarounds). This picks up the remaining Spark 3.1 / 3.2-era leftovers in non-ClickHouse modules.
Comment-only fixes
gluten-substrait/.../WriteFilesExecTransformer.scala: comment updated from "Spark3.2/3.3/3.4" to "Spark 3.3/3.4", with an extra sentence clarifying that Spark 3.5 strips these fields natively via SPARK-43123. Manual cleanup is still needed for the 3.3/3.4 branches (SPARK-43123 was not backported), so the code path stays.gluten-arrow/.../ArrowColumnarBatch.scala: rewrite the class docstring. The old claim "Spark-3.2 declares ColumnarBatch as final" is factually wrong on every currently supported branch (public class ColumnarBatch implements AutoCloseableon 3.3 / 3.4 / 3.5 / 4.0 / 4.1). Replaced with a factual note that this is a historical fork kept standalone to preserve the writable-column contract.gluten-celeborn/.../CelebornShuffleManager.java: drop the "Added in SPARK-32055, for Spark 3.1 and above" comment. That condition holds on every currently supported Spark version, so the note has no signal.Small code refactors
backends-velox/.../velox/VeloxFormatWriterInjects.scala: drop the "Do NOT add override keyword for compatibility on spark 3.1" comment and add theoverridekeyword to the anonymousOutputWriter.path(). Spark'sOutputWriter.path(): Stringis abstract on every supported branch, sooverrideis legal there and now matches the neighbouringoverride def close().tools/gluten-it/common/.../SparkJvmOptions.java: replace the reflection-based lookup oforg.apache.spark.launcher.JavaModuleOptionswith a directJavaModuleOptions.defaultModuleOptions()call. The class ispublicwith a public staticdefaultModuleOptions()method, documented@since 3.3.0, and reachable via thespark-launchertransitive dep ofspark-coreon every supported branch. The Spark 3.2ClassNotFoundExceptionfallback was already removed by [MINOR] Align supported Spark version references #12525; this eliminates the remaining reflection scaffolding.No behavior change on any supported Spark version.
How was this patch tested?
./build/mvn -Pbackends-velox -Pspark-3.5 -pl backends-velox -am test-compile -DskipTests: SUCCESS./build/mvn -Pbackends-velox -Pspark-3.3 -pl backends-velox -am test-compile -DskipTests: SUCCESS./build/mvn -Pbackends-velox -Pspark-3.5 -pl gluten-arrow -am test-compile -DskipTests: SUCCESS./build/mvn -Pbackends-velox -Pspark-3.5 -pl gluten-substrait -am test-compile -DskipTests: SUCCESS./build/mvn -Pbackends-velox -Pspark-3.5 -Pceleborn -pl gluten-celeborn -am test-compile -DskipTests: SUCCESSjavac -cp spark-launcher_2.12-3.3.1.jar tools/gluten-it/common/.../SparkJvmOptions.java: SUCCESSjavac -cp spark-launcher_2.12-3.5.7.jar tools/gluten-it/common/.../SparkJvmOptions.java: SUCCESSjavac -cp spark-launcher_2.13-4.1.0-SNAPSHOT.jar tools/gluten-it/common/.../SparkJvmOptions.java: SUCCESSSparkJvmOptions#mainagainstspark-launcher_2.12-3.5.7and confirmed the output matches Spark'sJavaModuleOptions.defaultModuleOptions()string.branch-3.3/branch-3.4/branch-3.5/branch-4.0/branch-4.1:ColumnarBatchispublic class ColumnarBatch implements AutoCloseable(non-final) on all branches.OutputWriter.path(): Stringis abstract on all branches.JavaModuleOptions.defaultModuleOptions()exists aspublic staticon all branches.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-4-7