sunchao commented on code in PR #5365: URL: https://github.com/apache/datafusion-comet/pull/5365#discussion_r4012778373
########## spark/pom.xml: ########## @@ -222,6 +222,27 @@ under the License. </dependencies> <profiles> + <profile> + <!-- Only the contrib build needs the test-jar: contrib/delta-spark reuses CometTestBase + and the shared test utilities in its suites. Binding it here keeps the jar out of + every other profile's install and out of the release upload. --> + <id>delta</id> Review Comment: ### Correctness [P2] Enable the Delta profile when installing the prerequisite test JAR Moving this execution into `delta` means the prerequisite installs in `delta_contrib_test.yml` (lines 113 and 168) no longer produce Spark's `-tests.jar`: both install `common,spark` without `delta`. The following contrib-only `test` invocation requires that test JAR through `contrib/delta-spark/pom.xml:108–114`, but does not rebuild Spark. If no matching test artifact is available, the contrib build fails dependency resolution. An older repository or cache artifact can instead supply stale test classes. These commands never produce this checkout's current test JAR. Please activate `delta` for the prerequisite installs and the README's matching command, or use a build sequence that produces the current test JAR. The parent POM supplies no alternative execution; this is a new build regression distinct from the release-policy discussion in #5882. ########## .github/workflows/delta_contrib_test.yml: ########## @@ -0,0 +1,226 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Delta Contrib Tests + +# Reusable: invoked by ci.yml. Triggering, path filters, and concurrency +# live in the umbrella workflow. +on: + workflow_call: + +permissions: + contents: read + +env: + RUST_VERSION: stable + RUST_BACKTRACE: 1 + # Force GNU ld on Linux: rust-lld cannot resolve -ljvm against the Zulu JDK + # layout installed by setup-java (same rationale as pr_build_linux.yml). + RUSTFLAGS: "-Clink-arg=-fuse-ld=bfd" + # The container's default locale is POSIX, which makes the JVM's file-path encoder + # reject non-ASCII partition directory names the suites create. + LANG: "C.UTF-8" + LC_ALL: "C.UTF-8" + +jobs: + + contrib-delta: + name: Delta contrib (Spark ${{ matrix.profile.spark }}) + runs-on: ubuntu-24.04 + container: + image: amd64/rust + strategy: + matrix: + profile: + - spark: "3.5" + java_version: "17" + - spark: "4.0" + java_version: "17" + - spark: "4.1" + java_version: "17" + # spark-4.2 is intentionally absent: the contrib profile is dormant + # until a Delta release supports Spark 4.2. + fail-fast: false + steps: + - uses: actions/checkout@v7 + + - name: Setup Rust & Java toolchain + uses: ./.github/actions/setup-builder + with: + rust-version: ${{ env.RUST_VERSION }} + jdk-version: ${{ matrix.profile.java_version }} + + - name: Cache Maven dependencies + uses: actions/cache@v6 + with: + path: | + ~/.m2/repository + /root/.m2/repository + key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}-delta-${{ matrix.profile.spark }} + restore-keys: | + ${{ runner.os }}-java-maven- + + - name: Restore Cargo cache + uses: actions/cache/restore@v6 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + native/target + key: ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}-${{ hashFiles('native/**/*.rs') }} + restore-keys: | + ${{ runner.os }}-cargo-ci-${{ hashFiles('native/**/Cargo.lock', 'native/**/Cargo.toml') }}- + + - name: Build native library (CI profile) + run: | + cd native + # `delta` is in the default feature set, so no feature flag is needed. + cargo build --profile ci + env: + # Must match the flags spark_sql_test_reusable.yml builds with: + # cargo folds RUSTFLAGS into its fingerprints, so any divergence + # would make the shared cargo cache restore without ever hitting. + RUSTFLAGS: "-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd" + + # No cache save: this workflow never runs on a push to main, so the restore above + # falls back to the cache build_linux saves there. + + - name: Stage native library at release path + run: | + # Maven's -Prelease profile (activated below) expects libcomet.so + # under native/target/release/; --profile ci builds it under + # native/target/ci/ instead (same as the other native-building + # workflows), so copy it into place. + mkdir -p native/target/release + cp native/target/ci/libcomet.so native/target/release/libcomet.so + + - name: Install Comet core jars + run: | + ./mvnw -B -q -Prelease -Pspark-${{ matrix.profile.spark }} install -pl common,spark -DskipTests -Dspotless.check.skip=true + + - name: Run Delta contrib test suites + run: | + SPARK_HOME=$(pwd) COMET_CONF_DIR=$(pwd)/conf ./mvnw -B -Prelease -Pspark-${{ matrix.profile.spark }},delta test -pl contrib/delta-spark + + # The container jobs above have no Docker socket, so CometDeltaS3Suite cancels itself + # there. This job runs it on the plain runner, where Testcontainers can start MinIO. + contrib-delta-s3: + name: Delta contrib S3 suite (Spark 3.5, MinIO) + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v7 + + - name: Setup Rust & Java toolchain + uses: ./.github/actions/setup-builder Review Comment: ### Correctness [P2] Use runner-compatible dependency installation for the MinIO job This new job runs directly on `ubuntu-24.04`, but `setup-builder/action.yaml:35–37` executes `apt-get update` and `apt-get install` without `sudo`. That action works inside the other jobs' root container; the hosted Linux runner requires elevation for those commands ([GitHub runner privileges](https://docs.github.com/en/actions/reference/runners/github-hosted-runners#administrative-privileges)). Setup therefore fails before reaching the native build or `CometDeltaS3Suite`. Please use a setup path that elevates package installation on the plain runner while retaining Docker access. This is verified from the workflow/action wiring; the current workflow has not executed. -- 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]
