Add status to per-check summary metrics

[moomindani]

Changes

Adds a status field to each check_metrics entry, derived from the existing error and warning aggregates with errors taking precedence:

[
  {"check_name": "id_is_not_null", "error_count": 5, "warning_count": 0, "status": "error"},
  {"check_name": "name_is_not_null_and_not_empty", "error_count": 0, "warning_count": 3, "status": "warn"},
  {"check_name": "id_and_name_both_flagged", "error_count": 2, "warning_count": 7, "status": "error"},
  {"check_name": "passenger_incorrect_count", "error_count": 0, "warning_count": 0, "status": "pass"}
]

Counts alone do not answer "did this check pass". The gap is widest for dataset-level checks: when an ungrouped dataset-level check fails, the result is attached to every input row, so error_count equals input_row_count and reads as though every row is individually bad. status gives the single table-level signal instead — this is the same field #1150 needs to make dataset-level results legible, so landing it here unblocks that discussion.

status is computed inside the same SQL expression rather than emitted as a separate metric, preserving the concat-based construction required by the two Spark Connect constraints already documented on _build_check_metrics_expr. The per-check count expressions are now bound to locals instead of repeated inline.

Also fixes: check-name escaping (pre-existing, #1474)

Review of this PR surfaced a pre-existing bug in the same function. Check names were embedded in the SQL literal with only single quotes escaped as '', but Spark's parser runs with spark.sql.parser.escapedStringLiterals false, where the backslash is the escape character and ANSI '' doubling is not honoured. Two silent failures followed:

• A single quote or backslash broke the exists() comparison — the '' pair is dropped rather than unescaped, so the comparison looked for a different name than the one in _errors, never matched, and the check was reported as passing with error_count 0. it's_valid was also reported as its_valid.
• A double quote produced malformed JSON, so json.loads on the metric raised.

Fixed via a shared _sql_literal_escape helper applied to both the JSON-encoded name and the comparison literal. The pre-existing test compared the generated SQL against get_metrics itself, so it passed regardless of correctness; added unit tests that pin the emitted SQL plus a parametrised integration test asserting the round-trip for plain, single-quote, double-quote, backslash and mixed names. Four of the five shapes fail before the change.

It is folded in here rather than split out because it touches the exact lines this PR rewrites — a separate PR would conflict with this one. Happy to split it if you would prefer to review it independently.

Review follow-ups

• Reverted the cookbook's failing-checks filter to the count-based predicate: status != 'pass' silently drops metrics rows written before status existed, where from_json yields NULL.
• The app task runner now emits status in the check_metrics it synthesises for cross-table SQL checks, so both producers write one shape into the shared metrics table.
• Renamed the passing status from passed to pass at review request, matching the pass/error/warn vocabulary the bundled dashboard already uses for display. The dashboard's remaining spellings (errored/warned/passed in the heatmap colour mappings) and its in-dashboard recomputation of per-check status are tracked separately in [FEATURE]: Use check_metrics.status for the dashboard's per-check status widget #1507.

Linked issues

Resolves #1166
Resolves #1474
Relates to #1150

Tests

• manually tested
• added unit tests
• added integration tests
• added end-to-end tests
• added performance tests

Unit: a new test pins the generated SQL literally. The existing _check_metrics_expr helper derives its expectation from get_metrics itself, so it cannot catch a change in the emitted JSON shape; the new test closes that gap. Verified it fails before the change and passes after. Two further tests pin the quote and backslash escaping.

Integration: new cases cover all three outcomes (error / warn / pass), assert that a check accumulating both errors and warnings reports error, and assert check-name round-trips for five name shapes. The full test_summary_metrics.py suite passes against serverless compute with a SQL warehouse configured — 68 passed, 0 skipped.

Documentation and Demos

• added/updated demos
• added/updated docs
• added/updated agent skills

Updated summary_metrics.mdx, table_schemas.mdx and query_results_cookbook.mdx. The status explanation now lives in a #### Check Status subsection tagged AvailableSinceVersion 0.17.0, with a condition table making the error-over-warning precedence explicit.

Follow-up (not in this PR)

DQX Studio's _parse_check_metrics reads the three existing keys explicitly, so the new field is ignored and nothing breaks — but the app does not surface status yet. Extending CheckMetricBreakdown requires regenerating the orval client (make app-regen-api), so it belongs in a separate app-scoped change.

This pull request and its description were written by Isaac.

[CLAassistant]

All committers have signed the CLA.

[mwojtyczka]

Generally looking good, left some small comments that need to be addressed before we merge

[moomindani]

@mwojtyczka All review comments are addressed and pushed — the passing status is now pass, the precedence rule is documented in a tagged "Check Status" subsection with a condition table, the tip is dropped, and the CSE note is in. The dashboard question is filed separately as #1507 rather than widened into this PR. test_summary_metrics.py passes 68/68 against serverless compute with a SQL warehouse configured. Ready for another look whenever you have time — and let me know if you'd like the "Fork test" mirror PR to run full CI.

[mwojtyczka]

@moomindani

Overall: The escaping refactor is correct and the round-trip test coverage is good. My concern is with the status field itself — it's derivable from data already in the struct, and this PR doesn't actually wire up any consumer to use it. If we're not going to consume status, this PR isn't worth shipping: it's denormalized, redundant state plus new escaping/CSE complexity, with no reader. Details below.

Additional review comments:

1. Escaping depends on an undocumented session precondition (metrics_observer.py).
The backslash escaping is correct only when spark.sql.parser.escapedStringLiterals=false (the default). The new docstring now states this precondition clearly — good. But it's still only a docstring, not a guard. If a session enables Hive-compat mode, a name like it's_valid produces 'it\'s_valid' where \' no longer collapses, and the observe() expression fails to parse (or yields malformed JSON so json.loads raises). Worth deciding: is documenting the precondition enough, or do we want a defensive check? I'd accept the docstring for now, but flag it.

2. status is fully derivable redundant state — the core question.
status is a pure function of error_count/warning_count, both present in the same struct (>0 → error, else >0 → warn, else pass). The Studio score view (score_view_service.py:370) already computes exactly this from the counts. Materializing it is only justified if we commit to it as a canonical contract — one authoritative precedence rule and a stable vocabulary for NL/Genie consumers. As shipped, that justification isn't met (see #3 and #4), so right now it's redundant state with no reader.

3. The named consumer wasn't wired up (score_view_service.py:103).
The PR justifies status because "consumers (dashboard, Studio, Genie) need a pass/fail signal." But _CHECK_METRICS_JSON_SCHEMA still omits status, so from_json silently drops it and Studio never surfaces the field. Not a crash — but the stated goal isn't delivered. This is the crux: if we're adding status for Studio, Studio has to read it.

4. The second producer mislabels warn as error (runner.py:706).
The synth SQL path hardcodes status = 'error' on any violation, even for a check authored with warn criticality. This breaks the "one canonical precedence rule" benefit that would justify materializing status in the first place — the two producers already disagree. (The underlying count convention is pre-existing, but adding status makes the mislabel consumable rather than latent.)

Recommendation: Decide whether status is a contract we're committing to.

• If yes: complete it in this PR — update _CHECK_METRICS_JSON_SCHEMA (Bump actions/setup-python from 4 to 5 #3), fix the runner precedence (Bump actions/checkout from 2.5.0 to 4.1.3 #4), and keep the good docstring/CSE comments. Then it's a coherent, defensible feature.
• If no: drop status and let consumers derive it from the counts. The escaping refactor is a genuine fix and could land on its own.

[moomindani]

Went with the "if yes" branch — status is a contract worth committing to, so I completed it rather than dropping it.

#3 Studio now reads it. _CHECK_METRICS_JSON_SCHEMA gained status: STRING; the shaping view explodes it and projects check_status. Runs recorded before the observer emitted status carry NULL, so it COALESCEs onto the same error-over-warning precedence rather than dropping those rows out of the column. The REST model (CheckMetricBreakdown) still omits it — that needs an orval regeneration, so it stays a separate app-scoped change.

#4 The runner no longer mislabels warn as error. Extracted _split_violations_by_criticality so the rule is named and unit-testable without Spark, and applied it to the metric payload, the run row and the failed-checks summary.

One judgement call to flag: you scoped this as "fix the runner precedence" and noted the count convention is pre-existing. I fixed the counts too, because a status derived from criticality while error_count/warning_count stayed error-only would contradict its own struct — and the documented rule is that status is a function of those two counts. The visible consequence is that a warn-criticality cross-table SQL check now reports warning_rows instead of error_rows in run history and the failed-checks summary. I think that's the bug rather than a behaviour change, but say the word and I'll narrow it back to status alone.

#1 Left as the docstring, per your call.

Thanks for the branch update — I built on your merge commit rather than force-pushing over it, so the two new commits sit on top. Also picked up the productName="DQX" prop on the new version tag to match what landed with the independent Studio versioning.

Verified: make fmt/lint clean, basedpyright 0 errors, 2503 core unit tests, app backend tests pass, and test_summary_metrics.py -k check_metrics 14/14 against serverless compute with a SQL warehouse. make app-check's TypeScript step needs bun, which I don't have locally — no UI files changed.