Copilot commented on code in PR #12667:
URL: https://github.com/apache/gluten/pull/12667#discussion_r3749981817
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -55,7 +55,57 @@ concurrency:
cancel-in-progress: true
jobs:
+ # Detect which parts of the codebase were modified so downstream jobs can
+ # skip work that is irrelevant to the change (e.g. a pure C++ fix does not
+ # need to run any spark-test-* job, and a Spark-3.3-shim-only change does
+ # not need to run spark-test-spark34/35/40/41).
+ #
+ # Output flags:
+ # cpp – C++ sources, Velox build scripts, or dev tooling changed
+ # java – any cross-version Java/Scala/Maven source changed
+ # shims33/34/35/40/41 – version-specific shim layer changed
+ # tools_it – tools/gluten-it changed
+ #
+ # A job runs when ANY of the relevant flags is true, so:
+ # • gluten-core change → java=true → all spark-test jobs run ✓
+ # • shims/spark35 only → shims35=true → only spark35 jobs run ✓
+ # • pure cpp change → cpp=true → only native/tpc jobs run ✓
+ detect-changes:
+ runs-on: ubuntu-22.04
+ outputs:
+ cpp: ${{ steps.filter.outputs.cpp }}
+ java: ${{ steps.filter.outputs.java }}
+ shims33: ${{ steps.filter.outputs.shims33 }}
+ shims34: ${{ steps.filter.outputs.shims34 }}
+ shims35: ${{ steps.filter.outputs.shims35 }}
+ shims40: ${{ steps.filter.outputs.shims40 }}
+ shims41: ${{ steps.filter.outputs.shims41 }}
+ tools_it: ${{ steps.filter.outputs.tools_it }}
+ steps:
+ - uses: actions/checkout@v4
+ with:
+ fetch-depth: 0
+ - name: Detect changed paths
+ id: filter
+ run: |
+ BASE=${{ github.event.pull_request.base.sha }}
+ HEAD=${{ github.sha }}
+ changed=$(git diff --name-only "$BASE" "$HEAD")
+ echo "$changed"
+
+ match() { echo "$changed" | grep -qE "$1" && echo true || echo
false; }
+
+ echo "cpp=$(match '^(cpp/|ep/build-velox/|dev/)')"
>> $GITHUB_OUTPUT
+ echo "java=$(match
'^(pom\.xml|backends-velox/|gluten-(uniffle|celeborn|ras|core|substrait|arrow|delta|iceberg|hudi|paimon|ut)/|package/|build/mvn)')"
>> $GITHUB_OUTPUT
Review Comment:
The `java` change-detection regex matches `gluten-ut/**` via `...|ut)/`,
which makes `java=true` even for version-scoped changes under
`gluten-ut/sparkXX/`. That defeats the intended optimization where a
`gluten-ut/spark35/`-only change should only run Spark 3.5 jobs (via `shims35`)
instead of all `spark-test-*` jobs. It also doesn’t treat
`.github/workflows/**` (including `util/` scripts in this workflow’s
`on.pull_request.paths`) as relevant, so CI-only changes can cause downstream
jobs to be skipped and the workflow changes to go unvalidated.
##########
.github/workflows/velox_backend_enhanced.yml:
##########
@@ -52,7 +52,36 @@ concurrency:
cancel-in-progress: true
jobs:
+ # Detect which parts of the codebase were modified so downstream jobs can
+ # skip work that is irrelevant to the change.
+ detect-changes:
+ runs-on: ubuntu-22.04
+ outputs:
+ cpp: ${{ steps.filter.outputs.cpp }}
+ java: ${{ steps.filter.outputs.java }}
+ shims35: ${{ steps.filter.outputs.shims35 }}
+ shims40: ${{ steps.filter.outputs.shims40 }}
+ steps:
+ - uses: actions/checkout@v4
+ with:
+ fetch-depth: 0
+ - name: Detect changed paths
+ id: filter
+ run: |
+ BASE=${{ github.event.pull_request.base.sha }}
+ HEAD=${{ github.sha }}
+ changed=$(git diff --name-only "$BASE" "$HEAD")
+ echo "$changed"
+
+ match() { echo "$changed" | grep -qE "$1" && echo true || echo
false; }
+
+ echo "cpp=$(match '^(cpp/|ep/build-velox/|dev/)')"
>> $GITHUB_OUTPUT
+ echo "java=$(match
'^(pom\.xml|backends-velox/|gluten-(uniffle|celeborn|ras|core|substrait|arrow|delta|iceberg|hudi|ut)/|package/|build/mvn)')"
>> $GITHUB_OUTPUT
Review Comment:
The `java` change-detection regex matches `gluten-ut/**` via `...|ut)/`,
which makes `java=true` even for version-scoped changes under
`gluten-ut/spark35/` or `gluten-ut/spark40/`. That prevents shims-only changes
from skipping the other Spark-version job in this workflow. Also, changes to
`.github/workflows/**` aren’t considered, so edits to this workflow file can
skip the very jobs being modified.
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -55,7 +55,57 @@ concurrency:
cancel-in-progress: true
jobs:
+ # Detect which parts of the codebase were modified so downstream jobs can
+ # skip work that is irrelevant to the change (e.g. a pure C++ fix does not
+ # need to run any spark-test-* job, and a Spark-3.3-shim-only change does
+ # not need to run spark-test-spark34/35/40/41).
Review Comment:
This comment says a pure C++ change doesn’t need to run any `spark-test-*`
job, but the job `if:` conditions below explicitly include `cpp == 'true'` for
spark tests. Please align this explanatory comment with the actual skip rules
so it doesn’t mislead future edits.
This issue also appears on line 72 of the same file.
##########
.github/workflows/velox_backend_arm.yml:
##########
@@ -51,7 +51,34 @@ concurrency:
cancel-in-progress: true
jobs:
+ # Detect which parts of the codebase were modified so downstream jobs can
+ # skip work that is irrelevant to the change.
+ detect-changes:
+ runs-on: ubuntu-22.04
+ outputs:
+ cpp: ${{ steps.filter.outputs.cpp }}
+ java: ${{ steps.filter.outputs.java }}
+ tools_it: ${{ steps.filter.outputs.tools_it }}
+ steps:
+ - uses: actions/checkout@v4
+ with:
+ fetch-depth: 0
+ - name: Detect changed paths
+ id: filter
+ run: |
+ BASE=${{ github.event.pull_request.base.sha }}
+ HEAD=${{ github.sha }}
+ changed=$(git diff --name-only "$BASE" "$HEAD")
+ echo "$changed"
+
+ match() { echo "$changed" | grep -qE "$1" && echo true || echo
false; }
+
+ echo "cpp=$(match '^(cpp/|ep/build-velox/|dev/)')"
>> $GITHUB_OUTPUT
+ echo "java=$(match
'^(pom\.xml|backends-velox/|gluten-(uniffle|celeborn|ras|core|substrait|arrow|delta|iceberg|hudi|paimon|ut)/|package/|build/mvn)')"
>> $GITHUB_OUTPUT
Review Comment:
The `java` change-detection regex matches `gluten-ut/**` via `...|ut)/`,
which means changes under `gluten-ut/sparkXX/` will be treated as cross-version
Java changes. That makes it hard to keep version-scoped UT changes from
triggering unrelated jobs, and it also doesn’t account for
`.github/workflows/**` edits (this workflow runs when its own YAML changes).
--
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]
