Refactor profiler column metrics into an extensible registry - #1384
Refactor profiler column metrics into an extensible registry#1384IvannKurchenko wants to merge 14 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mwojtyczka
left a comment
There was a problem hiding this comment.
Follow-up review of the head commit. The six earlier threads are all addressed — resolved them. A few new points on the extension point and conventions:
| """ | ||
|
|
||
| def wrapper(metric_func: Callable) -> Callable: | ||
| PROFILE_COLUMN_METRIC_REGISTRY[metric_func.__name__] = metric_func |
There was a problem hiding this comment.
The registry is keyed by metric_func.__name__, but the sibling extension points this mirrors — register_rule(rule_type) and register_profile_builder(profile_type) — take an explicit name string. Two custom metric functions with the same __name__ (e.g. both named count_non_null in different modules, or wrapped/functools.partial funcs) will silently overwrite each other in PROFILE_COLUMN_METRIC_REGISTRY with no error.
Also, since no argument is captured, the parameterless decorator-factory layer (the required ()) adds nothing over a plain def register_profile_column_metric(metric_func):. Consider either taking an explicit name: str (consistent with the siblings, and lets callers disambiguate) or dropping the factory layer.
|
|
||
| # Type alias for annotations; use TEXT_TYPES for isinstance() checks. | ||
| TextType = T.CharType | T.StringType | T.VarcharType | ||
| TEXT_TYPES: tuple[type[TextType], ...] = (T.CharType, T.StringType, T.VarcharType) |
There was a problem hiding this comment.
is_text() was moved here, but the identical TextType / TEXT_TYPES definitions still live in profile_builder.py (lines 35-36), which profiler.py continues to import from. The same configuration is now defined in two modules and can drift. Since profile_builder.py already imports is_text from this module, it could import TEXT_TYPES / TextType from here too and drop its own copies (DRY — per the AGENTS.md guideline).
| from databricks.labs.dqx.profiler.common import is_text | ||
|
|
||
|
|
||
| DQProfileColumnMetricFunc = Callable[[T.StructField, str], Column | None] |
There was a problem hiding this comment.
DQProfileColumnMetricFunc = Callable[[T.StructField, str], Column | None] is defined but not used to type the decorator: register_profile_column_metric returns a bare Callable and wrapper takes/returns a bare Callable. Using the alias in these signatures lets mypy/basedpyright verify that registered functions match the metric signature. (Note register_profile_builder has the same looseness, so this is optional consistency.)
| @register_profile_column_metric() | ||
| def empty_count(field: T.StructField, column_label: str) -> Column | None: | ||
| """ | ||
| Profiling column metric for empty count. Applicable for text columns only, otherwise returns literal `0` for |
There was a problem hiding this comment.
Docstring uses backticks around 0 (and None on line 28). AGENTS.md forbids backticks around object names in docstrings — they break API-doc rendering; use italics (0, None) instead.
Changes
Sets up an extension point for the profiler by moving inline column-metric aggregation in
DQProfiler._profileinto a registry-based system. Users can now register their own column metric functions via the@register_profile_column_metric()decorator, mirroring the existingregister_rule/register_profile_builderextension patterns.The three existing metrics (
count_non_null,count_distinct,empty_count) are moved into this registry with no behavioural change._build_column_metricsbuilds the aggregation from whatever is registered.Rationale for landing the refactoring without new metrics
The profiling pipeline is:
column metrics → profile builder → check. Adding a new metric only adds value once a builder consumes it and generates a check. Two paths were considered for #1067:is_aggr_not_less_than/is_aggr_not_greater_thanchecks. But these checks are mostly useful for measurement data (revenue, sales amount, latency, temperature) and not meaningful for keys or categorical columns. Applying them indiscriminately would generate false positives. Selective, purpose-aware application is tracked in [FEATURE]: Profile classification support #1343.This PR takes option 2. New built-in metrics can be added later, together with the specific builder that consumes them, once the classification work in #1343 makes selective application safe.
What changed
PROFILE_COLUMN_METRIC_REGISTRYandregister_profile_column_metricdecorator inprofiler/profiler_column_metrics.pycount_non_null,count_distinct,empty_count) moved into the registryDQProfiler._profilerefactored: inline aggregation extracted into_build_column_metrics, which iterates the registryis_texthelper moved fromprofile_builder.pytoprofiler/common.py(now used across modules)Linked issues
Relates to #1067, related to #1343
Tests
Unit tests cover: registry (register, overwrite, function-name key); built-in metric functions across column types;
is_texthelper;_build_column_metrics(alias correctness,count_nullderivation, summary merge, empty DataFrame,None-returning metrics). Existing integration tests already cover the refactored aggregation path end-to-end.Documentation and Demos
New sections in
data_profiling.mdxguide andprofiler.mdxreference showing how to register custom metrics;dqx-profile-and-generate/SKILL.mdupdated with the extension point.🤖 Generated with Claude Code