Copilot commented on code in PR #12525: URL: https://github.com/apache/gluten/pull/12525#discussion_r3587563450
########## 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) +### Override of Spark classes (For Spark3.3) Review Comment: The section header is version-specific (“For Spark3.3”), but the content below describes the general mechanism of overriding Spark classes. Since this doc covers limitations of the Velox backend across supported Spark versions, consider making the header generic to avoid implying it only applies to Spark 3.3. ########## 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) +### Override of Spark classes (For 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. Review Comment: Typo: “overrided” should be “overridden”. ########## 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) +### Override of Spark classes (For 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. So you need to ensure preferentially load the Gluten jar to overwrite the jar of vanilla spark. Refer to [How to prioritize loading Gluten jars in Spark](https://github.com/apache/gluten/blob/main/docs/velox-backend-troubleshooting.md#incompatible-class-error-when-using-native-writer). -If not officially supported spark3.2/3.3 version is used, NoSuchMethodError can be thrown at runtime. More details see [issue-4514](https://github.com/apache/gluten/issues/4514). +If not officially supported spark3.3 version is used, NoSuchMethodError can be thrown at runtime. More details see [issue-4514](https://github.com/apache/gluten/issues/4514). Review Comment: This warning currently says “unsupported spark3.3 version”, but the linked issue is about Spark 3.2 and the underlying problem applies to using any non-supported Spark build/version. Making the wording version-agnostic avoids an incorrect/overly narrow statement. ########## tools/gluten-it/common/src/main/java/org/apache/gluten/integration/SparkJvmOptions.java: ########## @@ -28,10 +28,10 @@ public static String read() { final Class<?> clazz = Class.forName("org.apache.spark.launcher.JavaModuleOptions"); 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 Review Comment: MODULE_OPTIONS_CLASS_NAME is declared but not used; Class.forName repeats the string literal. Using the constant avoids duplication and prevents future drift if the class name ever changes. ########## shims/spark33/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFileFormat.scala: ########## @@ -44,11 +44,10 @@ import java.io._ import java.net.URI /* - - * This class is copied from Spark 3.2 and modified for Gluten. \n + * This class is copied from Spark 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. + * we can move this class to a specific shim (shims-spark33, etc.). Review Comment: The ScalaDoc has a literal “\\n” and a couple grammar issues (“releases accepts… and make…”). Cleaning this up makes the comment clearer and avoids showing the escape sequence in generated docs. -- 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]
