Extend summary metrics with per-check-name breakdowns#1097
Conversation
Add per-check error and warning counts to the DQMetricsObserver, allowing users to drill down summary metrics by individual quality check name. The engine passes check names to the observer before collecting metrics, and the observer generates SQL expressions using exists() to count rows matching each check. Closes #943 Co-authored-by: Isaac
Cover per-check metric generation, custom column names, special character sanitization, and replacement of previously set check names. Co-authored-by: Isaac
ghanse
left a comment
There was a problem hiding this comment.
Good implementation. The per-check metric generation is clean, backward-compatible, and well-tested. Two minor findings below — neither is blocking but both are worth addressing.
Co-authored-by: Isaac
Co-authored-by: Isaac
ghanse
left a comment
There was a problem hiding this comment.
Addressed duplicate alias collision — _build_per_check_metrics now detects when two check names sanitize to the same alias and raises DQXError. Added unit test for the edge case (e.g. check-1 vs check.1). Ready for cleanup pass.
- Rename `default_metrics` to `all_metrics` in metrics property since it accumulates default, per-check, and custom metrics - Fix copy-paste docstring in test_dq_observer_custom_column_names that incorrectly said "default column names" Co-authored-by: Isaac
|
Cleanup pass complete.
Tests pass (16/16). Ready for review. |
ghanse
left a comment
There was a problem hiding this comment.
Post-cleanup final review: APPROVED
Reviewed the full diff across metrics_observer.py, engine.py, test_observer.py, and test_summary_metrics.py.
Correctness: The Spark SQL exists() higher-order function is the right approach for checking array-of-struct columns. Single-quote escaping, alias sanitization, and duplicate collision detection are all correctly implemented. The @cached_property to @property change on metrics is necessary and correct since check names are set dynamically.
Completeness: Fully addresses #943 with per-check-name error/warning counts. The track_extended_metrics flag defaults to False for full backward compatibility. Both apply_checks and apply_checks_by_metadata paths are covered since the latter delegates to the former.
Test coverage: 10 new unit tests and 2 parametrized integration tests (4 total runs) cover: default behavior, per-check metrics, metric ordering, custom column names, special character sanitization, check name replacement, empty alias error, and duplicate alias collision.
Code quality: Clean naming, proper docstrings, no dead code, pure helper function for sanitization. AGENTS.md compliant — type hints on all signatures, DQXError for exceptions, no new Any usage.
No changes required. Ready for human approval.
mwojtyczka
left a comment
There was a problem hiding this comment.
Review: Extend summary metrics with per-check-name breakdowns
Clean feature addition. The approach of generating per-check SQL expressions via the observer is sound. A few items below.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1097 +/- ##
==========================================
+ Coverage 91.97% 92.03% +0.06%
==========================================
Files 101 101
Lines 9408 9418 +10
==========================================
+ Hits 8653 8668 +15
+ Misses 755 750 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changes
Extended
DQMetricsObserverto emit a newcheck_metricsrow alongside the existing aggregates (input_row_count,error_row_count,warning_row_count,valid_row_count). The value is a JSON array of structs — one entry per check — that fits the existingmetric_name/metric_valueschema without widening it:[{"check_name": "id_is_not_null", "error_count": 1, "warning_count": 0}, ...]Backward compatible: existing metrics unchanged, new row is additive and opt-in.
Key design decisions:
exists()higher-order function.Linked issues
Closes #943
Tests
This pull request was AI-assisted by Isaac.