Close three completeness gaps from the main review: maintenance contract, CI gate, YAML cadence, SQL value typing - #694
Merged
Merged
Conversation
… registry `WorkloadEntry` had no `repeat_every`, so the startup pre-population loop pinned `QuerySpec::repeat_every` to `None` while the same declaration sent through `POST /api/v1/plan` carried its cadence into `QueryWorkload::repeat_every`. The deployment cost model reads that field as the batch-mode flush-period proxy, so one declaration was costed two ways depending on which entry point registered it. Add the field and collapse the three hand-copied entry -> QuerySpec literals (startup loop, emit test helper, MVP46 test loop) into `workload::query_spec_for_entry` — the duplication is why the field was missed in the first place. Also stop losing registry input silently: `WorkloadEntry` now denies unknown fields, and `WorkloadRegistry::try_load` fails startup on a file that exists but does not parse. A registry that degrades to empty is indistinguishable, at every later step, from a deployment that declared no workloads. A missing file stays non-fatal, which the process e2e tests rely on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Automatic SQL materialization took a non-null Float64 value column only, so an ordinary nullable or integer ClickHouse column could not be accelerated at all. The refusal was protecting the result — the ingest path had no way to know how to read the column — rather than expressing a semantic limit. Give it that way: `PrecomputeMaterialization::value_source_column` carries the producer column's declared type and nullability alongside the value projection. Typing is a read concern, not an identity one, so it stays out of the policy fingerprint — two materializations over the same column are the same policy however it is declared. The ClickHouse reader then reads the column through its type: * nullable -> `IS NOT NULL`, so the summary skips NULL inputs exactly as the SQL aggregate it stands in for does (and a NULL row no longer fails the row decode). * `Int64` -> explicit `toFloat64`, guarded by `throwIf(abs(col) > 9007199254740992)`. Summary state is f64; beyond 2^53 an integer no longer round-trips, so the read fails loudly instead of summarising a rounded value. Non-numeric columns stay refused: a string, boolean or timestamp column has no value semantics to summarise. The existing test that asserted an Int64 source must be rejected now asserts it is admitted with its producer typing, which is what makes the guard above possible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ta plane tests in CI Three things kept the data plane out of the default gate, and they compound: 1. A stable failure. `one_sid_with_two_populations_cannot_publish_a_partial_global_summary` asserted `execute_finite_maintenance` refuses a global reduce over one sid carrying two populations. It does not, and it should not: the `CanonicalLabelsV1` path reads the cohort through `read_complete_raw_maintenance_cohort`, which enumerates the whole durable population set for the definition and fails on the first population missing the window. The reduce sees both populations or it sees nothing, so the partial publication the test guarded against is unreachable there. Rewritten as the acceptance contract instead of dropping the assertion: the two populations carry distinct values, and the test proves one output series with one (empty) group and one window, that the reduce consumed both populations' state, that a re-run republishes nothing and writes no new part, and that the output is durably committed. The second half — a later lifetime repeating a logical population — is still refused, and now also proves it published nothing new. 2. Fifteen parallel flakes. Every one waited on a background flusher / sealer thread under a fixed 5s budget. At one test per core the waiter and the thread it waits on compete for the same CPUs and the budget expires while the work is merely queued — which is why they passed on serial re-runs. `tests::test_utilities::timing` scales the budget with test concurrency (`ASAP_TEST_TIMEOUT_SCALE` overrides); a passing wait still returns as soon as its condition holds, and a stuck one still fails. 3. No gate. `cargo test -p asap_types -p data_plane --lib` now runs in MVP CI. Compiling the data plane was never evidence that it runs: plan installation, maintenance execution, persistence recovery and query routing all live in these tests. `--lib` deliberately excludes `data_plane/tests/`, which needs live ClickHouse / Prometheus / VictoriaMetrics endpoints. Data plane library tests: 1194 passing under the default parallel run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the 2026-09-12 review of
ASAPQuery-backend/ASAPPlannermain. Closes the gaps the review filed as findings 0, 4 (typing / NULL half), 5 and 6. MetricsQL frontend integration (finding 1) and the continuous-maintenance / ERP closure (findings 2 and 3) are deliberately out of scope here.Note: finding 4's fixed-window half was already addressed on main by #691 (
reuse installed SQL plans for moving time windows), which landed after the review snapshot. This branch builds on3288aa11.Finding 0 — the stable data-plane failure was an outdated test contract
one_sid_with_two_populations_cannot_publish_a_partial_global_summaryasserted thatexecute_finite_maintenancerefuses a global reduce over one sid carrying two populations. It does not — and it should not. ForCanonicalLabelsV1the run goes throughexecute_finite_complete_populations, andread_complete_raw_maintenance_cohortenumerates the whole durable population set for the definition, failing on the first population that is missing the window. The reduce therefore sees both populations or it sees nothing; the partial publication the assertion guarded against is unreachable on that path.Per the review's acceptance requirement, the test is rewritten as the acceptance contract rather than having the assertion deleted. The two populations now carry distinct values (5 and 7), and the test proves:
Finding 6 — data plane tests now gate, after fixing what kept them out
The other 15 failures in the review's run were parallel flakes, all waiting on a background flusher / sealer thread under a fixed 5 s budget. At one test per core the waiter competes with the thread it is waiting on, and the budget expires while the work is merely queued — which is exactly why they passed on serial re-runs.
data_plane/src/tests/test_utilities/timing.rsscales the budget with test concurrency (ASAP_TEST_TIMEOUT_SCALEoverrides it); a passing wait still returns the moment its condition holds.With that,
cargo test -p asap_types -p data_plane --libjoins MVP CI.--libdeliberately excludesdata_plane/tests/, which needs live ClickHouse / Prometheus / VictoriaMetrics endpoints. Job timeout raised 45 → 60 minutes.Finding 5 — the startup YAML lost the query cadence
WorkloadEntryhad norepeat_every, so the startup loop pinnedQuerySpec::repeat_everytoNonewhile the same declaration throughPOST /api/v1/plancarried it intoQueryWorkload::repeat_every— the cost model's batch-mode flush-period proxy. One declaration, costed two ways depending on the entry point.The field is added, and the three hand-copied entry →
QuerySpecliterals collapse intoworkload::query_spec_for_entry; that duplication is why the field was missed. Silent input loss is closed too:WorkloadEntrydenies unknown fields, andWorkloadRegistry::try_loadfails startup on a registry file that exists but does not parse (a missing file stays non-fatal, which the process e2e tests rely on).Finding 4 — numeric and nullable SQL value columns
Automatic materialization took a non-null
Float64value column only, so an ordinary nullable or integer ClickHouse column could not be accelerated at all. That refusal was protecting the result — the ingest path had no way to know how to read the column — rather than expressing a semantic limit.PrecomputeMaterialization::value_source_columnnow carries the producer column's declared type and nullability. Typing is a read concern, not an identity one, so it stays out of the policy fingerprint. The reader uses it:IS NOT NULL, so the summary skips NULL inputs exactly as the SQL aggregate it stands in for does (and a NULL row no longer fails the row decode);Int64→ explicittoFloat64underthrowIf(abs(col) > 9007199254740992). Summary state is f64; beyond 2^53 an integer no longer round-trips, so the read fails loudly instead of summarising a rounded value.Non-numeric columns stay refused. The existing test that asserted an
Int64source must be rejected now asserts it is admitted with its producer typing, which is what makes the guard possible.Verification
All run locally against the CI-aligned dependency set (
asap_sketchlibat the commitmvp-ci.ymlpins, a cleanASAPCollector):cargo fmt -p control_plane -p data_plane -- --check: clean.cargo clippy -p control_plane -p data_plane -p asap_types --all-targets: 0 errors (remaining warnings match those already present in the review's own check log).cargo test -p asap_types -p control_plane -p data_plane --lib, default parallelism: 107 / 765 / 1194 passing, 0 failed. The review's run was 1169/1185 with 16 failures.cargo test -p control_plane(all targets): 765 + 31 + 1 + 1 + 6 passing, 3 ignored.ASAP_TEST_TIMEOUT_SCALE=4: passing.🤖 Generated with Claude Code