Repository navigation
Drop seed matrix cells that never produce committable bundles - #2971
Conversation
There was a problem hiding this comment.
oracle-verdict: failure for 4079a7cfcb4edf9ac0438db49457d402823d960d
This result supersedes those for earlier heads; their open review threads still apply.
Tier: low-medium (data-only or small single-area change).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
PR 2971 drops three TPC-H seed cells and rewrites the workflow test to expect two-identity TPC-H cohorts, contradicting the three-identity validator floor and the seed spec that still lists those cells as supported. Fix by restoring or replacing the third identity per affected cohort with evidence, and correcting the test/spec to enforce the floor.
Defects without a review thread
- High
.github/workflows/seed-corpus.yml:24-49: D1: Seed matrix leaves TPC-H cohorts below three-identity corpus floor
After removal the seed workflow generates only two local identities for three TPC-H cohorts: SF 0.01 and SF 0.1 have only duckdb+datafusion, and SF 1.0 has only duckdb+clickhouse-local. results-data/validate_corpus.py sets MINIMUM_PLATFORMS_PER_COHORT=3 and shallow_cohorts() fails any cohort with 1-2 rankable identities, and results-data/SEED_CORPUS_SPEC.md requires three local identities per supported cohort so it stays publishable without cloud secrets. A monthly refresh of only these two cells cannot sustain the depth gate; TPC-H SF 0.01/0.1 have no other rankable third identity since sub-scale cloud bundles are unofficial and version-matrix coverage is SF 10. Restore the removed cells or add an admitted third local identity per affected cohort.
Defects outside the diff count like the others: fix them before merge.
Reviewers not available
- luna: absent (error)
|
D1 (seed matrix below the three-identity floor) is addressed in 4f1ba92: instead of dropping to two identities per affected cohort, the three ineligible cells are replaced with evidence-backed third identities, keeping all six cohorts at three monthly-refreshable local identities. Evidence: tpch/polars-df SF 0.01/0.1 rankable=False with validation_not_clean (corpus validator output; run-3093 bundles dropped as mirror-ineligible in f4ef449); tpch/datafusion SF 1.0 fails deterministically on Q18 memory-pool exhaustion (seed run 38331461283 logs). Replacements each have a rankable committed bundle with summary.validation=passed: clickhouse-local at TPC-H SF 0.01/0.1 and polars-df at TPC-H SF 1.0. The per-cohort floor assertion is restored and the spec records the ineligibility evidence. Requesting oracle re-review of the new head. |
There was a problem hiding this comment.
oracle-verdict: success for 4f1ba927dd0811a139e9ea976b82eb5b94b88a41
This result supersedes those for earlier heads; their open review threads still apply.
Tier: low-medium (data-only or small single-area change).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
All TPC-H cohorts now have three local identities and the matrix test plus spec document the ineligibility evidence and replacement coverage. No new merge-blocking correctness, gate, or security defects found in the three changed files.
Verified fixed: D1, D2. Resolve their review threads.
tpch/polars-df at sf0.01 and sf0.1 always validate uncertain through the structural SQL-translation fallback, so their bundles are mirror-ineligible and hand-dropped from every seed PR. tpch/datafusion at sf1.0 fails deterministically when TPC-H Q18 exhausts the engine memory pool. Removing all three cells keeps every remaining cell committable and every Seed run green; the per-cohort platform test now pins the exact matrix.
Drop tpch/polars-df at SF 0.01/0.1 (always validation_not_clean via the SQL-translation fallback; run-3093 bundles dropped as mirror-ineligible) and tpch/datafusion at SF 1.0 (deterministic Q18 memory-pool exhaustion in seed run 38331461283). Substitute clickhouse-local at SF 0.01/0.1 and polars-df at SF 1.0, each backed by a rankable committed bundle, so every cohort keeps three monthly-refreshable local identities. Restore the per-cohort floor assertion and record the ineligibility evidence in the seed corpus spec.
4f1ba92 to
2f25685
Compare
|
Rebased onto current develop in 2f25685: the visual-regression mismatch was renderer drift, not this diff. Six explorer/results changes merged after this branch's base (including #2941 stream exclusions, #2922 compare views, and #2913 execution variants), which repaint the /results/* pages the baseline captures. No content changes in the rebase; seed matrix, floor test, and spec evidence are unchanged. Requesting fresh CI and oracle review of the new head. |
There was a problem hiding this comment.
oracle-verdict: success for 2f25685a601b5ef8fb9f27d46724e2e0605dbca2
This result supersedes those for earlier heads; their open review threads still apply.
Tier: low-medium (data-only or small single-area change).
Decision: SHIP, carried from head 4f1ba927dd0811a139e9ea976b82eb5b94b88a41.
The patch is unchanged since head 4f1ba927dd0811a139e9ea976b82eb5b94b88a41 was reviewed, so its decision is carried to this head without running a reviewer. Its open review threads still apply.
tpch/polars-df at sf0.01 and sf0.1 always validate uncertain through the
structural SQL-translation fallback, so their bundles are mirror-ineligible
and hand-dropped from every seed PR. tpch/datafusion at sf1.0 fails
deterministically when TPC-H Q18 exhausts the engine memory pool. Removing
all three cells keeps every remaining cell committable and every Seed run
green; the per-cohort platform test now pins the exact matrix.
Verification:
uv run -- pytest tests/unit/workflows/test_seed_corpus_pr_base.py -qpasses 11/11. Matrix goes from 18 to 15 cells; the three removed cells are tpch/polars-df sf0.01, tpch/polars-df sf0.1 (permanently uncertain, mirror-ineligible) and tpch/datafusion sf1.0 (deterministic Q18 memory-pool exhaustion confirmed in Seed run 38331461283 logs).