Copilot commented on code in PR #13327:
URL: https://github.com/apache/gluten/pull/13327#discussion_r4284729093
##########
backends-velox/src-iceberg/test/scala/org/apache/gluten/execution/VeloxIcebergSuite.scala:
##########
@@ -28,7 +28,7 @@ import org.apache.iceberg.{FileScanTask, UpdateSchema}
import org.apache.iceberg.expressions.Literal
import org.apache.iceberg.hadoop.HadoopCatalog
import org.apache.iceberg.io.{ResolvingFileIO, StorageCredential}
-import org.apache.iceberg.spark.{ScanTaskSetManager, Spark3Util, SparkCatalog,
SparkReadOptions}
+import org.apache.iceberg.spark.{ScanTaskSetManager, Spark3Util, SparkCatalog,
SparkTableCache}
Review Comment:
The test claims it works on both Iceberg 1.10 and 1.11+, but this
implementation hard-depends on `SparkTableCache` and changes the
`DataFrameReader.load(...)` argument from `tableName` to `taskSetId`. If this
test module is compiled/executed against Iceberg 1.10, it’s likely to fail
either at compile-time (if `SparkTableCache` isn’t present) or at runtime
(because `.load(taskSetId)` isn’t a resolvable table identifier in 1.10’s
path). A more robust approach is to branch by Iceberg capability: for 1.11+ use
`SparkTableCache` + `.load(taskSetId)`; for 1.10 use `.option(<old key>,
taskSetId).load(tableName)` (and avoid referencing `SparkTableCache` directly,
e.g., via reflection or a small compatibility shim compiled per Iceberg
version).
##########
backends-velox/src-iceberg/test/scala/org/apache/gluten/execution/VeloxIcebergSuite.scala:
##########
@@ -119,15 +119,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)
+ .load(taskSetId),
Review Comment:
The test claims it works on both Iceberg 1.10 and 1.11+, but this
implementation hard-depends on `SparkTableCache` and changes the
`DataFrameReader.load(...)` argument from `tableName` to `taskSetId`. If this
test module is compiled/executed against Iceberg 1.10, it’s likely to fail
either at compile-time (if `SparkTableCache` isn’t present) or at runtime
(because `.load(taskSetId)` isn’t a resolvable table identifier in 1.10’s
path). A more robust approach is to branch by Iceberg capability: for 1.11+ use
`SparkTableCache` + `.load(taskSetId)`; for 1.10 use `.option(<old key>,
taskSetId).load(tableName)` (and avoid referencing `SparkTableCache` directly,
e.g., via reflection or a small compatibility shim compiled per Iceberg
version).
##########
backends-velox/src-iceberg/test/scala/org/apache/gluten/execution/VeloxIcebergSuite.scala:
##########
@@ -119,15 +119,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)
Review Comment:
The test claims it works on both Iceberg 1.10 and 1.11+, but this
implementation hard-depends on `SparkTableCache` and changes the
`DataFrameReader.load(...)` argument from `tableName` to `taskSetId`. If this
test module is compiled/executed against Iceberg 1.10, it’s likely to fail
either at compile-time (if `SparkTableCache` isn’t present) or at runtime
(because `.load(taskSetId)` isn’t a resolvable table identifier in 1.10’s
path). A more robust approach is to branch by Iceberg capability: for 1.11+ use
`SparkTableCache` + `.load(taskSetId)`; for 1.10 use `.option(<old key>,
taskSetId).load(tableName)` (and avoid referencing `SparkTableCache` directly,
e.g., via reflection or a small compatibility shim compiled per Iceberg
version).
##########
.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:
The Spark 4.2 build lines drop `-Piceberg -Phudi -Pdelta -Ppaimon` compared
to the 3.5/4.0/4.1 builds, which will produce a materially different “bundle”
(missing integrations) and may surprise downstream users/jobs expecting parity
across Spark versions. If those profiles are intentionally unsupported on Spark
4.2, add an inline comment explaining why and consider ensuring CI coverage
elsewhere; otherwise, include the same integration profiles so nightly
artifacts remain consistent.
##########
.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
Review Comment:
The Spark 4.2 build lines drop `-Piceberg -Phudi -Pdelta -Ppaimon` compared
to the 3.5/4.0/4.1 builds, which will produce a materially different “bundle”
(missing integrations) and may surprise downstream users/jobs expecting parity
across Spark versions. If those profiles are intentionally unsupported on Spark
4.2, add an inline comment explaining why and consider ensuring CI coverage
elsewhere; otherwise, include the same integration profiles so nightly
artifacts remain consistent.
##########
.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:
The Spark 4.2 build lines drop `-Piceberg -Phudi -Pdelta -Ppaimon` compared
to the 3.5/4.0/4.1 builds, which will produce a materially different “bundle”
(missing integrations) and may surprise downstream users/jobs expecting parity
across Spark versions. If those profiles are intentionally unsupported on Spark
4.2, add an inline comment explaining why and consider ensuring CI coverage
elsewhere; otherwise, include the same integration profiles so nightly
artifacts remain consistent.
##########
.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:
The Spark 4.2 build lines drop `-Piceberg -Phudi -Pdelta -Ppaimon` compared
to the 3.5/4.0/4.1 builds, which will produce a materially different “bundle”
(missing integrations) and may surprise downstream users/jobs expecting parity
across Spark versions. If those profiles are intentionally unsupported on Spark
4.2, add an inline comment explaining why and consider ensuring CI coverage
elsewhere; otherwise, include the same integration profiles so nightly
artifacts remain consistent.
##########
.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:
The Spark 4.2 build lines drop `-Piceberg -Phudi -Pdelta -Ppaimon` compared
to the 3.5/4.0/4.1 builds, which will produce a materially different “bundle”
(missing integrations) and may surprise downstream users/jobs expecting parity
across Spark versions. If those profiles are intentionally unsupported on Spark
4.2, add an inline comment explaining why and consider ensuring CI coverage
elsewhere; otherwise, include the same integration profiles so nightly
artifacts remain consistent.
##########
backends-velox/src-iceberg/test/scala/org/apache/gluten/execution/VeloxIcebergSuite.scala:
##########
@@ -119,15 +119,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)
+ .load(taskSetId),
Review Comment:
Mixing “cached table ID” loading (`.load(taskSetId)`) with the staged-scan
option (`scan-task-set-id`) makes the intent ambiguous and ties the test to
multiple internal resolution paths. For Iceberg 1.11+ the cache lookup should
be sufficient (and the option may be redundant/ignored), while Iceberg 1.10’s
staged scan typically expects the real table identifier in `.load(...)`.
Consider making the test explicit with two clearly separated code paths (or
helper methods) so future upgrades don’t silently change which mechanism is
being exercised.
##########
.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
+ - name: Upload test report
+ if: always()
+ uses: actions/upload-artifact@v4
+ with:
+ name: ${{ github.job }}-report
+ path: '**/surefire-reports/TEST-*.xml'
+ - name: Upload unit tests log files
+ if: ${{ !success() }}
+ uses: actions/upload-artifact@v4
Review Comment:
This workflow uses `actions/upload-artifact@v7` elsewhere, but the newly
added/reintroduced Spark 3.5 test jobs use `@v4`. Standardizing on a single
major version within the workflow will reduce maintenance and avoid subtle
behavior differences between artifact action versions.
##########
.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
+ - name: Upload test report
+ if: always()
+ uses: actions/upload-artifact@v4
+ with:
+ name: ${{ github.job }}-report
+ path: '**/surefire-reports/TEST-*.xml'
+ - name: Upload unit tests log files
+ if: ${{ !success() }}
+ uses: actions/upload-artifact@v4
Review Comment:
This workflow uses `actions/upload-artifact@v7` elsewhere, but the newly
added/reintroduced Spark 3.5 test jobs use `@v4`. Standardizing on a single
major version within the workflow will reduce maintenance and avoid subtle
behavior differences between artifact action versions.
--
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]
