andygrove commented on PR #6112: URL: https://github.com/apache/datafusion-comet/pull/6112#issuecomment-5876210335
This is a light fully automated review since there are so many PRs open. The restore/save split matches the existing Maven cache pattern well. `.github/workflows/README.md:396-399` still describes the setup this PR replaces. It says the TPC-H and TPC-DS dataset caches "keep the read-write form and are out of scope entirely" and that they are "keyed on this workflow file". After this change they use `actions/cache/restore` plus a main-only `actions/cache/save`, the same pattern the section documents for the Maven and cargo caches just above, and they are keyed on `GenTPCHData.scala` and the pinned `tpcds-kit` commit. Could this paragraph be updated to match what `pr_build_linux.yml` does now? The new keys at `pr_build_linux.yml:707` and `:792` also no longer cover the generator arguments. `--scaleFactor 1 --numPartitions 1` is passed at `:716` and `:816` and lives only in the workflow file, which the old key hashed. If a later change bumps `--scaleFactor` without touching `GenTPCHData.scala` or the `tpcds-kit` ref, the cache still hits, the generate step is skipped, and the queries run against the old dataset. Would it make sense to fold the scale factor and partition count into the keys? -- 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]
