Copilot commented on code in PR #12525:
URL: https://github.com/apache/gluten/pull/12525#discussion_r3587923524
##########
tools/gluten-it/README.md:
##########
@@ -22,7 +22,7 @@ mvn clean package -P{Spark-Version}
sbin/gluten-it.sh
```
-Note: **Spark-Version** can only be **spark-3.2**, **spark-3.3**,
**spark-3.4** or **spark-3.5**.
+Note: **Spark-Version** can only be **spark-3.3**, **spark-3.4**,
**spark-3.5**, **spark-4.0** or **spark-4.1**.
Review Comment:
The README now advertises Spark 4.0/4.1 profiles, but the wrapper script
forces `SPARK_SCALA_VERSION=2.12` and prevents callers from setting it; Spark
4.x profiles in `tools/gluten-it/pom.xml` use Scala 2.13. As-is, following
these instructions will likely run with the wrong Scala binary version (or fail
to start). Update `sbin/gluten-it.sh` to set `SPARK_SCALA_VERSION` based on the
selected Spark profile (or stop overriding it), and then keep this expanded
profile list.
##########
docs/velox-backend-limitations.md:
##########
@@ -6,12 +6,12 @@ nav_order: 5
This document describes the limitations of velox backend by listing some known
cases where exception will be thrown, gluten behaves incompatibly with spark,
or certain plan's execution
must fall back to vanilla spark, etc.
-### Override of Spark classes (For Spark3.2 and Spark3.3)
-Gluten avoids to modify Spark's existing code and use Spark APIs if possible.
However, some APIs aren't exposed in Vanilla spark and we have to copy the
Spark file and do the hardcode changes. The list of override classes can be
found as ignoreClasses in package/pom.xml . If you use customized Spark, you
may check if the files are modified in your spark, otherwise your changes will
be overrided.
+### Override of Spark classes
+Gluten avoids to modify Spark's existing code and use Spark APIs if possible.
However, some APIs aren't exposed in Vanilla spark and we have to copy the
Spark file and do the hardcode changes. The list of override classes can be
found as ignoreClasses in package/pom.xml . If you use customized Spark, you
may check if the files are modified in your spark, otherwise your changes will
be overridden.
Review Comment:
This sentence has several grammatical issues and a stray space before the
period (`package/pom.xml .`). Since this paragraph was already edited, it’s a
good opportunity to make it clearer and more professional.
##########
tools/gluten-it/common/src/main/java/org/apache/gluten/integration/SparkJvmOptions.java:
##########
@@ -25,13 +25,13 @@ public class SparkJvmOptions {
public static String read() {
try {
- final Class<?> clazz =
Class.forName("org.apache.spark.launcher.JavaModuleOptions");
+ final Class<?> clazz = Class.forName(MODULE_OPTIONS_CLASS_NAME);
final Method method = clazz.getMethod("defaultModuleOptions");
return (String) method.invoke(null);
- } catch (ClassNotFoundException e) {
- // Could happen in Spark 3.2 which doesn't have this class yet.
- return "";
- } catch (NoSuchMethodException | InvocationTargetException |
IllegalAccessException e) {
+ } catch (ClassNotFoundException
+ | NoSuchMethodException
+ | InvocationTargetException
+ | IllegalAccessException e) {
throw new RuntimeException(e);
Review Comment:
`throw new RuntimeException(e)` loses important context when this fails via
`sbin/gluten-it.sh` (it will just abort with a stack trace). Including the
target class/method in the exception message makes classpath / Spark-version
issues much easier to diagnose.
##########
shims/spark33/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFileFormat.scala:
##########
@@ -43,12 +43,10 @@ import org.apache.orc.mapreduce._
import java.io._
import java.net.URI
-/* -
- * This class is copied from Spark 3.2 and modified for Gluten. \n
- * Gluten should make sure this class is loaded before the original class.
- * If new Spark releases accepts changes and make this class incompatible,
- * we can move this class to shims-spark32,
- * shims-spark33, etc.
+/**
+ * This class is copied from Spark and modified for Gluten. Gluten should make
sure this class is
+ * loaded before the original class. If a new Spark release accepts changes
that make this class
+ * incompatible, we can move this class to a version-specific shim
(shims-spark33, etc.).
Review Comment:
The comment refers to a non-existent shim name (`shims-spark33`). In this
repo the shim is organized under `shims/spark33` (artifact
`spark-sql-columnar-shims-spark33`). Using the actual path/name avoids
confusion for future maintainers.
##########
docs/get-started/Velox.md:
##########
@@ -7,18 +7,18 @@ parent: Getting-Started
# Supported Version
-| Type | Version |
-|-------|------------------------------|
-| Spark | 3.2.2, 3.3.1, 3.4.4, 3.5.5 |
-| OS | Ubuntu20.04/22.04, Centos7/8 |
-| jdk | openjdk8/jdk17 |
-| scala | 2.12 |
+| Type | Version |
+|-------|-------------------------------------|
+| Spark | 3.3.1, 3.4.4, 3.5.5, 4.0.2, 4.1.1 |
+| OS | Ubuntu20.04/22.04, Centos7/8 |
+| jdk | openjdk8/jdk17 |
+| scala | 2.12 |
Review Comment:
The supported-version table claims `jdk openjdk8/jdk17` and `scala 2.12`,
but this doc later states Spark 4.0/4.1 require JDK 17+ and Scala 2.13. Please
align the summary table with the per-profile requirements so readers don’t try
Spark 4.x with JDK 8 / Scala 2.12.
--
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]
