Copilot commented on code in PR #13131:
URL: https://github.com/apache/gluten/pull/13131#discussion_r4405465835
##########
.github/workflows/util/delta-spark-ut/java-test-args.sh:
##########
@@ -35,11 +35,22 @@
# the sbt LAUNCHER JVM) so the flags reach BOTH the launcher and the forked
test JVM
# -- the forked child inherits the parent env.
#
+# SparkConf also reads spark.* system properties as defaults. Keep all Gluten
+# test defaults here instead of replacing DeltaSQLCommandTest, so suites using
+# plain SharedSparkSession, TestHive or their own SparkSession builders inherit
+# the same settings. Those contexts must initialize Gluten too: Spark retains
+# its cache serializer across contexts in the same JVM, but the native
+# serializer needs the executor plugin to initialize TaskResources in each
task.
+#
+# Keep Delta's extensions and catalog suite-specific: some tests deliberately
+# omit them to check Delta's configuration errors.
+#
# NOTE: no -Xmx here on purpose. JAVA_TOOL_OPTIONS is processed BEFORE the JVM
# command line, so Delta's own -Xmx (build.sbt) would win; run-delta-tests.sh
bumps
# the forked-test-JVM heap via `set spark / Test / javaOptions ++= ...`
instead.
-export JAVA_TOOL_OPTIONS="${JAVA_TOOL_OPTIONS:+${JAVA_TOOL_OPTIONS} }\
+# Keep caller-provided -D properties last so they override these defaults.
Review Comment:
The comment says only caller-provided `-D` properties are kept last, but the
implementation appends the entire prior `JAVA_TOOL_OPTIONS` (which may include
non-`-D` JVM flags) after the defaults. Please update the comment to reflect
that all caller-provided `JAVA_TOOL_OPTIONS` entries are appended last (or, if
the intent truly is `-D` only, adjust the implementation accordingly).
##########
.github/workflows/util/delta-spark-ut/README.md:
##########
@@ -293,3 +328,6 @@ source
<gluten>/.github/workflows/util/delta-spark-ut/java-test-args.sh
`run-delta-tests.sh` sources the same file, so CI and local runs use identical
flags.
+
+Existing `JAVA_TOOL_OPTIONS` are preserved after the shared defaults, so
+caller-supplied `-D` properties take precedence.
Review Comment:
This implies precedence is specifically about `-D` properties, but in
practice the script appends the full prior `JAVA_TOOL_OPTIONS` (including any
non-`-D` JVM args) after the defaults. Consider rewording to indicate that
caller-supplied `JAVA_TOOL_OPTIONS` generally take precedence (including `-D`
properties).
--
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]
