Copilot commented on code in PR #13327:
URL: https://github.com/apache/gluten/pull/13327#discussion_r4325559546
##########
.github/workflows/velox_nightly.yml:
##########
@@ -411,6 +317,7 @@ jobs:
cd $GITHUB_WORKSPACE/ && \
./build/mvn clean install -Pspark-4.0 -Pscala-2.13 -Pjava-21
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
./build/mvn clean install -Pspark-4.1 -Pscala-2.13 -Pjava-21
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
+ ./build/mvn clean install -Pspark-4.2 -Pscala-2.13 -Pjava-21
-Pbackends-velox -Pceleborn -Puniffle -DskipTests -Dmaven.source.skip
Review Comment:
All Spark 4.2 build invocations omit integration profiles that are included
for Spark 3.5/4.0/4.1 (e.g., `-Piceberg -Phudi -Pdelta -Ppaimon`). If the
intent is to keep nightly coverage consistent across Spark versions, these
should be added to the Spark 4.2 commands as well; otherwise, nightly will stop
building/testing those integration artifacts for Spark 4.2.
##########
.github/workflows/velox_nightly.yml:
##########
@@ -576,6 +483,7 @@ jobs:
./build/mvn clean install -Pspark-3.5 -Pjava-17 -Pbackends-velox
-Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon -DskipTests
-Dmaven.source.skip
./build/mvn clean install -Pspark-4.0 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
./build/mvn clean install -Pspark-4.1 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
+ ./build/mvn clean install -Pspark-4.2 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -DskipTests -Dmaven.source.skip
Review Comment:
All Spark 4.2 build invocations omit integration profiles that are included
for Spark 3.5/4.0/4.1 (e.g., `-Piceberg -Phudi -Pdelta -Ppaimon`). If the
intent is to keep nightly coverage consistent across Spark versions, these
should be added to the Spark 4.2 commands as well; otherwise, nightly will stop
building/testing those integration artifacts for Spark 4.2.
##########
.github/workflows/velox_nightly.yml:
##########
@@ -135,6 +135,7 @@ jobs:
./build/mvn clean install -Pspark-3.5 -Pjava-17 -Pbackends-velox
-Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon -DskipTests
-Dmaven.source.skip
./build/mvn clean install -Pspark-4.0 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
./build/mvn clean install -Pspark-4.1 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
+ ./build/mvn clean install -Pspark-4.2 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -DskipTests -Dmaven.source.skip
Review Comment:
All Spark 4.2 build invocations omit integration profiles that are included
for Spark 3.5/4.0/4.1 (e.g., `-Piceberg -Phudi -Pdelta -Ppaimon`). If the
intent is to keep nightly coverage consistent across Spark versions, these
should be added to the Spark 4.2 commands as well; otherwise, nightly will stop
building/testing those integration artifacts for Spark 4.2.
##########
.github/workflows/velox_nightly.yml:
##########
@@ -592,3 +500,100 @@ jobs:
remote_port: ${{ secrets.NIGHTLIES_RSYNC_PORT }}
remote_user: ${{ secrets.NIGHTLIES_RSYNC_USER }}
remote_key: ${{ secrets.NIGHTLIES_RSYNC_KEY }}
+
+ spark-test-spark35-smj:
+ if: ${{ startsWith(github.repository, 'apache/') }}
+ needs: build-native-lib-x86
+ runs-on: ubuntu-22.04
+ env:
+ SPARK_TESTING: true
+ container: apache/gluten:centos-8-jdk8
+ steps:
+ - uses: actions/checkout@v4
+ - name: Download All Artifacts
+ uses: actions/download-artifact@v4
+ with:
+ name: velox-native-lib-${{github.sha}}
+ path: ./cpp/build/releases
+ - name: Prepare
+ run: |
+ dnf module -y install python39 && \
+ alternatives --set python3 /usr/bin/python3.9 && \
+ pip3 install setuptools==77.0.3 && \
+ pip3 install pyspark==3.5.5 cython && \
+ pip3 install pandas==2.2.3 pyarrow==20.0.0
+ - name: Build and Run unit test for Spark 3.5.5 (other tests)
+ run: |
+ cd $GITHUB_WORKSPACE/
+ export SPARK_SCALA_VERSION=2.12
+ yum install -y java-17-openjdk-devel
+ export JAVA_HOME=/usr/lib/jvm/java-17-openjdk
+ export PATH=$JAVA_HOME/bin:$PATH
+ java -version
+ $MVN_CMD clean test -Pspark-3.5 -Pjava-17 -Pbackends-velox -Piceberg
-Pdelta -Ppaimon -Pspark-ut \
+ -DargLine="-Dspark.test.home=/opt/shims/spark35/spark_home/
-Dspark.gluten.sql.columnar.forceShuffledHashJoin=false" \
+
-DtagsToExclude=org.apache.spark.tags.ExtendedSQLTest,org.apache.spark.tags.SlowHiveTest,org.apache.gluten.tags.UDFTest,org.apache.gluten.tags.EnhancedFeaturesTest,org.apache.gluten.tags.CudfTest,org.apache.gluten.tags.SkipTest
Review Comment:
This change newly excludes `org.apache.gluten.tags.CudfTest` from the Spark
3.5 nightly test run. If this exclusion is required (e.g., due to environment
constraints), it would be good to capture the rationale inline (comment)
because it reduces nightly coverage and can mask regressions.
##########
backends-velox/src-iceberg/test/scala/org/apache/gluten/execution/VeloxIcebergSuite.scala:
##########
@@ -120,15 +120,20 @@ class VeloxIcebergSuite extends IcebergSuite {
val table = Spark3Util.loadIcebergTable(spark, tableName)
val taskSetId = "vended-credentials"
val taskSetManager = ScanTaskSetManager.get()
+ val tableCache = SparkTableCache.get()
val tasks = table.newScan().planFiles()
try {
+ // Register the table under the task set ID and load by that ID,
which resolves to a
+ // staged scan on both Iceberg 1.10 (needs the "scan-task-set-id"
option) and Iceberg
+ // 1.11+ (removed SparkReadOptions.SCAN_TASK_SET_ID, resolves cached
tables directly).
+ tableCache.add(taskSetId, table)
taskSetManager.stageTasks(table, taskSetId,
tasks.asScala.toList.asJava)
compareDfResultsAgainstVanillaSpark(
() =>
spark.read
.format("iceberg")
- .option(SparkReadOptions.SCAN_TASK_SET_ID, taskSetId)
- .load(tableName),
+ .option("scan-task-set-id", taskSetId)
Review Comment:
The option key is now a raw string. To avoid typos and make future refactors
safer (especially since Iceberg removed `SparkReadOptions.SCAN_TASK_SET_ID`),
consider introducing a local constant (e.g., `private val ScanTaskSetIdOption =
\"scan-task-set-id\"`) and using it here.
##########
.github/workflows/velox_nightly.yml:
##########
@@ -170,110 +171,14 @@ jobs:
cd $GITHUB_WORKSPACE/ && \
./build/mvn clean install -Pspark-4.0 -Pscala-2.13 -Pjava-21
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
./build/mvn clean install -Pspark-4.1 -Pscala-2.13 -Pjava-21
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -DskipTests
-Dmaven.source.skip
+ ./build/mvn clean install -Pspark-4.2 -Pscala-2.13 -Pjava-21
-Pbackends-velox -Pceleborn -Puniffle -DskipTests -Dmaven.source.skip
Review Comment:
All Spark 4.2 build invocations omit integration profiles that are included
for Spark 3.5/4.0/4.1 (e.g., `-Piceberg -Phudi -Pdelta -Ppaimon`). If the
intent is to keep nightly coverage consistent across Spark versions, these
should be added to the Spark 4.2 commands as well; otherwise, nightly will stop
building/testing those integration artifacts for Spark 4.2.
##########
.github/workflows/velox_nightly.yml:
##########
@@ -376,6 +281,7 @@ jobs:
./build/mvn clean install -Pspark-3.5 -Pjava-17 -Pbackends-velox
-Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon -DskipTests
-Dmaven.source.skip
./build/mvn clean install -Pspark-4.0 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
./build/mvn clean install -Pspark-4.1 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -Piceberg -Phudi -Pdelta -Ppaimon
-DskipTests -Dmaven.source.skip
+ ./build/mvn clean install -Pspark-4.2 -Pscala-2.13 -Pjava-17
-Pbackends-velox -Pceleborn -Puniffle -DskipTests -Dmaven.source.skip
Review Comment:
All Spark 4.2 build invocations omit integration profiles that are included
for Spark 3.5/4.0/4.1 (e.g., `-Piceberg -Phudi -Pdelta -Ppaimon`). If the
intent is to keep nightly coverage consistent across Spark versions, these
should be added to the Spark 4.2 commands as well; otherwise, nightly will stop
building/testing those integration artifacts for Spark 4.2.
##########
backends-velox/src-iceberg/test/scala/org/apache/gluten/execution/VeloxIcebergSuite.scala:
##########
@@ -120,15 +120,20 @@ class VeloxIcebergSuite extends IcebergSuite {
val table = Spark3Util.loadIcebergTable(spark, tableName)
val taskSetId = "vended-credentials"
val taskSetManager = ScanTaskSetManager.get()
+ val tableCache = SparkTableCache.get()
Review Comment:
Using a fixed `taskSetId` can cause collisions if tests run concurrently (or
if a prior failure leaves state behind temporarily), since both
`ScanTaskSetManager` and `SparkTableCache` are effectively global. Consider
generating a unique ID per test invocation (e.g., suffix with a UUID) to avoid
cross-test interference/flakiness.
--
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]
