andygrove commented on code in PR #5841:
URL: https://github.com/apache/datafusion-comet/pull/5841#discussion_r4112539608
##########
.github/workflows/spark_sql_test_reusable.yml:
##########
@@ -91,55 +87,17 @@ jobs:
id: modules
run: python3 dev/ci/spark-sql-modules.py --modules "${{ inputs.modules
}}" --github-output "$GITHUB_OUTPUT"
- - name: Setup Rust & Java toolchain
- uses: ./.github/actions/setup-builder
- with:
- rust-version: ${{ env.RUST_VERSION }}
- jdk-version: ${{ inputs.java }}
-
- - name: Restore Cargo cache
- uses: actions/cache/restore@v6
+ - name: Setup Java toolchain
+ uses: actions/setup-java@v4
Review Comment:
Since #5881 the retrying Maven bootstrap runs inside `setup-builder`, so
once this is rebased, swapping `setup-builder` for a bare `setup-java` here
leaves `Build JVM Test Classes` as the only job that runs `./mvnw` (through
`setup-spark-builder`) without it. `linux_maven_bootstrap_failures` exempts
composite actions and main only has routing cases for the bootstrap, so nothing
would fail until a Maven Central reset takes out the queue's Spark SQL build.
Could this job call `./.github/actions/maven-bootstrap` right after
`setup-java`? The explicit `Bootstrap Maven` steps in
`pr_build_linux_checks.yml` also become redundant with `setup-builder` after
the rebase, and the guard would need to accept `setup-builder` as a bootstrap
if those steps go.
##########
dev/ci/compute-changes.py:
##########
@@ -546,11 +577,26 @@ def event_allows(job, event):
def compute(files, event):
- """Return {job: bool}, folding the path filter and the event policy."""
- return {
- name: event_allows(name, event) and matches(patterns, files)
+ """Return a new {output: bool} mapping for consumers and their native
build.
+
+ `files` is a reusable sequence of repository-relative changed paths;
+ `event` has the fields described by event_allows(). Neither input is
+ mutated. Manual dispatch selects every route even with no changed files;
+ other events require both path and event matches. The shared native build
+ is selected only after those decisions, taking the union of every caller's
+ routes, so Hive alone can start its producer and a denied opt-in route
+ cannot start an unused producer. Unknown events select nothing.
Configuration
+ lookup failures propagate as KeyError rather than returning partial output.
+ """
+ manual = event.get("name") == "workflow_dispatch"
+ outputs = {
+ name: event_allows(name, event) and (manual or matches(patterns,
files))
for name, patterns in FILTERS.items()
}
+ outputs["build_linux_native"] = any(
Review Comment:
When this is rebased onto #5976, its push-only override in `compute()`,
which sets `build_linux` for `NATIVE_LIBRARY_INPUTS` on push, has to run before
this union. If it is appended after it, a push that changes only
`contrib/lance/native/Cargo.toml` selects `build_linux` but not
`build_linux_native`, so the producer that owns main's native caches never runs
for it. I checked that by applying #5976's override after this `compute()` at
this head. #5976's `test_native_input_routing` asserts only `build_linux`, so
it would stay green either way. Could a test here assert `build_linux_native`
for a push of each native-only input? If #5976 takes comphead's suggestion to
extend `FILTERS["build_linux"]` instead of overriding, this goes away.
--
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]
