sunchao commented on PR #5976: URL: https://github.com/apache/datafusion-comet/pull/5976#issuecomment-5700408007
Thanks, @andygrove, for the [review](https://github.com/apache/datafusion-comet/pull/5976#pullrequestreview-5223975602). I pushed [e3b9e8542](https://github.com/apache/datafusion-comet/commit/e3b9e8542695253204f7aba5ce4a625b1976827e) and updated the description. Going through the six points: 1. **Sequencing:** agreed. The description now explicitly depends on #5973 landing first and calls for rebasing onto it before merging. #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](https://github.com/apache/datafusion-comet/blob/e3b9e8542695253204f7aba5ce4a625b1976827e/native/core/build.rs#L22-L40) 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. -- 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]
