feat(detector): compare distinct ratio for historically unique columns (#94)

[bferanmi806-sketch]

Summary

Closes #94. When the stored profile shows a column was near-unique (distinct_count / row_count >= 0.95) compare the distinct ratio (distinct / rows) instead of the absolute distinct count. A 60% row deletion on a unique id column then stays silent for cardinality (volume_drop still fires), while duplicates on a historically unique column still trigger warning or critical using the existing thresholds.

Changes

• src/scherlok/detector/cardinality.py: add UNIQUE_RATIO = 0.95 with comment, extend detect_cardinality_anomalies to accept optional current_vol and stored_vol, implement ratio branch with safe fallback for missing or zero row counts, message mentions distinct ratio when that path is used. Preserves adaptive history behaviour unchanged.
• src/scherlok/service.py: wire current_vol and stored_vol into detect_cardinality_anomalies call.

Tests

• Existing tests/test_detector.py 43 passed unchanged.
• Manual verification:
  • 60% deletion on unique id (1000 rows 1000 distinct -> 400 rows 400 distinct) produces volume_drop only, no cardinality.
  • Duplicates on unique column (1000 rows 1000 distinct -> 1000 rows 500 distinct) fires WARNING with distinct ratio and 50% change.
  • Low-cardinality status (5 distinct) keeps absolute thresholds: stable 5->5 gives no anomaly, 5->500 gives CRITICAL with distinct values message.
  • Missing or zero row counts fallback to absolute: None or 0 row counts give WARNING for 100->160 (60% change).
  • No-volume call (existing tests) still uses absolute.

Notes

• Preserves CARDINALITY_WARNING_PCT=50 and CRITICAL=200 for ordinary columns and for ratio comparison.
• Safe handling of missing or zero row counts falls back to absolute comparison.
• No breaking changes to existing detector tests.

[rbmuller]

Thanks for picking this up. The approach is right, the ratio branch reads well, and it leaves the adaptive-history path alone, which is what I hoped for.

Two things before this can go in.

1. It needs tests

Every PR here carries unit tests, and #94 asked for the regression case specifically. I wrote them against your branch so you don't have to start from scratch — please drop this into tests/test_detector.py (it follows the existing TestCardinalityDetector style, right below it):

class TestCardinalityNearUniqueColumns:
    """Distinct ratio, not distinct count, for columns that were near-unique.

    Regression for #94: deleting rows also drops the distinct count of a
    unique `id` or timestamp column, so one volume drop used to fire a
    cardinality warning per unique column on top of `volume_drop`.
    """

    # A 1,000-row table whose `id` column is unique.
    STORED_DIST = {"distinct_count": 1000}
    STORED_VOL = {"row_count": 1000}

    def test_volume_drop_on_unique_column_is_silent(self):
        """60% of rows deleted: the id column is still unique, so nothing fires."""
        anomalies = detect_cardinality_anomalies(
            "orders",
            "id",
            {"distinct_count": 400},
            self.STORED_DIST,
            {"row_count": 400},
            self.STORED_VOL,
        )
        assert anomalies == []

    def test_duplicates_on_unique_column_still_fire(self):
        """Same row count, half the distinct values: uniqueness broke, so it fires."""
        anomalies = detect_cardinality_anomalies(
            "orders",
            "id",
            {"distinct_count": 500},
            self.STORED_DIST,
            {"row_count": 1000},
            self.STORED_VOL,
        )
        assert len(anomalies) == 1
        assert anomalies[0]["severity"] == Severity.WARNING
        assert "decreased" in anomalies[0]["message"]
        assert "distinct ratio" in anomalies[0]["message"]

    def test_low_cardinality_column_keeps_absolute_comparison(self):
        """A status column exploding 5 -> 605 must still be CRITICAL after a volume drop."""
        anomalies = detect_cardinality_anomalies(
            "users",
            "plan",
            {"distinct_count": 605},
            {"distinct_count": 5},
            {"row_count": 400},
            {"row_count": 1000},
        )
        assert len(anomalies) == 1
        assert anomalies[0]["severity"] == Severity.CRITICAL
        assert "distinct values" in anomalies[0]["message"]

    def test_falls_back_to_absolute_without_volume_profiles(self):
        """Callers that pass no volume profiles keep the previous behaviour."""
        anomalies = detect_cardinality_anomalies(
            "orders", "id", {"distinct_count": 400}, self.STORED_DIST
        )
        assert len(anomalies) == 1
        assert anomalies[0]["severity"] == Severity.WARNING

    def test_falls_back_to_absolute_on_zero_row_counts(self):
        """An empty stored profile can't yield a ratio; don't divide by zero."""
        anomalies = detect_cardinality_anomalies(
            "orders",
            "id",
            {"distinct_count": 400},
            self.STORED_DIST,
            {"row_count": 0},
            {"row_count": 0},
        )
        assert len(anomalies) == 1
        assert anomalies[0]["severity"] == Severity.WARNING

    def test_column_just_below_the_unique_threshold_uses_absolute(self):
        """90% distinct is not near-unique, so the absolute path still applies."""
        anomalies = detect_cardinality_anomalies(
            "orders",
            "customer_id",
            {"distinct_count": 360},
            {"distinct_count": 900},
            {"row_count": 400},
            {"row_count": 1000},
        )
        assert len(anomalies) == 1
        assert "distinct values" in anomalies[0]["message"]

All six pass on your branch. On main the first one fails with the extra cardinality warning from #94, which is the regression we want locked down; the rest fail on the old signature, which is the point.

2. The CRITICAL branch on the ratio path can never run

The ratio branch only executes when stored_ratio >= UNIQUE_RATIO (0.95), and any ratio is in [0, 1], so the change is bounded:

case	change_pct
every value duplicated (1.0 → 0.0)	100%
0.95 → perfectly unique	5.3%

CARDINALITY_CRITICAL_PCT is 200, so if change_pct >= CARDINALITY_CRITICAL_PCT is dead code, and a unique id column that collapses to 1% distinct — real corruption — reports WARNING.

The thresholds were written for absolute counts, where 200% means "3x"; they don't carry over to a bounded ratio. Two ways out, your call:

• Ratio-specific thresholds, e.g. UNIQUE_RATIO_WARNING_PCT = 10 / UNIQUE_RATIO_CRITICAL_PCT = 50, so losing half the uniqueness of a key column is CRITICAL. My preference, and then a seventh test asserting CRITICAL on 1000 → 100 distinct over 1000 rows is worth adding.
• Drop the CRITICAL branch and document in the docstring that the ratio path only ever warns.

3. Minor

if stored_ratio == 0: return anomalies is unreachable inside a block already guarded by stored_ratio >= 0.95. Worth deleting.

Happy to review again as soon as you push. Good catch on keeping the fallback for missing and zero row counts.

[bferanmi806-sketch]

Thanks for the detailed review. I pushed commit 391b4eb with the requested follow-up.

• Added the six near-unique regression tests to tests/test_detector.py, plus a seventh test covering a 90% uniqueness loss as CRITICAL.
• Added ratio-specific thresholds: 10% warning and 50% critical boundary, instead of reusing the absolute 200% threshold.
• Removed the unreachable stored_ratio == 0 guard.
• Preserved the absolute-count path for low-cardinality columns and missing/zero volume profiles.

Validation on the branch:

• Focused cardinality tests: 7 passed
• Full suite: 440 passed, 1 skipped
• ruff check src/ tests/: passed
• mkdocs build --strict: passed

One boundary detail: the supplied test_duplicates_on_unique_column_still_fire expects exactly 50% loss (1000 → 500) to remain WARNING, while the review text describes 50% as CRITICAL. I followed the executable regression case, so the implementation treats losses strictly above 50% as critical; the 90% case is critical. If you intended 50% inclusive, I can adjust that assertion and comparison.

[rbmuller]

Both of your points land. Two small things:

Boundary — you followed my test, and my test was the wrong one. >= on both would match the absolute path below, which makes an exact 50% loss CRITICAL. Either way works; I'd just rather not mix > and >= in one file.

On the warning-band case: 1 - 0.9 is 0.09999999999999998, so a clean 10% falls just under the threshold. 1000 -> 800 stays clear of that edge.

NULLs in the denominator — scherlok demo on your branch gives 5 anomalies instead of 4: users.email fires a cardinality change next to the NULL alert for the same event. The deploy nulls 20% of a unique column, and the 1,600 remaining values are still distinct:

denominator	before	after
all rows	1.000	0.800
non-null rows	1.000	1.000

null_count is already in every distribution profile, so rows - nulls as the denominator covers it. I tried it locally: demo back to 4, suite green. What do you think?

[bferanmi806-sketch]

Thanks for catching both! I agree with using non-NULL rows as the denominator. I’ll update the threshold comparisons to >=, handle the floating-point boundary, and add regression tests for NULLs and uniqueness loss. I’ll also rerun the full suite and demo before pushing the follow-up.

[rbmuller]

Verified on your branch: 445 tests green, scherlok demo back to 4 anomalies, and every boundary case behaves as specified — silent on a volume drop, silent on NULLed unique values, WARNING at exactly 10%, CRITICAL at exactly 50%, and the low-cardinality path untouched. The epsilon on the comparisons is a nice touch.

Merging. Thanks for the careful back-and-forth on this one.