Conversation
The TPC-H and TPC-DS dataset caches were keyed on the hash of pr_build_linux.yml, so every workflow edit regenerated both datasets and left useless entries that evict main's. The generators were also unpinned: GenTPCHData cloned tpch-dbgen HEAD and the tpcds-kit checkout had no ref, so a generator change could not rotate a dataset key. Pin both generators and key the datasets on their inputs instead: - TPC-H key is the hash of GenTPCHData.scala; the generator now checks out a fixed tpch-dbgen commit before applying the stdout patch. - TPC-DS key embeds the pinned tpcds-kit commit, which the checkout now names explicitly. Save both from main only, matching the cache-write policy for other large caches. check_tpc_dataset_caches() in dev/ci/check-ci-config.py asserts all of the above so the old shape cannot come back.
github-actions
Bot
added
build
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Workflow edits invalidated both TPC datasets, and generator revisions were not fully pinned.
- Design approach: Pin both generators, key datasets on generator inputs, and save caches only from
main. - Correctness / compatibility analysis: Both pins match current upstream heads. Checked Spark’s generator sources for supported versions 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. No introduced P1/P2 issues found within this review.
- Key design decisions: Explicit restore/save steps fit the existing cache policy. The change adds no query-runtime overhead or new abstraction layer.
- Implementation sketch: Updates two workflow caches, pins
GenTPCHDatabefore applying its existing stdout patch, and adds configuration checks. - Behavioral changes worth calling out: Unrelated workflow edits retain cached data. PR and queue runs restore caches but regenerate on misses without saving.
- Suggested improvements: None meeting the P1/P2 reporting threshold.
Reviewed the full three-file diff from 9a4d5f28368d4fbb54ac23c14c5d52d5637ce26d to fb6260914e170276ec0bd87d105bbafdbfcc572e. Confirmed non-draft status. Read the discussion snapshot and live reviews/comments/threads, all empty. Routed skill: review-comet-pr; no sibling skill applies.
Exact-head CI: label passed. Comet CI, CodeQL, and Check PR Title report action_required. Comet CI has no executed jobs, so there is no build/test verdict.
Local validation passed: CI configuration checks, suite registration, workflow actionlint, and the new guard’s five expected failures against base fixtures. Both pinned data generators compiled, and bounded SF=1 stdout tests produced expected row counts. Full Maven/native builds, Spark Parquet generation, benchmark query suites, and hosted cache restore/save behavior were not executed. The project working tree remains unchanged.
|
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.
The new keys at |
The TPC-H and TPC-DS dataset caches were keyed on the hash of pr_build_linux.yml, so every workflow edit regenerated both datasets and left useless entries that evict main's. The generators were also unpinned: GenTPCHData cloned tpch-dbgen HEAD and the tpcds-kit checkout had no ref, so a generator change could not rotate a dataset key. Pin both generators and key the datasets on their inputs instead: - TPC-H key is the hash of GenTPCHData.scala; the generator now checks out a fixed tpch-dbgen commit before applying the stdout patch. - TPC-DS key embeds the pinned tpcds-kit commit, which the checkout now names explicitly. Save both from main only, matching the cache-write policy for other large caches. check_tpc_dataset_caches() in dev/ci/check-ci-config.py asserts all of the above so the old shape cannot come back.
Keep the verified refreshed tree while retaining the published head as an ancestor, allowing a normal push without rewriting history.
github-actions
Bot
added
area:writer
|
Updated against pinned main Scale factor and partition count now come from the same job inputs used by actual generator arguments, restore/save cache keys, and dataset/query paths. Pinned generators, exact-match dataset restores, main-only saves and upstream Cargo policy are retained. The workflow README now describes this behavior accurately. Verification on Apollo:
Limitations: the local TPC-H build used Updated head: TPC-DS follow-upTPC-DS follow-up: a sandbox-only PR source remains unchanged. Local generator patch and compiler compatibility flags were required; unmodified hosted-workflow compatibility and full TPC-DS/query suites remain unverified. Current-main diff correctionBoth intended commits were rebased onto Current-base input/mutation checks (15), CI/suite guards, benchmark checker (55 suites), inventory/summary tests and workflow formatting checks passed. Previous TPC-H and patched-local TPC-DS runtime evidence remains tied to the earlier source/base and its recorded compatibility overrides, not a new-base runtime or hosted-cache claim. Updated head: |
3 participants
Closes #6102.
The TPC-H and TPC-DS dataset caches were keyed on
hashFiles('.github/workflows/pr_build_linux.yml'), so any workflow editregenerated both datasets and left entries that a later run can never restore.
The generators were also unpinned:
GenTPCHDataclonedtpch-dbgenHEAD andthe
tpcds-kitcheckout had noref:.This pins both generators and keys the datasets on their inputs:
GenTPCHData.scala; the generator checks out a fixedtpch-dbgencommit before applying the stdout patch.tpcds-kitcommit, which the checkout namesexplicitly.
actions/cache/restoreplus a save step guarded tomain, matching the cache-write policy used for other large caches.check_tpc_dataset_caches()indev/ci/check-ci-config.pyasserts the keyshape, the pinned
ref:, and that the TPC caches are no longer bareactions/cache@vN.Testing: the new guard fails on the previous revision with five findings and
passes after the change (
CI config checks passed);dev/ci/check-suites.pyalso passes.