Add ODCS type: library quality metric support

[Christopher-Lawford]

Summary

• Adds native mapping from ODCS type: library quality entries to DQX checks for the five supported metrics: rowCount, nullValues, missingValues, invalidValues, and duplicateValues.
• Each metric maps its eight shared ODCS threshold fields (mustBe, mustNotBe, mustBeGreaterOrEqualTo, mustBeLessOrEqualTo, mustBeGreaterThan, mustBeLessThan, mustBeBetween, mustNotBeBetween) onto exact-fit DQX aggregate checks where possible, with a dataset-level sql_query fallback for strict inequalities and the (both-bounds-exclusive, per ODCS) mustBeBetween/mustNotBeBetween forms.
• Malformed or unrecognized type: library entries (missing/unknown metric, no recognized threshold field, malformed arguments, unrecognized unit, misplaced property/schema-level entries) are warned-and-skipped per entry rather than failing the whole contract; this processing is unconditional, with no opt-out flag.
• Adds end-to-end integration coverage generating and applying rules from a contract exercising all five metrics.
• Adds a new "Metric Rule Generation" guide section (and updates the "Metadata Fields" table) documenting the feature for end users.

Test plan

• tests/unit/test_datacontract_generator.py::TestDataContractGeneratorLibraryRules — unit coverage per metric/threshold-field combination
• tests/unit/test_checks_semantic_validator.py — updated coverage for the semantic validator changes
• tests/integration/test_datacontract_integration.py — end-to-end generation + apply_checks_by_metadata against a real DataFrame for all five metrics
• make test / make lint run clean on this branch (please confirm in CI)

🤖 Generated with Claude Code

[github-actions]

All commits in PR should be signed ('git commit -S ...'). See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

[CLAassistant]

All committers have signed the CLA.

[mwojtyczka]

Out of scope for this PR (should be a follow up): I would implement the missing functions and replace the sql query:

• mustBeGreaterThan (enforce metric > X): new function is_aggr_greater_than(limit=X)
• mustBeLessThan (enforce metric < X): new function is_aggr_less_than(limit=X)
• mustBeBetween (enforce lo < metric < hi): new function is_aggr_in_range(lo, hi)
• mustNotBeBetween (enforce metric ≤ lo OR metric ≥ hi): new function is_aggr_not_in_range(lo, hi)

Can you please create a follow up issue for this?

[mwojtyczka]

Automated code review — ODCS type: library quality metrics

Verified against head d2b6471. One serious correctness bug plus a serialization/validation cluster and a few lower-severity consistency issues. Details are in the inline comments; summary:

Severity	Finding
🔴 High	duplicateValues (any non-zero threshold) nests COUNT(*) OVER (...) inside a SUM/AVG aggregate → Spark AnalysisException at apply time. Only mustBe: 0 is execution-tested.
🟠 Med	nullValues percent and missingValues forbidden embed live F.when/F.lit Column objects in the generated rule dicts → not YAML/JSON-serializable (save_checks fails). Other percent paths already use SQL strings to avoid this.
🟠 Med	Because of the above, ChecksSemanticValidator silently skips conflict detection for those rules (unhashable Column in the key → TypeError swallowed).
🟡 Low	RLIKE string literals don't escape backslashes → regex patterns mangled vs the row-level regex_match path.
🟡 Low	Numeric validValues stringified into NOT IN ('..') → string vs numeric comparison mismatch with is_in_list.
🟡 Low	mustBe == 0 matches boolean False, misses string "0".
🟡 Low	Multiple rowCount entries on one schema collide on rule name.

Cleared: the sql_query fallback for mustBeGreaterThan/mustBeLessThan/mustBeBetween/mustNotBeBetween is correct — DQX has no strict-inequality or between aggregate check (only is_aggr_not_greater_than/not_less_than/equal/not_equal). SQL-injection via threshold interpolation is not viable — those fields are typed float | int.

[mwojtyczka]

Conflict detection is silently skipped for Column-valued arguments.

When a generated check carries a Column as its column argument (the nullValues-percent path in contract_rules_generator.py), _make_hashable returns the Column unchanged (it is neither list/tuple/dict), so the _conflict_key tuple contains an unhashable Column. In _conflict_issue, conflict_key not in seen then raises TypeError, which detect_conflicts catches and skips — so two genuinely conflicting generated rules on the same column are never flagged.

(Duplicate detection via _full_key is unaffected — it stringifies through json.dumps(default=str).) Fixing the root cause — keeping generated column args as SQL strings rather than Column objects — resolves this as well.

[mwojtyczka]

Going in the right direction. Left some comments

[vb-dbrks]

Thanks for taking this on, and for turning the last round around so quickly .. this is careful work and the previous fixes all landed cleanly (no pyspark import in the generator at all now, backslash and numeric literal handling covered with tests, mustBe normalisation handled, rowCount names carrying the threshold field).

Requesting changes. I went back to the ODCS docs and compared our mapping against other ODCS tooling, rather than just reading the code. There are a few places where we would report different numbers than other implementations for the same contract, which matters here because portability is the whole reason to support library rather than telling people to use type: custom. Details are inline; the summary:

• duplicateValues counts rows in duplicated groups, and ODCS tools disagree with each other on this. Ours matches vowl; datacontract-cli counts distinct recurring values instead. The spec does not settle it, so this needs documenting rather than changing.
• mustBeBetween / mustNotBeBetween are exclusive on both bounds, and the docs attribute that to the spec, which does not say it. Other tooling reads it inclusively.
• NULL is counted on the non-zero missingValues paths but not at mustBe: 0, so the metric means two different things depending on the threshold.
• invalidValues with both validValues and pattern becomes two independent thresholds rather than one invalid count.
• _is_dqx_library_rule only matches an explicit type: library, so the feature is inert for contracts written the way the spec documents them. One-line fix.
• duplicateValues still raises at apply time for every non-zero threshold, and the integration test written for it is misplaced and cannot fail.

Where the spec is genuinely silent (which is most of the above) I am not asking you to adopt anyone else's reading .. I would like the choice stated explicitly in the docs, and ideally raised with Bitol so the spec pins it down.

Two product asks

An opt-out flag. generate_rules_from_contract already has generate_predefined_rules and process_text_rules. Making library processing unconditional is inconsistent with that and leaves no escape hatch if a mapping misbehaves on someone's contract. Please add process_library_rules: bool = True alongside the existing flags.

Related: there is no dedup against the predefined path, so required: true plus a nullValues / mustBe: 0 entry on the same property produces two identical is_not_null checks under different names. Not a blocker, but we should pick a behaviour and document it.

Docs should position this as the fallback, not the recommended path. This is the part I feel strongest about, and it is not a criticism of the implementation.

There is no capability gain here. All five metrics were already expressible in DQX .. tolerances via is_aggr_* over a count or percentage indicator, rowCount via column: "*" with count, composite keys via is_unique with a column list, per-check severity via criticality. I checked each one. What this adds is authoring convenience, not new checking ability, and the docs currently read like it is the preferred way to express quality in a contract.

The coverage will also always be lopsided. ODCS library is five metrics. DQX has dozens of checks .. outliers, freshness, foreign keys, schema validation, dataset comparison. Anyone with real requirements outgrows library straight away and ends up in type: custom with engine: dqx, which is the extension point ODCS designed for exactly this. Specific doc asks are inline on the guide.

I would also like the five-metric surface treated as closed once this lands. Supporting library is fine because it is the portable, engine-agnostic part of ODCS, but I do not want it growing into a general ODCS interpreter. Every addition is permanent maintenance keyed to a vocabulary we do not control.

Scope

Per the principle Marcin set out for the new aggregate functions, the checks_semantic_validator.py change should also come out of this PR .. it is a core change and this PR no longer needs it. Comment inline.

+1 to Marcin's follow-up for is_aggr_greater_than / is_aggr_less_than / is_aggr_in_range / is_aggr_not_in_range. Worth having in core regardless of ODCS, and it would let most of these sql_query fallbacks go away.

One transparency note: parts of this review were machine-assisted, and the apply-time claims (the duplicateValues InvalidParameterError in particular) are read from the source rather than reproduced against Spark, so please sanity-check those as you go.

[vb-dbrks]

This makes the whole feature a no-op for most real contracts.

ODCS says the type "can be omitted, if a metric property is defined", and every library example in the spec omits it. Against open-data-contract-standard 3.1.2 an omitted type parses as None, so those entries fall straight through here and generate nothing. As it stands we do nothing for contracts written the way the spec documents them, which is exactly the silent drop #1424 was about.

It is not caught because every fixture and test entry sets type: library explicitly. Suggest:

return quality_rule.type == 'library' or (quality_rule.type is None and quality_rule.metric is not None)

plus a fixture with no type. Also worth deciding what to do with the deprecated rule: key, since contracts authored against ODCS 3.0.x use that instead of metric:.

[vb-dbrks]

Our duplicate count differs from other ODCS tools, and the spec does not settle which is right.

This sums group sizes, so it is the number of rows sitting in a duplicated group. The ecosystem is genuinely split:

• datacontract-cli delegates to Soda's duplicate_count, which counts distinct values that recur: WITH frequencies AS (SELECT COUNT(*) AS frequency FROM t GROUP BY cols) SELECT COUNT(*) FROM frequencies WHERE frequency > 1
• vowl counts participating rows, same as us. Its changelog says it moved to that deliberately, so that actual_value / failed_rows_count match the annotated row count.

For a column holding [A, A, A, B, B, C] we and vowl report 5, datacontract-cli reports 2. Percent diverges too: we do duplicate_rows / total_rows (83%), Soda does duplicate_count * 100 / row_count (33%). mustBe: 0 is unaffected since every reading agrees at zero.

The ODCS docs only say "Counts duplicate values in a column", and ODCS designates no reference implementation, so this is a judgement call and yours is defensible. I am not asking you to change it. Two things instead:

• document the choice explicitly in the guide, with the arithmetic, so someone comparing DQX output against another ODCS tool is not surprised. Same for the percent denominator.
• consider raising it with Bitol. There is no open issue on duplicateValues semantics and at least three defensible readings in the wild, so the spec is the right place to pin this down and you are well placed to ask.

[vb-dbrks]

This still fails at apply time, and the test written for it cannot catch it.

The window-inside-an-aggregate is gone, but the replacement has the same practical outcome. This emits SELECT <scalar subquery> <op> N AS condition FROM {{ input_view }} with no aggregate in the outer select, so it returns one row per input row. sql_query's dataset-level path requires exactly one row and raises InvalidParameterError. That is all seven non-zero threshold forms, including the composite-key example in the new docs.

Selecting from a single-row source, or wrapping the comparison in an aggregate, should sort it. Flagging that I read this from the source rather than reproducing it against Spark, so please confirm when you add execution coverage.

[vb-dbrks]

The docs attribute this behaviour to the spec, and the spec does not say it.

The comments and new docs state both bounds are exclusive "per the ODCS specification". I cannot find that anywhere. The docs and JSON schema say only "Must be between the two numbers to be valid. Smallest number first in the array.", and the spec repo notes that where the JSON Schema and the standard conflict the standard takes precedence, so neither artifact settles it.

Other tooling reads it inclusively .. datacontract-cli maps it to SodaCL between X and Y, and Soda excludes a bound only when it is written with a round bracket. So mustBeBetween: [0, 5] with a rowCount of exactly 5 is a violation for us and a pass there.

Since the spec is silent this is a judgement call like duplicateValues, so I am not insisting on inclusive. But please either drop the "per the ODCS specification" claim or cite what you were working from, and state the choice explicitly in the docs. Worth settling before the range functions land, since whatever we pick has to carry into is_aggr_in_range / is_aggr_not_in_range.

[vb-dbrks]

NULL handling differs between the mustBe: 0 path and every other threshold.

_missing_values_condition_sql always emits col IS NULL, but this path gates the null check on null appearing explicitly in the list. So missingValues: ["N/A"] means "N/A only" at mustBe: 0 and "N/A or NULL" everywhere else. That is our own inconsistency regardless of what any other tool does .. the metric ends up meaning two different things depending on the threshold.

For context, datacontract-cli strips null out of arguments.missingValues and relies on Soda's missing condition, which is seeded with column IS NULL before the configured values are appended, so NULL counts always and listing it is a no-op. That lines up with our non-zero path. Suggest counting NULL unconditionally on both paths and treating a null entry as redundant.

[vb-dbrks]

Good catch on the bare-string-resolves-as-a-column-expression trap, that is an easy one to miss.

One gap though: this escapes quotes but not backslashes, while _sql_scalar_literal does both. So a \N sentinel is compared differently on the mustBe: 0 path than on the non-zero paths for the same contract value. Worth aligning the two.

[vb-dbrks]

There are 5 comments pointing at .scratch/odcs-library-metrics/issues/*.md, which are not in the repo, so nobody reading this later can follow them. Please inline the reasoning or drop the references.

[vb-dbrks]

This is a legitimate fix on its own merits, but it is a core change and I do not think this PR needs it any more .. the generator no longer imports pyspark at all, so it cannot produce the Column that the truthiness fix guards against.

Same principle Marcin set out for the new aggregate functions: please split this into a standalone hardening PR so this one stays confined to the datacontract module.

Related and also for a separate PR, but worth raising while we are here: _conflict_key is (function, column), so with column: "*" two nullValues entries on different columns both key as ("is_aggr_not_greater_than", "*") and get reported as conflicting on a perfectly valid contract. Including row_filter in the key would fix it.

[vb-dbrks]

This test cannot catch the bug it was written for. The quality block sits on the schema object rather than on the order_id property, so schema-level duplicateValues looks for arguments.properties, finds nothing, warns and returns no rules .. so assert len(rules) == 1 fails before it ever applies anything. The later assertions reference order_id_duplicateValues, the property-level name, so the intent was clearly to nest it inside the property.

[vb-dbrks]

Request on framing for this section, rather than the mechanics.

I would like the docs to position library metrics as the option for simple contracts, not as the recommended way to express quality in an ODCS contract. Concretely:

• say plainly that library metrics are intended for simple contracts
• point at type: custom with engine: dqx as the recommended path for anything beyond the five metrics
• drop the framing that suggests this is the primary way to express quality
• where our semantics are a judgement call the spec does not settle (duplicateValues, the percent denominator, the invalidValues combination), say so explicitly and say what we chose

Reasoning is in the review summary.