You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/07/17 06:11:50 UTC

[PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

LuciferYang opened a new pull request, #12543:
URL: https://github.com/apache/gluten/pull/12543

   ### 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 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.
   
   **Small code refactors**
   
   - `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 #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`: SUCCESS
   - `javac -cp spark-launcher_2.12-3.3.1.jar tools/gluten-it/common/.../SparkJvmOptions.java`: SUCCESS
   - `javac -cp spark-launcher_2.12-3.5.7.jar tools/gluten-it/common/.../SparkJvmOptions.java`: SUCCESS
   - `javac -cp spark-launcher_2.13-4.1.0-SNAPSHOT.jar tools/gluten-it/common/.../SparkJvmOptions.java`: SUCCESS
   - Ran the compiled `SparkJvmOptions#main` against `spark-launcher_2.12-3.5.7` and confirmed the output matches Spark's `JavaModuleOptions.defaultModuleOptions()` string.
   - Cross-checked upstream Spark on `branch-3.3` / `branch-3.4` / `branch-3.5` / `branch-4.0` / `branch-4.1`:
     - `ColumnarBatch` is `public class ColumnarBatch implements AutoCloseable` (non-final) on all branches.
     - `OutputWriter.path(): String` is abstract on all branches.
     - `JavaModuleOptions.defaultModuleOptions()` exists as `public static` on all branches.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   Generated-by: Claude claude-opus-4-7
   


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #12543:
URL: https://github.com/apache/gluten/pull/12543#issuecomment-5029939417

   cc @jackylee-ch 


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #12543:
URL: https://github.com/apache/gluten/pull/12543#discussion_r3600980989


##########
tools/gluten-it/common/src/main/java/org/apache/gluten/integration/SparkJvmOptions.java:
##########
@@ -16,28 +16,12 @@
  */
 package org.apache.gluten.integration;
 
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
+import org.apache.spark.launcher.JavaModuleOptions;
 
 public class SparkJvmOptions {
-  private static final String MODULE_OPTIONS_CLASS_NAME =
-      "org.apache.spark.launcher.JavaModuleOptions";
 
   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();
   }

Review Comment:
   Thanks for the note. I don't think a guard is worth adding here:
   
   - `spark-launcher` is a `compile`-scope dep of `spark-core` on every currently supported branch (Spark 3.3 through 4.1), so `JavaModuleOptions` will always be on the classpath at runtime.
   - The only caller is `tools/gluten-it/sbin/gluten-it.sh`, which runs `SparkJvmOptions` against the `package/target/lib` directory 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.



##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/WriteFilesExecTransformer.scala:
##########
@@ -287,7 +287,8 @@ object WriteFilesExecTransformer {
       .toLowerCase(Locale.ROOT)
   }
 
-  // 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.

Review Comment:
   Good catch, fixed in bf250e982 — "we do cleanup spark..." → "we clean up Spark...".



-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12543:
URL: https://github.com/apache/gluten/pull/12543#issuecomment-4999584257

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12543:
URL: https://github.com/apache/gluten/pull/12543#issuecomment-5029926932

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #12543:
URL: https://github.com/apache/gluten/pull/12543#issuecomment-5087284153

   Thank you @jackylee-ch 


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12543:
URL: https://github.com/apache/gluten/pull/12543#issuecomment-4999855728

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12543:
URL: https://github.com/apache/gluten/pull/12543#discussion_r3600767170


##########
tools/gluten-it/common/src/main/java/org/apache/gluten/integration/SparkJvmOptions.java:
##########
@@ -16,28 +16,12 @@
  */
 package org.apache.gluten.integration;
 
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
+import org.apache.spark.launcher.JavaModuleOptions;
 
 public class SparkJvmOptions {
-  private static final String MODULE_OPTIONS_CLASS_NAME =
-      "org.apache.spark.launcher.JavaModuleOptions";
 
   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();
   }

Review Comment:
   Switching from reflection to a direct reference changes the failure mode when spark-launcher (or JavaModuleOptions) is missing from the runtime classpath: this will now fail during class/method resolution with a NoClassDefFoundError, losing the prior targeted RuntimeException message. Consider keeping a small guard to rethrow with a clearer diagnostic while still using the direct call.



##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/WriteFilesExecTransformer.scala:
##########
@@ -287,7 +287,8 @@ object WriteFilesExecTransformer {
       .toLowerCase(Locale.ROOT)
   }
 
-  // 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.

Review Comment:
   Grammar/capitalization in this updated comment reads awkwardly ("we do cleanup spark..."). Consider rephrasing to "we clean up Spark...".



-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org

Re: [PR] [MINOR] Clean up Spark 3.2 / 3.1-era compat leftovers in non-CH modules [gluten]

Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch merged PR #12543:
URL: https://github.com/apache/gluten/pull/12543


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org