ci: reuse Linux native libraries across workflow runs

[sunchao]

Which issue does this PR close?

Part of #5830. Complements #5841, which shares native builds within one workflow run. This PR reuses finished Linux native libraries across runs and preserves #5973's main-only cache writes.

Rationale for this change

After the recent CI improvements, the relevant baseline is a warm incremental build. The #5991 native job took 3m51s, and a nightly native job took 4m13s. Restoring the target cache took 43–47 seconds; Cargo took 1m57s–2m22s. More recent warm jobs on September 24–25 took 4m16s to 4m30s. These are baseline measurements, not this PR's library-hit timings.

A Scala planner or test change can reuse the same libcomet.so when native inputs and environment match. Downloading that library avoids restoring the roughly 1.4 GiB compiler cache and invoking Cargo. The historical cold-build cost is not the expected saving. Actual library-hit, end-to-end turnaround, entry-size and retention benefits remain to be measured after main publishes the new caches.

What changes are included in this PR?

The Linux, Spark SQL, Iceberg and manual writer workflows share a composite action that fingerprints the checkout and builder, then restores or builds libcomet.so. PR, queue, scheduled and manual runs skip Cargo on an exact library hit. A miss restores compatible intermediate files and runs cargo build --locked --profile ci. Intermediate-cache hits alone never bypass Cargo. dev/local-ci.sh also uses --locked; manifest changes requiring a lockfile update must include it.

Only pushes to main publish caches. Main skips compilation when both the library and incremental entries match exactly; otherwise it builds to replenish the missing entry. A Rust-changing PR cannot publish its own reusable library, so a later Scala-only update to that PR still requires compilation until its native inputs are available from main.

The fingerprint covers tracked native sources, protobufs, Cargo manifests/lockfile/configuration, shared setup/build actions, and observed Rust, package, JDK and relevant build-environment identity. Disabled contrib crates contribute their manifests because Cargo validates them; their Rust sources and standalone lockfiles are excluded. Benchmarks enter the separate debug key. Caller workflow files are excluded, so changing test shards preserves library reuse. The shared input lists and glob-matching implementation participate in the fingerprint independently of unrelated routing policy.

Linux routing now includes the shared native build inputs directly. Contrib-manifest changes therefore select validation on PRs and the merge queue as well as main's producer, while retaining shuffle-benchmark coverage. After compilation, a dep-info guard checks libcomet.d against the tracked-input contract before either cache is published, with explicit generated-protobuf and JDK exceptions. New file inputs require updating that contract.

Preflight checks that consumers match main's native producer: Linux runner, Rust container, toolchain/JDK selection, and declared environment before native reuse. A different builder needs a compatible publisher; unknown overrides require a contract update. The fingerprint supports the official builder, not arbitrary external files identified only by a path. Both library and incremental keys retain package/JDK identity because native dependencies compile C against JNI headers and Cargo does not fully track external tool/header changes; unrelated package updates can cause conservative misses.

The action scopes native RUSTFLAGS to fingerprinting and compilation without changing later caller steps, and exposes distinct library-key and cargo-key outputs. Incremental CI/debug entries contain only native/target; Cargo re-fetches registry/Git sources as needed. Rust checks and tests continue with their separate debug cache, and JVM consumers receive the library through the existing artifact flow.

With #5841, one producer can restore or build once and distribute the result within a run. Either PR can land first; the planned integration lands this PR, then rebases #5841 onto the composite action while retaining main-only writes.

The new cache namespaces deliberately exclude legacy entries, so the first main build is cold. At merge time, inventory and narrowly delete superseded legacy native-cache IDs on refs/heads/main, preserving new namespaces and Maven/dataset caches. No live caches have been deleted. Then measure all three compressed entries, retention and hit rate, verify a matching PR skips Cargo and passes downstream tests, and compare native-job and end-to-end timings with the warm baseline.

How are these changes tested?

Local validation at de25d50ff2c7fd58769917dc5c8249f9a05f601a: 22 native-cache/configuration tests, CI configuration and suite checks, 21 Iceberg tests, four PR-label tests, benchmark-runner checks, actionlint, shell syntax, and whitespace checks all pass. Coverage includes fingerprint invalidation/stability, routing, dependency-list enforcement, cache hit/miss flows, main publication policy, scoped flags and producer-environment invariants.

Independent small Cargo builds verified the dep-info parser against real output and rejected excluded tracked inputs and untracked files inside a watched directory. A real Comet cargo metadata --locked --offline --filter-platform x86_64-unknown-linux-gnu probe reproduced the stale-lockfile failure for a Lance manifest-only edit. No full Comet native/JVM build ran locally.

At the previous head 1c8698c4, regular CI, Spark 4.1, and Iceberg label runs passed, and all six producers computed identical library keys. At the new head, hosted Preflight passed, including license/Markdown checks, native-cache tests, CI invariants and actionlint. The full CI run remains in progress; the Spark 4.1 and Iceberg test labels remain applied. The manual writer workflow has no exact-head hosted run. These checks do not establish a real cross-run library hit: main publication and the rollout measurements above remain outstanding.

[andygrove]

Thanks for this. Collapsing four copy-pasted cargo cache blocks into one composite action is a clear win on its own, and the security posture is right: only push-to-main saves and pull requests only consume, so a fork PR cannot plant a libcomet.so that another PR then executes. That is the property that matters most in a scheme like this and it is handled correctly. The lookup-only flag on main so the producer does not download a library it will not run is a nice touch, and fixing the ~/.cargo versus /usr/local/cargo CARGO_HOME mismatch is a real latent bug fix. The test file using throwaway git repositories rather than mocks is also good to see.

A few things I would like to work through before this lands.

Sequencing against #5973

I think this needs #5973 in front of it, and I would rather not merge it first.

I queried the cache API while reading this. The repository reports 9.06 GB in use, and refs/heads/main holds nothing at all. Everything live is on refs/pull/5420/merge, refs/pull/5615/merge, refs/pull/5565/merge and one gh-readonly-queue/main/pr-5934-* branch, and there is no Linux-cargo-ci-* or Linux-cargo-debug-* entry left anywhere. That independently reproduces what you documented on #5973 from 2026-09-15.

The concern is that this PR's whole premise is that main publishes the library and pull requests restore it, but a run can only restore from its own ref plus the default branch. With main holding nothing and 2.1 to 2.3 GB Maven entries still being written from PR refs, I would expect the new library entry to be evicted before any pull request gets to read it. This PR is well behaved on its own writes, so it cannot fix that from here. It also adds two fresh namespaces, Linux-cargo-ci-v3- and Linux-native-ci-v2-, which consume budget while delivering nothing until the eviction pressure is gone.

Would you be up for landing #5973 first and then rebasing this onto it? That would also give you a real hit rate to put in the description instead of the current 28m45s cold build.

The incremental restore prefix gets weaker than what it replaces

In cache_keys, the prefix is Linux-cargo-{profile}-v3-{digest([environment, dependencies])}-, and environment carries the full dpkg-query -W package list along with rustc -vV, java_release, java_home and cargo_home.

setup-builder runs apt-get update && apt-get install -y protobuf-compiler clang against amd64/rust, which is an unpinned rolling tag. So package versions can drift between main's producer run and a pull request run hours later with no repository change at all. When that happens we miss the binary key and the incremental prefix together and get a fully cold build. Today the fallback is just the Cargo.lock and Cargo.toml hash, so it would still restore.

Could the prefix stay coarse and keep packages in the binary key only? That keeps the exact-match safety where it matters without giving up the incremental fallback.

contrib/*/native/** in the binary key

native/Cargo.toml carries exclude = ["../contrib"] and pulls the contrib crates in only as optional path dependencies behind their features, and the CI build is cargo build --locked --profile ci with no contrib feature enabled.

Does a contrib/*/native/** change actually affect libcomet.so? If it cannot, having it in INPUT_PATTERNS means a Delta-only change invalidates the shared library key for every consumer.

The JDK entries reverse a documented invariant

This drops # Note: Java version intentionally excluded - Rust target is JDK-independent from the debug key, and environment_inputs now feeds java_home and java_release into both keys.

Was that comment wrong? If the JNI headers and libjvm genuinely are build inputs it would help to say so where the old note used to be, since this directly contradicts it. If the target really is JDK independent, leaving the JDK out would avoid invalidating everything on a toolcache patch bump. Worth noting java_home is a path carrying the exact version, so it moves on patch bumps too. Every caller passes java: 17 today, so nothing is fragmented across jobs right now, but that is the part I would not want to rely on silently.

Two glob dialects for one set of paths

native-cache-key.py matches with fnmatch.fnmatchcase, where * crosses / and ** carries no special meaning. compute-changes.py matches with glob_to_regex, where * becomes [^/]* and ** is recursive. The same path strings now appear in three places: INPUT_PATTERNS, the new inline list inside compute(), and the FILTERS loop.

Concretely, contrib/a/b/native/x.rs enters the binary key under the first dialect but does not warm main under the second. test_every_binary_key_input_has_a_main_cache_warmer only walks files present in the tree today plus three hardcoded names, so that kind of drift would not be caught.

Could the helper import the matcher from compute-changes.py so there is one dialect and ideally one list?

--locked is a behaviour change worth calling out

The build step goes from cargo build --profile ci to cargo build --locked --profile ci. A pull request that edits Cargo.toml without refreshing Cargo.lock now fails the build rather than updating the lock. That seems like the right call for something we are going to cache and reuse, but it is not mentioned in the description and it will surprise someone. Worth confirming it is deliberate and noting it there.

[sunchao]

Thanks, @andygrove, for the review. I pushed e3b9e8542 and updated the description. Going through the six points:

1. Sequencing: agreed. The description now explicitly depends on ci: write large actions/cache entries only on push to main #5973 landing first and calls for rebasing onto it before merging. ci: write large actions/cache entries only on push to main #5973 is still open, so that rebase and verification of main's cache retention remain pending. The description continues to distinguish the sampled cold-build cost from savings that still need measurement.

2. Incremental fallback: I kept the package/JDK compatibility boundary for the compiled target cache after checking the native dependencies. There is a concrete correctness issue with relying on Cargo alone here: in a small offline build using Comet's exact locked cc 1.4.5, changing an external C header from a value of 1 to 2 left the ordinary rebuild returning 1; cleaning the target produced 2. Replacing a compiler at the same path similarly left its old output cached until cleaning. The default hdfs-sys dependency compiles C against JDK headers, and those external inputs are not fully tracked by its build scripts. A coarse target restore could therefore publish old native objects under the new library fingerprint. I documented why the environment stays in the fallback prefix and extended the existing test to cover that boundary. Unrelated package updates can still cause misses; narrowing that identity needs evidence about the actual native toolchain inputs, or a pinned builder.

3. Contrib inputs: narrowed to contrib/*/native/Cargo.toml. Changes to disabled contrib Rust sources and their standalone lockfiles now preserve the key and no longer select the shared Linux cache warmer. The manifests stay included because Cargo resolves optional dependencies when validating the native workspace lockfile, even when those features are disabled.

4. JDK identity: the old JDK-independent comment was wrong for the default HDFS build. It uses JNI headers and links libjvm; core/build.rs already documents a stale cached JDK-path failure. I added that explanation beside the debug fingerprint and retained the JDK identity.

5. Glob dialects: the key helper now imports the existing matcher and shared native-input lists from compute-changes.py. Main's warmer uses that same library-input list. The matcher module itself is included in the fingerprint, and the existing tests cover nested contrib paths as well as the supported one-level manifests.

6. --locked: deliberate, and now explicit in both the description and workflow documentation. A manifest edit that requires a new native/Cargo.lock fails CI until the lockfile update is included.

The six focused tests, CI configuration checks, actionlint, Markdown formatting, and whitespace checks pass. The expanded contrib/matcher regressions failed before these changes and pass afterward. Hosted CI for this new head is pending; actual cross-run library reuse still needs verification after main populates the cache.

[andygrove]

Thanks for the revision. I re-read the whole thing at e3b9e8542 and checked out the branch to verify the answers rather than just reading the diff. Four of the six are clean as far as I can tell:

The glob dialects really are unified now. contrib/a/b/native/Cargo.toml gets the same answer from the fingerprint and from main's routing, and the tests pin both directions. The contrib narrowing is right, and the rationale holds up: native/Cargo.toml has exclude = ["../contrib"], native/core/Cargo.toml has default = ["hdfs-opendal"] with no contrib feature, and both comet-contrib-delta and comet-contrib-lance do appear in native/Cargo.lock, so --locked genuinely depends on those manifests. The JDK point is settled: hdfs-opendal is a default feature, it pulls hdfs-sys, and core/build.rs reads JAVA_HOME, so the old comment was simply wrong. And --locked is now called out in both places.

On the incremental prefix I was wrong and you were right. The stale C object argument is the correct one and I should have tested cc's header tracking before asserting the old fallback was strictly better. The paragraph you added explaining it lands exactly where I wanted it.

I also want to flag something structural that I did not say clearly enough the first time, because it is the lens for most of what follows. An exact binary key hit is the first cache in Comet's CI that skips compilation outright. Every cargo cache we have had until now was an incremental aid where Cargo still re-validated everything, so an incomplete key cost time and nothing else. Here an incomplete key produces a wrong library that the JVM suites then test against. That raises the bar on key completeness a long way, and it is worth the two of us being paranoid about it.

Five things from this pass.

The four caller workflows in the library key are the biggest source of churn

NATIVE_BUILD_INPUTS hashes pr_build_linux.yml, spark_sql_test_reusable.yml, iceberg_spark_test_reusable.yml and spark_sql_writer_tests.yml into the library key. I counted commits touching each library key input on main over the last 90 days:

input	commits
.github/workflows/pr_build_linux.yml	57
native/Cargo.lock	40
dev/ci/compute-changes.py	17
.github/workflows/spark_sql_test_reusable.yml	9
.github/workflows/iceberg_spark_test_reusable.yml	7
.github/workflows/spark_sql_writer_tests.yml	4
.github/actions/setup-builder/action.yaml	0

pr_build_linux.yml is the highest churn input in the entire key, ahead of Cargo.lock, at roughly one edit every 1.6 days. Each of those is a cold native build for the PR that makes the edit.

After this PR I do not think those files carry anything the key needs. The build recipe moved into .github/actions/build-native-ci, which is already in the key. The only native relevant content left in the callers is the container image, RUST_VERSION, the JDK version and RUSTFLAGS, and environment_inputs already observes all four directly through dpkg-query, rustc -vV, $JAVA_HOME/release and the step env. The observational probe is the stronger guard anyway, because it also catches a base image change that no file in the repository records.

spark_sql_writer_tests.yml is the clearest case. It is workflow_dispatch only, so a manual workflow that never runs on a PR currently invalidates the shared library for everybody.

Could NATIVE_BUILD_INPUTS keep .github/actions/setup-builder/** and NATIVE_CACHE_RECIPES and drop the four workflow paths?

environment_inputs allowlists three variables

It reads JAVA_HOME, CARGO_HOME and RUSTFLAGS. Everything else that reaches the compiler passes through unrecorded: CC, CXX, CFLAGS, PROTOC, RUSTC_WRAPPER, CARGO_BUILD_*, CARGO_PROFILE_CI_*.

I checked all four callers and none of them sets anything beyond RUST_VERSION, RUST_BACKTRACE and RUSTFLAGS, so there is no bug in this revision. What bothers me is that nothing in the new tests or in check-ci-config.py would notice a fourth variable appearing, and per the point above this is the one place where being wrong produces a stale library rather than a slow build.

Would a prefix sweep be safer than an allowlist? Something like

"env": {k: v for k, v in sorted(env.items())
        if k.startswith(("CARGO_", "RUST"))
        or k in {"CC", "CXX", "CFLAGS", "CXXFLAGS", "LDFLAGS", "AR", "PROTOC", "JAVA_HOME"}},

It picks up RUSTFLAGS, CARGO_HOME and JAVA_HOME for free, stays deterministic on the fixed builder, and means a future env: addition invalidates the key by default rather than by someone remembering to update this function. It matters more if you take the previous point, since dropping the caller workflows from the file list leaves this probe as the only guard.

RUSTFLAGS is written out twice in the composite

Lines 29 and 58 of .github/actions/build-native-ci/action.yaml carry the same literal, once as the env the key is computed under and once as the env the build runs under. Those two have to agree or the key describes a build that did not happen, and nothing fails if they drift.

Composite actions do not take a runs: level env:, but a leading step would do it:

- name: Pin native build flags
  shell: bash
  run: echo 'RUSTFLAGS=-Ctarget-cpu=x86-64-v3 -Clink-arg=-fuse-ld=bfd' >> "$GITHUB_ENV"

and then both step level env: blocks can go. That also makes the flags visible to the sweep above, if you take it.

The test file is routed to the merge queue suites

The _native_consumer loop appends dev/ci/test-native-cache-key.py alongside the recipes. I diffed the routing before and after: on merge_group, editing only that file now selects spark_4_1, spark_4_1_hive and iceberg_1_11, which are the three heaviest suites left on that tier after #5963.

The test cannot affect the library, the routing or the recipe, and Preflight already runs it on every event through the new "Check native cache keys" step, so those three suites are not checking anything. The comment above NATIVE_CACHE_RECIPES says tests "are routed separately below", but separately turns out to be the same nine jobs.

Dropping it from the loop leaves it covered by Preflight plus the existing dev/ci/** route into build_linux. I would keep dev/ci/compute-changes.py in the loop, since that one really is in the fingerprint.

The CARGO_HOME fix makes the entry bigger, not the same size

Worth saying explicitly in the description. ~/.cargo/registry does not exist in the amd64/rust container, so today's Linux-cargo-ci-* entry only ever held native/target. Pointed at /usr/local/cargo it will now also carry the registry for a 675 crate lockfile and the iceberg-rust git checkout.

I re-queried the cache API while reading this revision. The repository is now at 17.34 GB across 22 entries, up from the 9.06 GB I reported yesterday, refs/heads/main still holds nothing, and there is still no Linux-cargo-ci-* or Linux-cargo-debug-* entry anywhere. Everything live is on a PR merge ref or the merge queue, dominated by Linux-java-maven-* entries at 0.9 to 1.9 GB each. That is not an objection to the design, it just confirms the sequencing you already agreed to, and it means the new namespaces will be landing into a tighter budget than the description assumes.

Could the first main push after this lands report the measured size of both new entries?

Two smaller things

native-cache-key.py reaches for runpy.run_path and test-native-cache-key.py reaches for importlib.util.spec_from_file_location, for the same "this filename has a hyphen" problem, and then line 119 of the test goes back to runpy. Worth picking one so the next person copying either file gets a consistent answer.

digest and command have docstrings about as long as their bodies. The longer ones further down earn their keep, particularly the note on why the environment stays in the fallback prefix.

One question about shape

Since #5973 is still open and this is waiting on it, is there a case for landing the composite on its own first? The extraction, the CARGO_HOME fix and deleting the read-write Linux-cargo-registry-* cache from the linux-test matrix all stand alone, and that last one only ever ran in a job with skip-native-build: true that never calls cargo, so it is pure budget back at a moment when we are short of budget. The composite could carry the existing hashFiles key at first and take the fingerprint in a follow-up once main is actually retaining entries.

Happy to be told the double churn on the composite's key is not worth it.

---

For what it is worth, on the things I could check locally the fingerprint looks complete. The six new tests pass, check-ci-config.py and the Iceberg shard tests still pass, and on the real tree 297 of the 399 tracked files under native/ land in the CI key with the other 102 being exactly the 6 markdown files and the 96 under benches/. There is no include_str! or include_bytes! anywhere in native/, so excluding markdown is safe, and there is no [patch] section or path dependency outside native/ and contrib/. I walked the six step conditions in the composite by hand, including the case where steps.cargo-cache is skipped and cache-hit evaluates to the empty string, and did not find a hole.

[sunchao]

Thanks, Andy. Addressed this pass in 347fb07 and updated the description and workflow documentation.

Caller workflows and environment. The four caller files are out of the fingerprint. The shared setup/build actions remain included, and the observed environment now captures Cargo/Rust controls, compiler/linker/protobuf overrides, target-qualified compiler variables, and the HDFS controls used by our existing dependencies. For example, a shard edit preserves the key, while CARGO_PROFILE_CI_OPT_LEVEL, CC_x86_64_unknown_linux_gnu, or HDFS_LIB_DIR changes invalidate it. The documentation keeps the scope explicit: this describes our official builder; recording a path to an arbitrary external tool or library does not identify its contents.

One small clarification on the earlier behavior: a caller edit invalidated the finished-library key, but it preserved the incremental restore prefix when dependencies and environment were unchanged, so compilation was required without necessarily being cold.

Flags and routing. RUSTFLAGS is now defined once through GITHUB_ENV, before fingerprinting and compilation. The test file is removed from the explicit consumer loop. It still runs in Preflight and retains existing Linux routing, but test-only edits no longer select the extra Spark/Iceberg suites on the merge queue or nightly tier. The actual cache recipes remain in that loop.

Cache size and sequencing. The description now explicitly says that fixing CARGO_HOME grows the incremental entry by adding registry/Git contents, separately from the new finished-library entry. The first-main validation calls for reporting both compressed cache sizes from the save logs or cache API, then observing an exact library hit and passing downstream tests. Those measurements are still pending main publication. #5973 remains an explicit prerequisite, followed by rebasing this PR before merge. I would keep this PR together for now: the extraction and reuse behavior share one recipe, and splitting it would add another transition without removing the cache-retention prerequisite.

Smaller cleanup. Both files now use the same importlib loading idiom, the test reuses the already-loaded matcher, and the two short helper docstrings are removed. The longer contract explanations remain.

Validation: the existing six cache-key tests pass with expanded caller/environment/routing coverage; all 15 Iceberg shard tests, CI configuration and suite checks, benchmark-runner checks, actionlint, Markdown formatting, and whitespace checks pass. Independent review found no further issues. Hosted CI for this new commit is pending; the previous head's selected checks all passed.

[comphead]

Thanks @sunchao that makes sense to me as a direction, I came across the same yesterday and then realized you already have a PR.

Let me spin up an automated review process

[viirya]

Reviewed at bd4ee8a14bfd42fe7a35e6219985f667936c3bc9, including the previous review discussions and author responses.

The earlier concerns appear addressed: the fingerprint and routing now share their input definitions and matcher, disabled contrib sources no longer invalidate the library, caller workflow edits are excluded while the build environment is observed, and RUSTFLAGS has a single definition. Keeping the JDK/package boundary in the incremental restore prefix is justified by the native dependencies’ external inputs. The prerequisite #5973 has also landed and is included in this branch.

I did not find a blocking correctness issue in the current implementation. Only an exact library hit skips Cargo, main still builds after a lookup-only hit, and the existing artifact paths and downstream JVM packaging remain consistent. The independent Rust checks and tests continue to run.

I reran the six cache-key tests, CI configuration checks, and fifteen Iceberg shard tests successfully. The hosted checks for this head also passed, although the native job exercised a cache miss. Actual cross-run library reuse, retention, and turnaround improvements therefore remain to be verified as documented.

Two non-blocking suggestions below concern coverage of the action’s control flow and including the debug cache in the size measurements.

[comphead]

Review: reuse Linux native libraries across workflow runs

Solid design and the rationale holds up where I checked it. No correctness blocker found.

Verified while reviewing

• All four callers run the native build in the same amd64/rust container at JDK 17 (pr_build_linux.yml:364, spark_sql_test_reusable.yml:83 with every ci.yml call passing java: 17, iceberg_spark_test_reusable.yml:78, spark_sql_writer_tests.yml resolving to 17), so one key really is reachable from all of them.
• No native build input escapes the fingerprint. native/core/build.rs and native/proto/build.rs read nothing outside native/, there is no include_str!/include_bytes! reaching out, all seven .proto files live under native/proto/src/proto/, and the only path dependencies outside the workspace are contrib/{delta,lance}/native, both covered by contrib/*/native/Cargo.toml. The JAVA_HOME handling in core/build.rs confirms the JDK-in-key rationale.
• native/target/ci/libcomet.so is the only build output any consumer needs, so restoring just that file is sufficient for the artifact upload and for the writer workflow's stage-to-release step.
• The importlib load writes dev/ci/__pycache__, which is gitignored, and check-working-tree-clean.sh runs only in lint, so the helper cannot dirty a checked tree.
• Ran the new suite locally: 6 tests pass in 0.8s. check-ci-config.py passes with the dev/ci part of the diff applied.

Findings (inline)

Major - main rebuilds on a double cache hit, which is the common main push. The library key takes the whole dpkg database, which shortens entry life for packages that cannot affect libcomet.so.

Minor - source_inputs discards the blob OID that git ls-files --stage already gives it and then re-hashes every file, in two passes, with a whole-changeset predicate called per file. Two of the three NATIVE_CACHE_RECIPES routes added to build_linux are already covered by its dev/ci/**. compute() hardcodes the push tier outside POLICY. binary-key="" is dead output. Two tests assert one property many times. The README section carries a one-off verification checklist.

I did not find anything to move to SQL-file tests: this change has no expression or operator surface.

[comphead]

Three things collapse here.

1. git ls-files --stage already emits the content hash. metadata.split() is [mode, oid, stage], and this keeps [0] and throws away [1], then re-reads and SHA-256s every file. Keying on mode + oid removes all the file I/O and the FileNotFoundError that an index entry with no worktree file would raise. The docstring already scopes this to a clean checkout, and untracked generated files never appear in ls-files, so hashing the working tree buys nothing the index does not already give.

2. dependencies on L68 is a second pass over the dict just built. Both maps fit in the one loop.

3. CHANGES.matches() is a whole-changeset predicate being invoked once per file, so it rebuilds the compiled include/exclude lists on every call. Sharing the semantics with compute-changes.py is the right instinct, but the reusable unit is the matcher, not the any-file wrapper. A compile_matcher(patterns) there that returns a predicate, with matches() calling it too, gives the same guarantee without the per-file rebuild.

For calibration, I measured this on the current tree: 309 files and 5.2 MB, 0.15s. So this is about single-traversal clarity, not runtime.

[sunchao]

The index-OID approach is reasonable under the clean-checkout contract. I have kept this part unchanged in the focused revision: hashing working-tree bytes directly describes the files Cargo sees, and the measured 0.15s does not justify changing that behavior or adding a matcher API here. The dependency comprehension is also small and readable. I did simplify the separate routing test so it no longer scans and hashes the repository inventory. Leaving this thread open since the helper refactor is deferred.

[sunchao]

Implemented the single-traversal and reusable-matcher parts in de25d50. source_inputs now compiles the shared matcher once and builds both maps in one inventory loop; compute-changes uses the same predicate. I retained working-tree byte hashing so the key describes the bytes Cargo reads, including any tracked file changed by setup. The measured cost is small, and switching to index OIDs would weaken that property. Leaving the remaining OID suggestion open for your assessment.

[comphead]

Major. The library key takes the entire dpkg database. amd64/rust is an unpinned latest tag that the runner re-pulls per job, and setup-builder runs apt-get update before installing, so any archive refresh discards a finished library over packages that cannot affect libcomet.so: tzdata, ca-certificates, git, imagemagick and the rest of the buildpack-deps base.

The stated rationale is the C/JNI boundary in hdfs-sys and core/build.rs, and that needs only the toolchain: clang*, gcc*, binutils, libc6-dev, libstdc++*, protobuf-compiler. Restricting dpkg-query -W to those keeps the invariant you actually depend on and materially extends how long an entry stays usable, which is the thing this PR is buying. Worth folding into the hit-rate measurement you already planned rather than deferring it, since it decides whether the reuse pays off at all.

Same question, smaller, for java_release on L93: the full release file rotates on every Zulu 17 patch, while the JNI headers it stands in for essentially never change.

[sunchao]

Unrelated installed-package changes can indeed cause conservative misses. I retained the current boundary because the suggested package list does not cover the full compiler dependency set. In the official Rust image I inspected, GCC's cc1 also links libisl23, libmpc3, libmpfr6, libgmp10, zlib1g and libzstd1. GCC constrains these with minimum versions, so they can change while the proposed whitelisted package versions remain unchanged. Narrowing this safely needs the complete set of relevant toolchain dependencies. Also, apt-get update alone changes repository indexes, not the installed versions this helper hashes.

I retained the JDK identity too: default HDFS builds against its JNI headers and links libjvm. Removing java_release alone would still leave patch-specific JAVA_HOME and PATH values in the key. The PR's rollout plan now explicitly calls for tracking package/JDK changes behind misses, alongside sizes, retention and hit behavior. Leaving this open rather than claiming the proposed narrowing is implemented.

[sunchao]

I retained the conservative package/JDK boundary for the reasons in my earlier reply. The updated README and PR description explicitly document conservative misses and matching producer environments; preflight now rejects producer JDK/image/environment drift. The rollout still requires measuring actual hits and package/JDK changes behind misses. I have not claimed that the narrower dependency set is implemented, and am leaving that design choice open for review.

[andygrove]

Thanks for the revision. The rebase onto #5973 is in and all 38 checks are green, so the sequencing prerequisite is satisfied. I re-queried the cache API and it confirms #5973 worked: 8 entries, 8.58 GB, everything on refs/heads/main except two TPC dataset entries on a PR merge ref. That is a completely different picture from the 17.34 GB with nothing on main that I reported on Monday.

Before the concerns, the things I checked rather than took on trust, since a couple of them were my own earlier worries. The RUSTFLAGS pin genuinely works: JobExtension.cs applies workflow- and job-level env: once into Global.EnvironmentVariables at job init, the GITHUB_ENV file command mutates that same dictionary, and StepsRunner rebuilds each step's env from it, so the composite's write beats the callers' workflow-level value and the fingerprint step reads exactly what the build gets. check_cache_save_scope does scan .github/actions/*/action.yaml, so #5973's guard covers both new save steps. All four callers have byte-identical env: blocks and every one passes java: 17, so nothing fragments the key across workflows today. And there are no submodules, no include_str! or include_bytes! under native/, no git-SHA stamping, and the three tracked symlinks fall outside the matched patterns. The helper resolves 307 files for the CI key and 405 for debug, the extra 98 being exactly the benches. Six tests and check-ci-config.py pass locally.

Two things I would like to work through, and they are connected.

The 23-minute figure is pre-#5973

I measured the warm case. PR #5991 started at 22:05Z on the 16th and this PR at 22:12Z, seven minutes apart against the same cache state. #5991 changed three native Rust files and its Build Native Library took 4m39s. This PR's took 29m44s, because -v3- is a fresh namespace with nothing to restore. On the nightly against main the next morning, PR Build (Linux) / Build Native Library was 4.2 minutes and the three Iceberg builders 7.6 to 7.8.

So the step this PR removes now costs about 4.2 minutes, not 23. Worth noting too that binary-key and source-key are both functions of the same (environment, sources) pair, so the binary cache hits in exactly the cases the cargo cache would already have exact-hit. What it really buys is skipping a 1.4 GB download and a no-op Cargo revalidation in exchange for ~30 MB, plus a small entry surviving eviction better than a 2 GB one. That is a genuine 2 to 3 minutes per native-building job and I think it is still worth having, but could the description be re-based on the current numbers? Right now it leads with a saving its own prerequisite already delivered, and the CI-status paragraph still says hosted CI for the rebased head is pending.

Could the registry and git paths come out rather than be repointed?

Linux-cargo-debug-* is 4131 MB today and Linux-cargo-ci-* is 1439 MB, and both hold native/target alone, since ~/.cargo does not exist in amd64/rust. Those are the same entries producing the 4.2-minute builds above, which I think is the evidence that the registry is not needed for the target cache to work: cargo re-fetches it from crates.io in well under a minute, and registry deps are fingerprinted by package id rather than source mtime, so re-extracting them does not force a recompile. Pointing at /usr/local/cargo adds the registry and the iceberg-rust checkout to both entries, on the budget #5973 just recovered. Deleting the two dead path lines fixes the same wart at no cost.

That also softens the transition. Linux-cargo-ci- to -v3- and Linux-cargo-debug- to -v3- orphan 5.5 GB that stays resident for its 7-day window while the new entries are written alongside, and we both know what happens to main's entries when this repo goes over. Without the registry the new entries are the same size as the old, so the peak is around 11 GB instead of 15.

Either way, could we agree what happens to the superseded entries at merge? Deleting them through the cache API keeps the budget flat but sends every unrebased PR cold; leaving them doubles the cargo footprint for a week. I lean towards deleting, given how the eviction went last time, but it is worth being deliberate about rather than discovering it.

One last question on shape

#5841 is still open and shares one native build across workflows within a run, which would remove the duplicate Iceberg and Spark SQL native builds. That is where most of the remaining per-run saving lives now that the warm build is 4.2 minutes. How do you see these two composing, and does the order they land in matter?

---

For the record, two things from my earlier passes that I am dropping. Main's warmer runs on nearly every push, since build_linux matches spark/** and common/** and not just the native inputs, so an environment drift that invalidates the whole key self-heals within hours rather than waiting for a native change. And the workflow-level RUSTFLAGS in the four callers is now shadowed from the composite onwards, which is harmless given the precedence result above.

[sunchao]

Following up on your latest review.

Thanks, Andy. I addressed the two connected concerns in 80296cb7b and revised the description around the post-#5973 baseline.

Both CI and debug caches now contain only native/target; I removed the registry/git paths and the unused cargo-home output. The effective Cargo home still participates in the environment fingerprint. I checked the newer #5991 native job: it restored the target archive, fetched registry/git sources, and rebuilt only six Comet workspace crates. A separate disposable Cargo build with registry and Git dependencies also kept all compiled dependencies fresh after their source directories were deleted and fetched again.

The description now uses that job's 3m51s and the nightly Linux job's 4m13s, including 43–47s restoring the target cache and 1m57s–2m22s in Cargo. Both were fallback restores with workspace recompilation, so I explicitly distinguish them from the exact-library-hit path. The historical 23-minute cold build is no longer presented as the expected saving. Actual library-hit and end-to-end savings remain to be measured after main publishes an entry.

For migration, I agree with deliberate deletion: the description calls for inventorying and deleting the superseded legacy Cargo cache IDs on refs/heads/main at merge time, preserving the new namespaces and unrelated caches. The first main build will populate the new caches from cold. This avoids keeping both large generations resident, with the explicit tradeoff that PRs using old keys lose their warm cache and need to rebase. No live caches were deleted as part of this update.

For #5841, this PR reuses a library across runs; #5841 shares the producer within a run. Together, one producer restores or builds and then distributes the library. There is no functional ordering requirement, but the planned integration is to land this PR first, then rebase #5841 and use this composite action in its shared producer, preserving main-only writes. Ordinary unlabeled PRs already have one covered producer, so #5841 mainly removes duplicates in queue/nightly/manual runs.

The six fingerprint tests, action-flow tests, CI configuration checks, actionlint, Markdown formatting and whitespace checks pass locally. Hosted CI for this new head is pending; the description keeps that separate from the earlier green head and the still-unmeasured library-hit path.

[viirya]

Re-reviewed at 80296cb7bcddcd539fbf80e6cdeae67ba64ab877, including the changes since my previous review and the subsequent discussion. No new blocking issues found.

The main double-hit optimization looks correct: main still restores the target cache, which supplies libcomet.so for artifact upload, and skips Cargo only when both entries match exactly. A missing entry still triggers a build to replenish it.

Keeping the incremental caches target-only is a reasonable storage/network tradeoff. I checked the referenced #5991 log: Cargo fetched dependency sources and rebuilt only the six Comet workspace crates. The revised warm-build baseline and explicit migration plan also address the concerns about overstating savings and retaining both cache generations.

Both of my previous suggestions are addressed. The action-flow tests now cover the build/save decisions, and the rollout plan includes all three cache sizes.

On the remaining review discussion, I favor retaining the conservative package/JDK fingerprint for now. Narrowing it safely requires accounting for the relevant toolchain dependencies, not just the compiler packages themselves. This can reduce reuse after unrelated package updates, so tracking the causes of misses during rollout remains important. Keeping a single RUSTFLAGS definition also seems reasonable for the current producer jobs, whose downstream steps do not invoke Cargo.

The six fingerprint tests, action-flow test covering twelve scenarios, CI configuration checks, and fifteen Iceberg shard tests pass locally. Hosted CI for this head has also passed, so the description’s pending-status note can be updated. Actual library-hit behavior, retention, and end-to-end savings still need the documented rollout verification.

I remain comfortable approving this revision. That is my assessment of the remaining tradeoffs; it does not imply that the other reviewers’ open threads are settled.

[andygrove]

Thanks for the revision. I re-read the whole thing at 80296cb7b and worked through the two new commits rather than just the diff.

The main double-hit skip in 1dc4bf318 is correct as far as I can tell. I walked all five step conditions plus lookup-only by hand across twelve combinations, separately from your new test, and did not find a hole. The property that makes it safe is that the incremental entry is only ever written after a successful --profile ci build, so on the double-hit path the restored native/target genuinely does carry ci/libcomet.so for the artifact upload. test-native-cache-workflow.py is also a faithful model of what the runner does here: GitHub's &&/|| precedence matches bash [[ ]], shlex.quote leaves every value in the table unquoted except the empty string, and no context name is a prefix of another. Reading the conditions out of the action itself rather than restating them is the right shape.

Taking the registry and git paths out entirely in 80296cb7b is better than what I asked for. And on the fingerprint I re-resolved the inventory on the current tree: 307 files in the CI key, 405 in the debug key, the difference being exactly the 98 benches. No tracked symlink falls inside either pattern set, so read_bytes() cannot follow a dangling link. No include_str! or include_bytes! under native/, so excluding markdown is safe. All four callers run in amd64/rust, all run setup-builder before the composite so JAVA_HOME is always set, and all resolve to JDK 17. I also checked that no caller needs anything from native/target beyond ci/libcomet.so: setup-spark-builder with skip-native-build: true runs mvnw install and never calls cargo.

Two things, and one small one.

Only one of the four converted workflows has ever run the composite

I pulled the job list for each of 80296cb7b, bd4ee8a14 and e3b9e8542, and in every one the only job matching "native" is PR Build (Linux) / Build Native Library. spark_4_1 sits behind ['queue', 'label:run-spark-4.1-tests'] and the Iceberg jobs behind label:run-iceberg-tests, and this PR carries neither label, so the Spark SQL and Iceberg producers would first execute in the merge queue. spark_sql_writer_tests.yml is workflow_dispatch only, so it will not run at all until someone dispatches it.

That matters more here than it usually would, because those three jobs differ from the tested one in exactly the places this PR touches. The Spark SQL and writer jobs run setup-spark-builder and SBT after the composite, which is where the GITHUB_ENV RUSTFLAGS widening lands. The Iceberg job puts its python step between setup-builder and the composite rather than before both. None of that looks wrong to me on reading, but it is the kind of thing where reading is weaker evidence than one green run, and a composite failure discovered in the merge queue blocks everyone.

Could we put run-spark-4.1-tests and run-iceberg-tests on this PR for one run before merging, and dispatch spark_sql_writer_tests.yml against the branch once?

dev/ci/compute-changes.py in the library key is all churn and no signal

NATIVE_CACHE_RECIPES hashes the whole file into the library key and the incremental prefix, but the native build never reads it. It is in the key because native-cache-key.py imports NATIVE_LIBRARY_INPUTS, NATIVE_BUILD_INPUTS and matches from it. Everything else in those 650 lines is FILTERS, POLICY and the tier logic.

I counted it the same way as last time. 18 commits touched that file on main in the last 90 days, and git log -L :glob_to_regex:dev/ci/compute-changes.py over the same window returns zero. Reading all 18, every one is routing or policy work: the tier moves in #5963, #5939, #5926, #5930, #5843 and #5871, the Spark bumps in #5181, #5182 and #5183, filter additions in #5881, #5852, #5782 and #4777, Iceberg sharding in #5459 and #4840, and the policy move in #5850. Not one changed the pattern tuples or the matcher. That makes it the highest-churn library-key input outside native/ itself, against setup-builder at 1 commit and rust-toolchain.toml at 0. It is the same shape as the caller workflows you dropped last round, and since 1dc4bf318 it has a second cost: the file is in the consumer filter loop, so a routing edit now selects spark_4_1, spark_4_1_hive and iceberg_1_11 on the merge queue, which is the thing you removed for test-native-cache-key.py.

Could the fingerprint take the imported values instead of the file? Something like

"matcher": [CHANGES.NATIVE_BUILD_INPUTS, CHANGES.NATIVE_LIBRARY_INPUTS,
            inspect.getsource(CHANGES.glob_to_regex), inspect.getsource(CHANGES.matches)],

and then drop dev/ci/compute-changes.py from NATIVE_CACHE_RECIPES. That is strictly more precise in both directions: a pattern or matcher edit still invalidates, a comment or an unrelated FILTERS edit no longer does. I checked main's warmer is unaffected, since FILTERS["build_linux"] already carries dev/ci/** and a push editing only that file still selects build_linux.

One small thing

80296cb7b removed the cargo-home output, so the environment_inputs stub at dev/ci/test-native-cache-key.py:208 returning {"cargo_home": ...} now names something nothing reads. It still passes because cache_keys only digests the value, but {} would say what it means.

---

For the record, two things I looked at and am not raising. Main's double-hit push still downloads the roughly 1.4 GB incremental archive even though it no longer runs cargo, and a lookup-only probe before the library restore would let the two swap roles. I do not think that is worth it: it saves about 45 seconds on main only, it adds a fourth cache step to the one control flow where a condition bug produces a wrong library rather than a slow build, and restoring the entry is what keeps its 7-day access window alive. And PATH in the environment sweep is redundant with java_home rather than a source of fragmentation, since the only thing that touches PATH before the composite is actions/setup-java and all four callers install Zulu 17.

[viirya]

Took another pass at 80296cb7b, this time focusing on how the change interacts with code outside the diff. My earlier approval stands for the core design. Three things turned up that I think are worth handling before merge, left inline. The first one I reproduced locally.

I'd also +1 Andy's latest review on two points. Running the label-gated producers once before merging still seems important, since on this head Spark SQL Tests (*) and Iceberg Spark SQL Tests (*) are skipped and spark_sql_writer_tests.yml has not been dispatched. And keying on the imported pattern tuples and matcher rather than the whole compute-changes.py file would avoid invalidating the library on routing-only edits.

[comphead]

doing the second review

[comphead]

Second pass at 80296cb7b, focused on what changed around the PR since the last round.

Rebase. The branch is 59 commits behind main and now conflicts in dev/ci/check-ci-config.py, where #5881 widened the maven-bootstrap row to MVN_JOBS. Keeping main's row plus the two new rows resolves it, and on that merged tree check-ci-config.py and both new test files pass. +1 to Andy's request to run the label-gated producers once, ideally after the rebase.

dev/local-ci.sh. #5974 merged after this head was pushed. The script says it mirrors the Spark SQL and Iceberg workflows, but its build_native() still runs cargo build --profile ci. Adding --locked there makes a local replay fail on a stale native/Cargo.lock the same way CI now does.

Checked, no change needed.

• All four producers check out Comet at the default path, so workspace in the fingerprint does not split the key across callers.
• A failed library restore falls back to a build. @actions/cache turns non-validation restore errors into a warning and leaves cache-hit unset.
• The two workflows that run in main's cache scope on behalf of a pull request, label_prs.yml (pull_request_target) and take.yml (issue_comment), never execute pull-request code. So I don't see a way for a fork to pre-seed a Linux-native-ci-v2-* entry.

Follow-up question, not for this PR. Was apache/infrastructure-actions/stash considered for the two native/target entries? It is ASF infra's alternative to actions/cache for large build caches, and apache/* actions need no allowlist review. Each stash is a workflow artifact, so it does not count against the 10 GB cache budget and is never evicted. Its lookup order matches actions/cache, and merge-queue runs fall back to the default branch. There are two catches. It has no prefix restore-keys (the newest stash for a key wins), so the key would be today's restore prefix and main's exact-hit skip would need another signal. Expiry also counts from upload rather than last use, so it would want a longer retention-days than the 5-day default. The ~30 MB library could stay in actions/cache while the ~1.4 GB CI and ~4.1 GB debug entries move out of the shared budget.

Inline: one suggestion that removes the push-only routing override and closes viirya's --locked gap, plus two small cleanups.

[comphead]

Thanks @sunchao it is lgtm with some minor nits

[sunchao]

Summary

• Prior state and problem: Four Linux workflows repeated native cache/build steps, restoring compiler outputs and invoking Cargo even when native inputs were unchanged.
• Design approach: A shared composite restores an exactly fingerprinted libcomet.so, falling back to an incremental cache and cargo build --locked --profile ci.
• Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The existing contrib-manifest routing blocker remains reproducible. Native build flags, artifact staging and JVM packaging remain consistent. Spark plugin-loading sources for 3.4.3, 3.5.9, 4.0.4 and 4.1.3 introduce no additional compatibility concern for this CI-only change.
• Key design decisions: Main alone publishes caches. Library reuse requires an exact match. Incremental caches retain the environment boundary, and Rust tests retain a separate debug cache. Main skips compilation only when both cache entries match exactly.
• Implementation sketch: The helper fingerprints tracked native inputs and observed tools/environment. Shared input patterns connect fingerprinting to main’s cache routing. The composite removes duplicated build sequences without adding a separate artifact-distribution mechanism.
• Behavioral changes worth calling out: --locked now rejects stale lockfiles. Target-only caches require dependency-source downloads when Cargo runs. Exact library hits avoid target restoration and compilation for consumers, but hosted hit-path savings and retention remain unmeasured.
• Suggested improvements: Resolve the existing routing concern before merge by ensuring contrib manifests receive locked validation on PR and merge-queue runs. In a disposable copy, Linux-targeted cargo metadata --locked --offline passed initially, then failed after changing only the disabled Lance crate’s package version. The same change passed without --locked. Routing currently selects no PR or queue job for that file, allowing the first failure to occur on main and affect subsequent native builds. This is an existing P2 blocker, not a new inline finding.

Reviewed the complete 12-file diff from 20faa2346b56b952d3d8462229986722a61fce45 to 80296cb7bcddcd539fbf80e6cdeae67ba64ab877. Used .ai/skills/review-comet-pr/SKILL.md, read AGENTS.md, and checked sibling skills. None additionally applies to this CI scope. Read existing reviews, comments and threads, excluding Copilot.

Exact-head CI: 24 checks succeeded and 14 were skipped. Required Checks, native compilation, Rust tests and selected Spark 4.1 JVM suites passed. The native log confirms cache misses followed by successful compilation. Spark SQL and Iceberg workflows were skipped, and the manual writer workflow has no run at this head.

Local validation passed: six fingerprint tests, twelve action-flow scenarios, CI configuration checks, fifteen Iceberg shard tests, actionlint and whitespace checks. No full native/JVM build was run locally. Hosted cache hits, main’s publication path and end-to-end performance remain rollout validation limits. The project working tree is unchanged.

[andygrove]

I checked the rebase with git range-diff against 80296cb7b. The PR's own changed lines are byte-identical, and the only differences are context from #6155 in ci.yml and #5881's MVN_JOBS row in check-ci-config.py, resolved the way comphead suggested. At 1c8698c4d the four dev/ci/test-*.py files, check-ci-config.py and actionlint all pass locally, and the branch merges cleanly with #6218 with the same checks still passing.

So what was open at 80296cb7b is still open, and none of the threads from my last review, viirya's or comphead's has a reply yet. The one I'd most like settled before this merges is viirya's contrib/*/native/Cargo.toml routing thread, which your automated review above also calls a blocker. At this head a change to only contrib/lance/native/Cargo.toml still selects nothing on pull_request or merge_group, and build_linux on push, so an edit that needs a lockfile update would pass both tiers and then fail --locked on main. comphead's FILTERS["build_linux"].extend(NATIVE_BUILD_INPUTS) fixes that and drops the push-only override in compute(). It would also make #5841's rebase safer. #5841 derives build_linux_native from the other outputs in compute(), and if the override lands after that union, a push that changes only a contrib manifest selects build_linux without the producer that writes the cache. test_native_input_routing asserts only build_linux, so it would stay green.

The Spark SQL and Iceberg producers still haven't run the composite. At this head the only native job was PR Build (Linux) / Build Native Library, 28 minutes of cold build because nothing on main has the -v3- keys yet, and every Spark SQL and Iceberg job was skipped. I'd like one label run with run-spark-4.1-tests and run-iceberg-tests to exercise spark_sql_test_reusable.yml and iceberg_spark_test_reusable.yml before this lands, so I'll add those labels. I'll drop my ask to dispatch spark_sql_writer_tests.yml, since it has never been dispatched on main either.

[sunchao]

Summary

• Prior state and problem: Four Linux workflows repeated native build steps and restored compiler outputs even when native inputs were unchanged.
• Design approach: A shared composite restores an exactly fingerprinted libcomet.so, falling back to the incremental cache and cargo build --locked --profile ci.
• Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The existing lockfile-routing blocker remains reproducible. Native flags, artifact staging and JVM packaging remain consistent. Checked Spark plugin-loading sources for supported versions 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. This CI change introduces no additional Spark compatibility concern.
• Key design decisions: Only pushes to main publish caches. Library reuse requires an exact match. Incremental keys retain the toolchain/package/JDK boundary. Rust checks retain their separate debug cache, and main skips compilation only when both entries match exactly.
• Implementation sketch: The helper fingerprints tracked native inputs before source generation and observes the build environment. Shared patterns and matching logic connect fingerprinting to main’s routing. One composite replaces four repeated build sequences while preserving artifact distribution.
• Behavioral changes worth calling out: --locked rejects stale lockfiles. Target-only caches trade dependency-source downloads for reduced cache storage. Exact library hits can avoid target restoration and compilation, but hosted savings and retention remain unmeasured. Linux, Spark SQL and Iceberg producer logs emitted the same library key at this head.
• Suggested improvements: Resolve the existing P2 routing concern before merge by routing contrib manifests through locked validation on PR and merge-queue runs. In a disposable copy of this head, Linux-targeted cargo metadata --locked --offline passed initially, failed with exit 101 after changing only the Lance package version, and passed without --locked. That file selects no PR or queue job, but selects build_linux on push. A stale lockfile can therefore reach main and break subsequent native builds. No duplicate inline finding is added.

Reviewed the entire 12-file diff from 14f0f59f74dbcf273ff3cbdb1721a5ee535bbe84 to 1c8698c4d127a3c0d33662522b37c6c285666859. Confirmed the PR is not a draft. Read AGENTS.md, existing reviews, issue comments, inline comments and threads. Routed through .ai/skills/review-comet-pr/SKILL.md; no subsystem sibling skill applies to this CI-only scope.

Exact-head CI: 69 checks succeeded and 40 were skipped, with no failures or pending checks. The regular CI run, Spark 4.1 label run, and Iceberg label run passed. These exercise the Linux, Spark SQL and Iceberg producers, addressing the earlier untested-producer concern. Inspected producer logs confirm cache misses followed by successful locked compilation.

Local validation passed: six fingerprint tests, twelve action-flow scenarios, CI configuration checks, fifteen Iceberg shard tests, six report-summary tests, actionlint and whitespace checks. No full native/JVM build ran locally. Hosted library-hit behavior, main’s publication path, cache retention and end-to-end performance remain unverified. The manual writer workflow has no exact-head run. The project working tree is unchanged.

[andygrove]

The label runs I added have finished, and they settle the producer question. I pulled the logs for all six producers at 1c8698c4d: PR Build (Linux) / Build Native Library, the Spark 4.1 Build Native + JVM Test Classes job, and the four Iceberg Build Native Library jobs. All six emitted the same binary-key, source-key and restore-prefix on the same amd64/rust digest, with the Linux job and the label runs four and a half hours apart. So the Spark SQL and Iceberg producers will pick up main's entry, and I'm closing that ask.

The rest of my last comment still stands, and those threads still have no reply. On viirya's routing thread, here is one more reason I'd like it fixed before merge. contrib/delta/native/ has its own Cargo.lock, so a Delta dependency bump made from inside that directory updates that lockfile and not native/Cargo.lock. At this head the queue only runs delta_gate for a Delta manifest change, and none of its cargo tree or cargo build calls use --locked.

The "How are these changes tested?" section still describes 80296cb7b with hosted validation pending. Could it be updated for this head and the label runs?

[sunchao]

Summary

• Prior state and problem: Four Linux workflows repeated native build steps and restored compiler outputs even when native inputs were unchanged.
• Design approach: A shared composite restores an exactly fingerprinted libcomet.so, falling back to an incremental cache and cargo build --locked --profile ci.
• Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The existing lockfile-routing P2 remains reproducible. Native flags, artifact staging and JVM packaging remain consistent. Checked Spark plugin-loading sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 without finding an additional compatibility issue.
• Key design decisions: Only pushes to main publish caches. Library reuse requires an exact match. Incremental caches retain the environment boundary, Rust tests use a separate debug cache, and main skips compilation only when both entries match exactly.
• Implementation sketch: The helper fingerprints tracked inputs before source generation and observes the build environment. Shared patterns connect fingerprinting to producer routing. The composite removes duplicated build sequences while retaining existing artifact distribution.
• Behavioral changes worth calling out: --locked rejects stale lockfiles. Target-only caches require dependency-source downloads when Cargo runs. Exact library hits can avoid target restoration and compilation. Conservative environment changes can cause misses, and hosted savings remain unmeasured.
• Suggested improvements: Resolve the existing routing blocker in dev/ci/compute-changes.py:597 by ensuring contrib manifests receive locked validation before merging. A Lance manifest-only edit selects no native validation on PR or queue events, but selects build_linux on push. In a disposable copy, cargo metadata --locked --offline --filter-platform x86_64-unknown-linux-gnu --format-version 1 passed initially, failed with exit 101 after changing only Lance’s package version from 1.1.0 to 1.1.1, and passed without --locked. Such an edit can reach main and break subsequent native builds. No duplicate inline finding is added.

Reviewed the entire 12-file diff from 14f0f59f74dbcf273ff3cbdb1721a5ee535bbe84 to 1c8698c4d127a3c0d33662522b37c6c285666859. Confirmed non-draft status. Read AGENTS.md and existing reviews, issue comments, inline comments and threads, excluding Copilot. Used .ai/skills/review-comet-pr/SKILL.md; no sibling skill applies to this CI-only scope.

Exact-head CI: 69 checks succeeded and 40 were skipped, with none failed or pending. The regular CI run, Spark 4.1 label run, and Iceberg label run passed. Inspected Linux, Spark SQL and Iceberg producer logs show identical cache keys and successful locked compilation after misses.

Local validation passed: six fingerprint tests, twelve action-flow scenarios, CI configuration checks, fifteen Iceberg shard tests, six report-summary tests, actionlint and whitespace checks. No full native/JVM build ran locally. Hosted library hits, main’s publication path, retention and end-to-end performance remain unverified. The manual writer workflow has no exact-head run. Project files remain unchanged.

[sunchao]

Updated in de25d50, rebased onto main at 65b334b.

• Closed the pre-merge --locked routing gap and added Cargo dep-info and producer-environment guards.
• Fingerprinting now hashes the imported input rules/matcher separately from unrelated routing policy. The helper compiles the matcher once and builds both input maps in one pass.
• Scoped RUSTFLAGS to fingerprint/build steps, aligned cache-key names, removed redundant fields/outputs, and added --locked to local CI.

All 22 focused native-cache/configuration tests pass, along with the existing CI configuration/suite, Iceberg, label, benchmark, actionlint, shell and whitespace checks. Small real Cargo builds validated dependency-file handling; real Comet metadata reproduced the stale-lockfile failure. No full Comet native/JVM build ran locally.

The PR description now records the successful previous-head Linux/Spark/Iceberg runs and distinguishes them from new-head CI, currently running. The test labels remain applied.

Replied to all 11 open inline threads and resolved the nine implemented requests. The working-tree-hash and conservative package/JDK-hash discussions remain open with rationale. Main publication, actual library hits, entry retention and end-to-end savings still need rollout measurements.

[sunchao]

Summary

• Prior state and problem: Four workflows repeated native build steps and invoked Cargo even when native inputs were unchanged.
• Design approach: A shared composite restores an exactly fingerprinted libcomet.so, falling back to incremental outputs and cargo build --locked --profile ci.
• Correctness / compatibility analysis: Found one introduced P1: the post-build dependency check fails under container checkout ownership differences. The previous contrib-manifest routing blocker is fixed. Checked artifact staging, Comet library loading, and Spark plugin-loading sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 without finding another compatibility issue.
• Key design decisions: Only pushes to main publish caches. Consumers skip Cargo only on exact library hits. Main requires both entries to match before skipping compilation. Rust tests retain their separate cache and still run.
• Implementation sketch: Shared input patterns and observed builder identity determine cache keys. One composite replaces four repeated sequences. Preflight checks constrain producer environments, while the new dependency check verifies file coverage.
• Behavioral changes worth calling out: --locked rejects stale lockfiles. Target-only caches require dependency-source downloads when Cargo runs. Exact library hits avoid compilation and target-cache restoration, but actual savings and retention remain unmeasured. Conservative package/JDK fingerprints can cause additional misses.
• Suggested improvements: Fix the repository-root handling described below and add regression coverage combining the native/ working directory with different checkout ownership. No additional introduced P1/P2 issues found.

Reviewed the entire 14-file diff from 65b334bfdd25196091a42d773bc3e61d19212e52 to de25d50ff2c7fd58769917dc5c8249f9a05f601a. Confirmed non-draft status. Read AGENTS.md, existing reviews, issue comments, inline comments and threads, excluding Copilot. Used .ai/skills/review-comet-pr/SKILL.md; no subsystem sibling skill applies to this CI-only scope.

Exact-head CI: 17 checks succeeded, 11 failed and 26 were skipped. In run 36489698536, all six native producers finished Cargo compilation, then failed on the same Git ownership error. Downstream native-dependent tests could not run, four Iceberg coverage checks failed, and Required Checks is red. Preflight and Rust tests passed.

Local validation passed: all 22 native-cache/configuration tests, CI configuration and suite checks, 21 Iceberg sharding/report tests, benchmark-runner checks, shell syntax and whitespace checks. A disposable reproduction confirmed the new failure. No full native/JVM build ran locally. Actionlint was unavailable locally but passed in hosted Preflight. Cross-run library hits, main publication, retention and end-to-end savings remain unverified. The manual writer workflow has no exact-head run. Project files and GitHub state were unchanged.

[sunchao]

[P1] Trust the repository root when checking dependencies from native/. The composite executes cd native before invoking --check-depinfo, so this command trusts /__w/datafusion-comet/datafusion-comet/native instead of the Git worktree root. With the container's checkout ownership mismatch, git rev-parse exits 128 after Cargo succeeds. All six exact-head native producers fail here, preventing artifact upload and downstream testing. Main's cache-miss builds would also fail before publication. Please invoke the check from the checkout root with the adjusted dep-info path, or explicitly use the repository root for safe.directory. Add a regression combining different ownership with the native/ working directory.

Evidence: Exact-head job https://github.com/apache/datafusion-comet/actions/runs/36489698536/job/109156416323 logs successful Cargo compilation, then fatal: detected dubious ownership and safe.directory=/__w/datafusion-comet/datafusion-comet/native. The other five producer logs show the same failure. Independently reproduced in a disposable Git repository containing a tracked native source, a valid dependency file and temporary JAVA_HOME: with GIT_TEST_ASSUME_DIFFERENT_OWNER=1, isolated global Git configuration and GIT_CONFIG_NOSYSTEM=1, the helper passes from the repository root but exits 1 from native/ because Git exits 128. No global Git configuration changes were needed.