Add status to per-check summary metrics - #1471
Conversation
Each check_metrics entry now carries a status field derived from the
existing error and warning aggregates, with errors taking precedence
over warnings:
{"check_name": "...", "error_count": 0, "warning_count": 3,
"status": "warn"}
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 is derived inside the same SQL expression rather than emitted as
a separate metric, keeping 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 being repeated inline.
Resolves databrickslabs#1166
Co-authored-by: Isaac
|
|
Check names were embedded in Spark SQL string literals with only single
quotes escaped, as ''. 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, because
the '' pair is dropped outright rather than unescaped. The
comparison looked for a different name than the one recorded in
_errors, never matched, and the check was reported as passing with
error_count 0. A name like it's_valid was also reported as
its_valid.
* A double quote produced malformed JSON: json.dumps encodes it as \",
the parser consumed the backslash, and json.loads on the metric
raised JSONDecodeError.
Escape the backslash as \\ and the single quote as \' via a shared
_sql_literal_escape helper, applied both to the JSON-encoded name and to
the exists() comparison literal.
The pre-existing test only compared the generated SQL against
get_metrics itself, so it passed regardless of whether the escaping was
correct. Added unit tests that pin the emitted SQL and 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 this change.
Also from review of databrickslabs#1471:
* Reverted the cookbook's failing-checks filter to the count-based
predicate. status != 'passed' 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.
Co-authored-by: Isaac
| {"check_name": "passenger_incorrect_count", "error_count": 0, "warning_count": 0} | ||
| {"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": "passenger_incorrect_count", "error_count": 0, "warning_count": 0, "status": "passed"} |
There was a problem hiding this comment.
add an example where we have both error and warning count > 0
| - `check_name` — the name of the check (either explicitly set via `name` in the rule definition, or auto-derived from the check function and arguments) | ||
| - `error_count` — number of rows where this check triggered an error (`0` if no rows failed the check) | ||
| - `warning_count` — number of rows where this check triggered a warning (`0` if no rows failed the check) | ||
| - `status` — outcome of the check for this run: `error` if `error_count > 0`, `warn` if only `warning_count > 0`, otherwise `passed` |
There was a problem hiding this comment.
what if error_count > 0 and warning_count > 0? I assume this will produce error status but this is currently not documented
| - `warning_count` — number of rows where this check triggered a warning (`0` if no rows failed the check) | ||
| - `status` — outcome of the check for this run: `error` if `error_count > 0`, `warn` if only `warning_count > 0`, otherwise `passed` | ||
|
|
||
| <Admonition type="tip" title="Why status and not just counts"> |
There was a problem hiding this comment.
I don't think this explanation is necessary. I would drop this tip. We also care about how many rows violated depending on dashboard so this is not always true.
| "check_name": str(check_name), | ||
| "error_count": invalid_rows, | ||
| "warning_count": 0, | ||
| "status": "error" if invalid_rows > 0 else "passed", |
There was a problem hiding this comment.
Please use pass, not passed. If we use passed then we should also use errored, warned which reads oddly
| "status": "error" if invalid_rows > 0 else "passed", | |
| "status": "error" if invalid_rows > 0 else "pass", |
In the dashboard we also use pass, error, warn. It would be good to check the dashboard (DQX_Dashboard.lvdash.json) to see if we can use the new status there. We are calculating it today inside the dashboard
| json_check_name_sql_esc = _sql_literal_escape(json.dumps(check_name)) | ||
| err = self._error_column_name | ||
| warn = self._warning_column_name | ||
| error_count = f"count(case when exists({err}, x -> x.name = '{check_name_escaped}') then 1 end)" |
There was a problem hiding this comment.
The error_count and warning_count aggregate expressions are each embedded twice in the emitted SQL — once in the cast(... as string) fragment and again inside the status CASE — so the per-check aggregate text is duplicated. Spark's common-subexpression elimination collapses the identical aggregates at execution, so runtime cost is unchanged, but the generated observe() SQL string is larger and harder to read/debug. A single derived-column alias isn't expressible inside one observe() expression, so the duplication is inherent to this approach — worth a short comment noting that CSE makes it free at runtime, rather than leaving it implicit.
| json_check_name_sql_esc = _sql_literal_escape(json.dumps(check_name)) | ||
| err = self._error_column_name | ||
| warn = self._warning_column_name | ||
| error_count = f"count(case when exists({err}, x -> x.name = '{check_name_escaped}') then 1 end)" |
There was a problem hiding this comment.
The error_count and warning_count aggregate expressions are each embedded twice in the emitted SQL — once in the cast(... as string) fragment and again inside the status CASE — so the per-check aggregate text is duplicated. Spark's common-subexpression elimination collapses the identical aggregates at execution, so runtime cost is unchanged, but the generated observe() SQL string is larger and harder to read/debug. A single derived-column alias isn't expressible inside one observe() expression, so the duplication is inherent to this approach — worth a short comment noting that CSE makes it free at runtime, rather than leaving it implicit.
| - `check_name` — the name of the check (either explicitly set via `name` in the rule definition, or auto-derived from the check function and arguments) | ||
| - `error_count` — number of rows where this check triggered an error (`0` if no rows failed the check) | ||
| - `warning_count` — number of rows where this check triggered a warning (`0` if no rows failed the check) | ||
| - `status` — outcome of the check for this run: `error` if `error_count > 0`, `warn` if only `warning_count > 0`, otherwise `passed` |
There was a problem hiding this comment.
The new status field is a user-facing feature but is documented inline without a <FeatureTags> / AvailableSinceVersion marker. AGENTS.md ("Authoring documentation") asks new features to be tagged with lifecycle stage and version so readers can tell which DQX release introduced them. Consider adding a version tag for the status field. (Minor: the guidance targets subsections/headings, and this is inline prose on an already-tagged page, so it's arguably not a strict violation.)
Tag version 0.17.0
mwojtyczka
added
under-review
mwojtyczka
left a comment
mwojtyczka
left a comment
There was a problem hiding this comment.
Generally looking good, left some small comments that need to be addressed before we merge
3 participants
Changes
Adds a
statusfield to eachcheck_metricsentry, 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": "passenger_incorrect_count", "error_count": 0, "warning_count": 0, "status": "passed"} ]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_countequalsinput_row_countand reads as though every row is individually bad.statusgives 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.statusis 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 withspark.sql.parser.escapedStringLiteralsfalse, where the backslash is the escape character and ANSI''doubling is not honoured. Two silent failures followed: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 witherror_count0.it's_validwas also reported asits_valid.json.loadson the metric raised.Fixed via a shared
_sql_literal_escapehelper applied to both the JSON-encoded name and the comparison literal. The pre-existing test compared the generated SQL againstget_metricsitself, 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
status != 'passed'silently drops metrics rows written beforestatusexisted, wherefrom_jsonyieldsNULL.statusin thecheck_metricsit synthesises for cross-table SQL checks, so both producers write one shape into the shared metrics table.error/warn/passedvocabulary: it matches theCriticalityenum and the shape proposed in [FEATURE]: Add status to summary & check_metrics #1166. The bundled dashboard already carries two other spellings ('Error'/'Warn'/'Pass'for display,errored/warned/passedfor heatmap colours) — a pre-existing inconsistency worth a separate cleanup.Linked issues
Resolves #1166
Resolves #1474
Relates to #1150
Tests
Unit: a new test pins the generated SQL literally. The existing
_check_metrics_exprhelper derives its expectation fromget_metricsitself, 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/passed), assert that a check accumulating both errors and warnings reportserror, and assert check-name round-trips for five name shapes. The fulltest_summary_metrics.pysuite passes against serverless compute with a SQL warehouse configured — 66 passed, 0 skipped.Documentation and Demos
Updated
summary_metrics.mdx,table_schemas.mdxandquery_results_cookbook.mdx.Follow-up (not in this PR)
DQX Studio's
_parse_check_metricsreads the three existing keys explicitly, so the new field is ignored and nothing breaks — but the app does not surfacestatusyet. ExtendingCheckMetricBreakdownrequires 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.