Skip to content

Competitive-gap closure: validation suites, fallback policy, ER accuracy evidence, 25M benchmarks, CI honesty#134

Merged
JohnnyWilson16 merged 7 commits into
mainfrom
feat/competitive-gaps-jwd
Jul 12, 2026
Merged

Competitive-gap closure: validation suites, fallback policy, ER accuracy evidence, 25M benchmarks, CI honesty#134
JohnnyWilson16 merged 7 commits into
mainfrom
feat/competitive-gaps-jwd

Conversation

@JohnnyWilson16

@JohnnyWilson16 JohnnyWilson16 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Competitive-gap closure: validation suites, fallback policy, ER accuracy evidence, 25M-row benchmarks, CI honesty

Closes the specific technical gaps where freshdata loses to Great Expectations (validation), OpenRefine (dedupe/ER), PyJanitor (API ergonomics), and pandas (performance evidence). Implementation + tests + committed benchmark evidence + honest docs — no marketing changes.

Phase-1 findings this PR responds to (verified against main + CI run 29158260290)

  • Validation was three disjoint pandas-only surfaces with no suite API, no CLI, no failure tolerance; strict_columns was declared but never read; datetime ranges silently no-op'd (pd.to_numeric coerced datetimes to NaN and the check passed).
  • The default strategy="balanced" silently routed every native-engine run through a whole-pipeline pandas fallback; no way to refuse or even be warned. Reports lacked requested-backend, peak-memory, and rows-materialized fields.
  • ER's module headline said "Probabilistic (Splink-style)" while the score is a normalized weighted average; the P/R/F1 harness existed but no accuracy results were committed; missing sides scored 0.0 and dragged sparse records down.
  • Committed performance evidence topped out at 10M rows; the reference CI run was green with the large-data lane skipped (if: schedule + continue-on-error: true — it could not fail even when it ran).
  • No plugin points for comparators or exporters; no fluent pipeline builder.

What landed (one commit per phase)

  1. feat(validation)fd.ValidationSuite / fd.ColumnRule / fd.CrossColumnRule / raise_if_failed() as a facade compiling onto the existing DataContract engine (one engine, two front doors; from_contract() migrates). New primitives on that engine: GX-style mostly= tolerance, string lengths, datetime bounds (+ regression fix for the silent no-op), dtype_exact, dataset min_rows/max_rows/compound_unique, live strict_columns, enforce_contract(). CLI freshdata validate with exit codes 0/1/2. Validation stays read-only; non-pandas inputs record their materialization.
  2. feat(execution)fallback_policy="allow"|"warn"|"error" on EngineConfig/fd.clean/CLI; "error" raises before any pandas materialization at every backend delegation point. CleanReport gains requested_backend, peak_memory, rows_materialized. freshcore added to CLI engine choices.
  3. feat(polars) — native subset dedup (duplicate_subset + keep first/last) with byte-for-byte pandas keep-semantics parity (nulls/unicode/multi-column tested); one fewer fallback row in the matrix.
  4. feat(er)token_set + metaphone comparators; null_policy="neutral" (missing = no evidence, not disagreement); mode="precision"|"recall" presets that raise on threshold conflict; honest "rule-weighted" naming throughout; committed labelled benchmark: 5k-row dataset (typos, reordered names, abbreviations, blanked fields, transliteration, household non-matches, near-collisions) + er_results.json with P/R/F1, FP/FN, false-merge counts, blocking reduction per config, plus the pandas exact-dedupe baseline (recall 0.054).
  5. feat(api)fd.plan() (per-column choices plus the backend/fallback verdict before execution) + fd.apply alias; fd.pipeline() immutable fluent builder compiling 1:1 to CleanConfig (same fallback matrix, reports, audit trail; JSON round-trip).
  6. feat(plugins)freshdata.comparators + freshdata.exporters entry-point groups with the existing safety discipline (reserved names, output clamping, exception isolation), fd.export(report, format=...), contract helpers, worked examples.
  7. bench/CI — reference benchmark extended to 25M rows (10k→25M, pandas/polars/duckdb, wall + peak RSS + throughput + trust parity, machine/versions/command recorded in src/freshdata/benchmarks/RESULTS.md). nightly-online-large can now actually fail (removed continue-on-error) and failures open a pinned nightly-failure issue (same for cleanbench-full & perf-regression); new large-data-status job on every PR asserts the lane still collects tests and states in the step summary that it runs nightly; new ~1M-row large_smoke tests run in the PR lane.
  8. docsdocs/comparison.md (win/loss/tie/unproven vs pandas/PyJanitor/GX/OpenRefine, every win linked to committed evidence, losses stated plainly), docs/validation.md (four surfaces, limitations first), docs/fallback-matrix.md (transcribed from PlanGenerator.fallback_reason()), ER accuracy table in docs/benchmarks.md, trust-claims additions.

What this PR deliberately does NOT do

  • No native strategy="balanced" (would fork the decision engine per backend) — documented loudly instead, and fallback_policy makes the fallback refusable.
  • No Fellegi–Sunter/EM linkage — terminology corrected instead, per the "name the method accurately" rule.
  • No optimize_memory native port (pandas-specific downcasting by design).
  • No 100M/1B-row claims — harness keys exist, committed evidence stops at 25M and the docs say so.

Verification

  • Full fast suite + coverage gate (≥93%), ruff, mypy (188 files), python -m build + twine check, mkdocs build, CLI smoke (exit codes 0/1/2), import smoke on all new lazy exports — all green locally; see final session report for exact commands and outputs.
  • Backward compatibility: all additions are additive; ColumnContract/DataContract fields appended with defaults (old JSON loads unchanged); ComparisonLevel.kind relaxed from Literal to validated str (runtime behavior unchanged for built-ins); no public signature removed or changed.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added fluent, serializable cleaning pipelines with planning and apply helpers.
    • Added validation suites, cross-column rules, contract enforcement, CLI validation, and detailed results.
    • Added configurable backend fallback policies and execution metadata.
    • Added comparator and exporter plugins, including entity-resolution improvements and new benchmark modes.
    • Added native Polars subset deduplication and support for larger benchmarks.
  • Documentation

    • Expanded API, validation, fallback, plugin, benchmark, comparison, and limitations documentation.
  • Bug Fixes

    • Improved fallback visibility, failure reporting, and nightly workflow alerts.
    • Added large-data test guardrails and one-million-row smoke coverage.

JohnnyWilson16 and others added 7 commits July 12, 2026 12:21
… engine

- New fd.ValidationSuite / fd.ColumnRule / fd.CrossColumnRule / fd.ValidationResult
  with raise_if_failed(), JSON round-trip (freshdata-suite-v1), save/load, and
  ValidationSuite.from_contract() migration from existing DataContracts.
- Contract engine upgrades (same engine, no parallel system):
  * mostly= per-column failure tolerance (GX-style) on value-level checks
  * min_length/max_length string-length rules
  * min_datetime/max_datetime bounds; numeric range on datetime columns now
    warns instead of silently no-op'ing (pd.to_numeric coercion bug)
  * dtype_exact opt-in raw dtype comparison
  * dataset-level min_rows/max_rows and compound_unique groups
  * strict_columns flag is now enforced (was declared but never read)
  * new enforce_contract(df, contract) — contract-only check, no baseline
- fd.validate(df, suite=...) front door (mutually exclusive with context/policy).
- CLI: freshdata validate <file> --suite|--contract [--json OUT]; exit 0/1/2.
- Validation stays read-only; non-pandas inputs record a materialization event
  on ValidationResult.execution instead of silently converting.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… report fields

- EngineConfig.fallback_policy: 'allow' records (as before), 'warn' emits
  FallbackWarning, 'error' raises FallbackError *before* any pandas
  materialization — a user asking for out-of-core execution can now make an
  unrequested full-frame pandas pull impossible. Enforced at every backend
  delegation point (polars/duckdb/spark/freshcore + the semantic fallback);
  error messages quote the exact trigger and the native escape hatch.
- fd.clean(fallback_policy=...) passthrough (rejects engine='pandas', which
  cannot fall back); CLI clean --fallback-policy and freshcore in --engine.
- CleanReport gains requested_backend (pre-auto-resolution engine),
  peak_memory (ru_maxrss, platform-normalized, None on Windows), and
  rows_materialized (rows actually pulled into memory; None for native
  handles). All serialized by to_dict. engine='auto' now stamps the resolved
  backend on the report.
- fd.FallbackError / fd.FallbackWarning exported at top level.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PlanGenerator is now backend-aware; duplicate_subset with keep=first/last
routes natively on Polars via unique(subset=..., maintain_order=True), which
reproduces pandas order-sensitive keep semantics byte-for-byte (parity tests
incl. nulls, unicode, multi-column). Order preservation makes this stage
eager rather than streaming; that is disclosed on the report. keep=drop/
aggregate and other backends keep the disclosed pandas fallback.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… labelled accuracy benchmark

- New comparison kinds: token_set (Jaccard over token sets — reordered names)
  and metaphone (second phonetic key; models silent K/PH->F/TH that Soundex's
  fixed classes miss). Pure-Python, dependency-free.
- null_policy='neutral': a missing side removes the field from the weight sum
  instead of scoring 0.0 — sparse records stop dragging scores down. Ignored
  fields are still listed in the pair explanation. Default stays 'penalize'
  (backward compatible).
- mode='precision'|'recall' threshold presets; combining a preset with
  explicit thresholds raises instead of silently overriding.
- Honest naming: module/config docstrings now say 'rule-weighted linkage —
  normalized weighted average, no m/u-probabilities, no EM, no Fellegi-Sunter,
  no Splink parity' instead of the 'Probabilistic (Splink-style)' headline.
- Labelled accuracy benchmark: committed generator (gen_er_labelled.py),
  committed 5k-row dataset with typos/reordered names/abbreviations/missing
  fields/transliteration/household non-matches/near-collisions, committed
  er_results.json: P/R/F1, FP/FN, false-merge counts, blocking reduction,
  runtime per null_policy x mode + a pandas exact-dedupe baseline
  (recall 0.054 — the gap fuzzy linkage closes).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ent builder

- fd.plan(df, strategy=, engine=): previews per-column choices AND the
  execution verdict before anything runs — plan.backend + plan.fallback_reason
  (from the data-free PlanGenerator) say whether the chosen engine runs the
  config natively or would delegate to pandas, surfaced in summary()/to_dict().
  fd.apply = alias of apply_plan (risk/reversibility per action live on the
  RepairPlan as before).
- fd.pipeline(): immutable fluent builder (PyJanitor-style) —
  .normalize_columns().normalize_missing().validate_types()
  .impute(strategy=, columns=).deduplicate(subset=).outliers().run(df).
  Each step maps 1:1 to a CleanConfig field and ONLY chained steps are
  enabled, so the fallback matrix, reports, and audit trail are identical to
  fd.clean — no second execution path. JSON round-trip
  (freshdata-pipeline-v1), value equality, duplicate/unknown-step validation,
  engine/fallback_policy passthrough on run().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…arators / freshdata.exporters)

Two new plugin kinds through the existing one-registry mechanism (entry
points or fd.register_*), with the same safety discipline as
experts/backends/validators:

- ER comparators: named callable (a, b) -> [0,1]; usable directly as
  ComparisonLevel(kind=<name>); built-in kind names are reserved (explicit
  registration raises, entry-point shadowing registers inactive); _Safe
  wrapper clamps output to [0,1] and turns exceptions into a skipped field,
  never a distorted score.
- Report exporters: name + export(report) -> str | dict, consumed via
  fd.export(report, format=<name>, path=...); wrong-type output raises.
- Contract-test helpers fd.testing.comparator_contract / exporter_contract
  (range, determinism, symmetry, identity=1.0, no-mutation).
- Worked examples: examples/plugins/custom_comparator/initials_comparator.py,
  examples/plugins/custom_exporter/markdown_exporter.py — both contract-tested.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…idation/fallback docs

Benchmarks:
- New 25m size key; reference run re-executed 10k -> 25M for
  pandas/polars/duckdb with full provenance (Apple M2 16GB, Python 3.12.13,
  pandas 2.3.3, polars 1.42.1, duckdb 1.5.4; exact command + seed + dataset
  shape recorded). Raw results JSON/MD committed. Trust-score parity holds
  at every size including 25M.
- Honest 25M caveat documented: gaps narrow and native-engine RSS exceeds
  pandas on the materializing path under memory pressure; the native-handle
  path is the larger-than-RAM lever. No claims beyond 25M.

CI honesty:
- nightly-online-large: removed continue-on-error (a failing nightly now
  fails); on failure it opens/refreshes a pinned 'nightly-failure' issue.
  Same alert step added to cleanbench-full and perf-regression.
- New large-data-status job on every push/PR: fails if the large/online
  markers stop collecting tests (rotted lane) and writes a step summary
  stating the lane runs nightly — skipped is now visible, not silent green.
- New large_smoke marker + tests: ~1M-row deterministic clean + polars
  parity in the PR lane (<10s), so real-scale execution is continuous.

Docs:
- docs/comparison.md: win/loss/tie/unproven vs pandas/PyJanitor/GX/OpenRefine;
  every win links committed evidence, losses stated plainly (incl. the 25M
  memory loss row), verdict computed from the tables.
- docs/validation.md: four validation surfaces, limitations before examples,
  suite quickstart + migration. docs/fallback-matrix.md: transcription of
  PlanGenerator.fallback_reason() + fallback_policy guide (cross-referenced
  from the code).
- ER labelled-accuracy table in docs/benchmarks.md (P/R/F1/FP/FN/false-merges
  per config + honest readings). trust-claims.md: 6 new evidence-mapped
  claims. limitations/backends/plugins/api-reference/mkdocs nav updated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@strix-security

Copy link
Copy Markdown
Contributor

Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds fallback-policy enforcement and execution reporting, a fluent pipeline API, declarative validation suites, comparator/exporter plugins, labelled entity-resolution benchmarks, expanded benchmark coverage, CI failure alerts, and documentation for the new APIs and behaviors.

Changes

Execution and API workflows

Layer / File(s) Summary
Fallback policy and execution reporting
src/freshdata/api.py, src/freshdata/execution/*, src/freshdata/execution/backends/*, src/freshdata/report.py, tests/test_execution/*
Native backends now enforce allow, warn, or error fallback behavior and record backend, materialization, and memory metadata.
Pipeline builder and API front doors
src/freshdata/pipeline.py, src/freshdata/api.py, src/freshdata/plan.py, src/freshdata/__init__.py, tests/test_pipeline_builder.py
Adds immutable fluent pipelines, planning/apply helpers, exporter dispatch, serialization, and top-level exports.

Validation suites and contracts

Layer / File(s) Summary
Contract constraint engine
src/freshdata/enterprise/contracts.py
Contracts now support tolerance-aware checks, row bounds, compound uniqueness, string/datetime constraints, exact dtypes, and direct enforcement.
Validation suite execution and CLI
src/freshdata/validation_suite.py, src/freshdata/api.py, src/freshdata/enterprise/cli.py, tests/test_validation_suite.py
Adds serializable validation suites, cross-column rules, execution metadata, result handling, and a validation CLI command.

Entity resolution and plugins

Layer / File(s) Summary
Entity-resolution comparison and evaluation
src/freshdata/enterprise/config.py, src/freshdata/enterprise/entity_resolution.py, tests/test_er_comparators.py
Adds token-set and Metaphone comparisons, comparator plugins, null policies, threshold modes, and related scoring tests.
Comparator and exporter plugin system
src/freshdata/plugins.py, src/freshdata/testing.py, examples/plugins/*, tests/test_plugin_comparators_exporters.py
Adds registration, discovery, safety wrappers, contracts, examples, and end-to-end tests for comparator and exporter plugins.

Benchmarks, CI, and documentation

Layer / File(s) Summary
Labelled entity-resolution benchmark
benchmarks/*, docs/benchmarks.md
Adds deterministic labelled-data generation, baseline and configuration sweeps, metrics, results JSON, and reproduction documentation.
Benchmark scale and CI guardrails
src/freshdata/benchmarks/*, tests/test_large_smoke.py, .github/workflows/*, pyproject.toml
Adds 25M-row benchmark support, a 1M-row smoke lane, marker collection checks, and failure issue alerts for nightly workflows.
Public API and feature documentation
docs/*, mkdocs.yml
Documents validation suites, fallback behavior, plugin extension points, comparisons, limitations, trust claims, and new navigation entries.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.51% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change areas and matches the changeset.
Description check ✅ Passed The description is detailed and covers the main changes, but it omits the template's issue reference and checklist sections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/competitive-gaps-jwd

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

FreshData benchmark report — 2026-07-12T10:30:06Z

  • freshdata: 1.1.1
  • python: 3.12.13
  • platform: Linux-6.17.0-1018-azure-x86_64-with-glibc2.39
fixture n_rows n_cols p50 s p95 s peak MB repair % false-repair % preserve % trust monotonic export %
crm 10,200 40 1.567 1.572 13.8 100.0 0.0 100.0 93.84 100.0
event_log 10,000 25 0.610 0.614 5.1 100.0 0.0 100.0 99.677 100.0
finance 10,200 60 1.968 1.974 18.3 100.0 0.0 100.0 99.499 100.0
gold 10,200 7 0.229 0.232 3.7 100.0 0.0 100.0 98.3 100.0
provenance 10,000 18 0.503 0.509 7.2 100.0 0.0 100.0 99.765 100.0
wide_schema 10,000 100 3.685 3.696 13.7 100.0 0.0 100.0 96.042 100.0

Authored-code reduction (Metric 6)

  • FreshData: 3 lines
  • pandas baseline: 25 lines (88.0% reduction)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/freshdata/enterprise/config.py (1)

419-439: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject inactive comparator names in ComparisonLevel known_comparator_names() includes inactive registrations, but scoring only uses get_active_comparator(), so a config can validate and still skip that field at runtime. Consider validating against active comparators here instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/freshdata/enterprise/config.py` around lines 419 - 439, Update
ComparisonLevel.__post_init__ to validate non-built-in kinds against active
comparator registrations using the same active-comparator lookup used by
scoring, rather than known_comparator_names(). Preserve the existing ValueError
message and built-in kind handling while rejecting inactive or unavailable
comparator plugins during configuration validation.
src/freshdata/execution/backends/_polars.py (1)

406-426: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Subset-column validation is skipped for empty frames.

n_before < 1 triggers an early return lf (line 415-416) before the duplicate_subset existence check (lines 417-426) runs. A nonexistent duplicate_subset column therefore silently passes validation whenever the frame reaching this stage happens to have 0 rows, but correctly raises ValueError for the same config on a non-empty frame. This makes error behavior depend on incidental row count rather than being a pure structural/schema check.

🐛 Proposed fix: validate subset columns before the row-count short-circuit
-        n_before = int(lf.select(pl.len()).collect().item())
-        if n_before < 1:
-            return lf
-        subset = list(config.duplicate_subset) if config.duplicate_subset is not None else None
-        if subset:
+        subset = list(config.duplicate_subset) if config.duplicate_subset is not None else None
+        if subset:
             schema_names = set(lf.collect_schema().names())
             missing = [c for c in subset if c not in schema_names]
             if missing:
                 raise ValueError(
                     f"duplicate_subset column(s) not found: {missing}. "
                     f"Available columns: {sorted(schema_names)}. "
                     "Note: names refer to columns *after* renaming when column_names=True."
                 )
+        n_before = int(lf.select(pl.len()).collect().item())
+        if n_before < 1:
+            return lf
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/freshdata/execution/backends/_polars.py` around lines 406 - 426, Move the
duplicate_subset schema validation in _stage_drop_duplicates before the n_before
< 1 early return. Ensure missing subset columns always raise the existing
ValueError, including for empty frames, while retaining the current
short-circuit after validation.
🧹 Nitpick comments (7)
docs/fallback-matrix.md (1)

59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer "afterward" for American English consistency.

The heading "What the report tells you afterwards" uses the British variant while the rest of the documentation uses American English (e.g., "behavior", "normalize"). Consider "afterward" for consistency.

✏️ Suggested fix
-## What the report tells you afterwards
+## What the report tells you afterward
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/fallback-matrix.md` at line 59, Update the “What the report tells you
afterwards” heading to use the American English term “afterward,” leaving the
rest of the documentation unchanged.

Source: Linters/SAST tools

examples/plugins/custom_exporter/markdown_exporter.py (1)

28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing explicit max_risk — defaults to "high".

Unlike the sibling InitialsComparator example (max_risk = "low"), this exporter doesn't declare max_risk, so _check_metadata's getattr(plugin, "max_risk", "high") fallback marks it "high" risk in registered_plugins() introspection — misleading for a benign, local Markdown renderer, and a bad template for third-party plugin authors copying this example.

♻️ Suggested fix
 class MarkdownExporter:
     """Render the report's headline numbers and action log as Markdown."""

     name = "markdown"
+    max_risk = "low"
     uses_network = False
     requires: tuple[str, ...] = ()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/plugins/custom_exporter/markdown_exporter.py` around lines 28 - 34,
Declare an explicit low-risk max_risk value on the MarkdownExporter class,
alongside name, uses_network, and requires, so registered_plugins() metadata
correctly identifies this local renderer as benign and the example provides the
intended template for plugin authors.
src/freshdata/plugins.py (1)

517-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Naming asymmetry between comparator and exporter accessors.

known_comparator_names() returns all registered names (active or not) while active_exporter_names() returns only active ones — reasonable given their different consumers, but the naming convention ("known_" vs "active_") doesn't make that distinction self-evident to future readers/API consumers. Worth a brief docstring cross-reference or a consistent naming scheme.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/freshdata/plugins.py` around lines 517 - 556, The comparator and exporter
name accessors use inconsistent naming without documenting their differing
active/inactive semantics. Add brief docstrings to known_comparator_names and
active_exporter_names that explicitly state whether inactive registrations are
included and cross-reference the other accessor’s behavior, without changing
their return values.
.github/workflows/cleanbench-full.yml (1)

44-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated "open/refresh alert issue" script across workflows.

This exact list-by-label → find-by-title → comment-or-create script is repeated verbatim (only title/body differ) in .github/workflows/ci.yml's existing step and again in .github/workflows/perf-regression.yml. Consider extracting it into a reusable composite action taking title/body/label inputs so the alerting logic has one source of truth.

# .github/actions/notify-failure/action.yml
name: Open/refresh alert issue on failure
inputs:
  title: { required: true }
  body: { required: true }
  label: { default: "nightly-failure" }
runs:
  using: composite
  steps:
    - uses: actions/github-script@v7
      with:
        script: |
          const title = "${{ inputs.title }}";
          const body = `${{ inputs.body }}`;
          const label = "${{ inputs.label }}";
          const open = await github.rest.issues.listForRepo({
            ...context.repo, state: "open", labels: label,
          });
          const existing = open.data.find(i => i.title === title);
          if (existing) {
            await github.rest.issues.createComment({ ...context.repo, issue_number: existing.number, body });
          } else {
            await github.rest.issues.create({ ...context.repo, title, body, labels: [label] });
          }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/cleanbench-full.yml around lines 44 - 65, Extract the
duplicated alert logic from the failure steps in the workflows into a reusable
composite action, such as the suggested notify-failure action, with required
title and body inputs and a default label input. Update cleanbench-full.yml,
ci.yml, and perf-regression.yml to invoke the composite action while preserving
each workflow’s existing titles, bodies, labels, and failure conditions; keep
the list-by-label, find-by-title, comment-or-create behavior centralized in the
action.
src/freshdata/execution/_plan.py (1)

117-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

backend param not propagated by DuckDB/Spark callers.

PlanGenerator now takes an optional backend, but only PolarsEngine.execute() passes backend=self.name. DuckDBEngine.execute() (_duckdb.py:97) and SparkEngine.execute() (_spark.py:104) both call PlanGenerator(config).plan(...) without a backend argument.

No current bug — None and "duckdb"/"spark" produce the same (fallback) outcome today since _SUBSET_DEDUP_NATIVE_BACKENDS only contains "polars". But this is a latent trap: if a future backend capability is added to that set for duckdb/spark, these two engines would silently keep falling back instead of picking it up, since they never identify themselves to PlanGenerator. Recommend passing backend=self.name consistently now, mirroring Polars.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/freshdata/execution/_plan.py` around lines 117 - 127, Propagate the
engine identity through the planning calls in DuckDBEngine.execute() and
SparkEngine.execute() by passing backend=self.name to PlanGenerator, matching
PolarsEngine.execute(). Keep the existing configuration and plan-generation flow
unchanged.
src/freshdata/execution/_config.py (2)

64-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Generic escape-hatch hint doesn't fit most triggers.

The hard-coded 'strategy="conservative" with default options is the fully native path' clause is appended to every fallback message regardless of reason. It's misleading for triggers unrelated to strategy:

  • _freshcore.py's "module" step ("freshdata_freshcore is not installed") — switching strategy won't fix a missing extension.
  • duplicate_subset + a non-polars backend (see test_subset_dedupe_still_falls_back_on_duckdb) — the fix is switching to Polars, not strategy="conservative".
  • fix_dtypes defaults to True independent of strategy, so strategy="conservative" alone (without also disabling fix_dtypes) still triggers a fallback, per the NATIVE test fixture requiring both strategy="conservative" and fix_dtypes=False.

Consider conditioning this hint on whether reason is engine/decision-related, or softening the phrasing to avoid asserting something untrue for other trigger categories.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/freshdata/execution/_config.py` around lines 64 - 68, The fallback
message constructed in the execution configuration must not append the strategy
hint for every reason. Update the message logic around the fallback `message`
construction to conditionally include that guidance only for
engine/decision-related reasons, or replace it with wording that remains
accurate for module, duplicate-subset, and fix-dtypes triggers; preserve the
existing backend, step, and reason details.

51-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hard-coded stacklevel=3 doesn't consistently reach the user's call site.

enforce_fallback_policy is invoked from differing call depths across this PR:

  • Directly inside execute() for DuckDB/Polars/Spark (e.g. _duckdb.py line 100).
  • Nested one level deeper, inside _fallback(), for FreshCore (_freshcore.py line 95).
  • Directly inside run_with_engine()'s semantic branch (execution/__init__.py line 173).

A fixed stacklevel=3 therefore attributes the FallbackWarning to a different (and always internal-library) frame depending on which path triggered it — never the actual user call site, and inconsistently so between FreshCore and the other backends. Libraries like pandas solve this with a dynamic "skip frames inside the package" helper (e.g. find_stack_level()) rather than a hard-coded constant.

Also applies to: 69-74

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/freshdata/execution/_config.py` around lines 51 - 63, Update
enforce_fallback_policy to determine the warning stack level dynamically by
skipping internal freshdata frames until reaching the external caller, rather
than using a fixed stacklevel=3. Reuse an existing stack-level helper if
available, or add the minimal package-frame detection needed, and apply the
computed level when emitting FallbackWarning across all invocation paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 112-143: Harden the large-data-status job by adding a job-level
permissions block granting only contents: read, and configure its
actions/checkout step with persist-credentials: false. In the “Assert the
large/online lane still collects tests” step, remove the pytest stderr
suppression so collection and import errors remain visible while preserving the
existing zero-test failure check and summary.
- Around line 87-111: Ensure the nightly failure alert workflow provisions the
nightly-failure repository label before the “Open/refresh alert issue on
failure” github-script step uses it. Add an idempotent setup step that creates
the label only when missing, preserving the existing issue creation and lookup
behavior.

In `@benchmarks/bench_entity_resolution.py`:
- Around line 212-250: Align the no-positive-predictions precision fallback
across the sibling evaluators: update evaluate_labelled to use the same fallback
as evaluate_exact_baseline when tp + fp is zero, while preserving the existing
calculation for nonzero denominators.

In `@docs/backends.md`:
- Around line 111-119: Update the conservative-strategy native-path wording in
the relevant configuration documentation or message to explicitly state that
fix_dtypes=False is required, or remove the claim that default options provide a
fully native path. Keep the wording consistent with the fallback behavior and
native-path documentation.

In `@docs/benchmarks.md`:
- Line 172: Update the benchmark statement in the surrounding results section to
call 12.5M the pre-blocking all-pairs total rather than “candidate pairs,” while
preserving the 99.96% pruning figure and clarifying that candidate pairs refers
to the post-blocking survivors.

In `@docs/plugins.md`:
- Around line 12-23: Update the plugin safety rule following the extension-point
table to distinguish proposal/validator plugins from comparator/exporter
plugins: retain the no-data-mutation constraint for experts, backends, and
validators, while accurately stating that comparators read field values and
exporters consume reports. Ensure the guidance no longer claims all plugins only
propose or validate.

In `@src/freshdata/api.py`:
- Around line 347-367: Update the fallback_policy handling to validate the
effective engine for both newly created and prebuilt engine_config paths. Before
dataclasses.replace in the existing-config branch, reject engine="pandas" when
the config’s fallback_policy applies and the dataframe is not a native engine
source; preserve the existing native-source and alternate-engine behavior.

In `@src/freshdata/enterprise/contracts.py`:
- Around line 1265-1280: Restore handling for warn_on_extra_columns in the
extra-column validation around contract.strict_columns and
contract.allow_extra_columns. When undeclared columns are allowed but warnings
are enabled, add the existing warning finding path for each extra column;
preserve error findings for strict or disallowed columns and the separate
on_unexpected="warn" behavior.

In `@src/freshdata/enterprise/entity_resolution.py`:
- Around line 582-587: Update the comparator handling around
get_active_comparator in ComparisonLevel.__post_init__ or its comparison path to
explicitly handle an inactive plugin: raise a clear configuration error or emit
an appropriate warning instead of returning None silently. Remove the misleading
pragma comment, and preserve normal plugin invocation when an active comparator
is available.
- Around line 204-315: Fix boundary-sensitive lookahead checks in metaphone() by
requiring nxt to be non-empty before testing membership in vowel or character
sets, especially for C, G, H, W, and Y branches. Preserve valid digraph and
vowel-context behavior while ensuring leading H and word-final letters are
processed correctly; add regressions covering Harry, King, Kelly, and Mac.

In `@src/freshdata/execution/backends/_freshcore.py`:
- Around line 53-72: Rework the FreshCore execution flow around
_unsupported_reason, materialize_to_pandas, and _fallback so config-only
unsupported conditions are evaluated before materializing the source and
fallback_policy="error" can raise without reading file paths. Defer only checks
requiring frame data until after one materialization, and pass that existing
frame into fallback handling so the default allow path does not reread the
original source; preserve native execution when no reason applies.

In `@src/freshdata/pipeline.py`:
- Around line 34-45: Extend the step metadata used by `_STEP_PARAMS` to
distinguish required parameters, marking `strategy` as required for `impute`
while preserving optional parameters such as `columns`. Update
`Pipeline.__post_init__` to raise the class’s established actionable
`ValueError` when required parameters are missing, so direct construction and
`from_json` reject malformed steps before `compile()` accesses
`params["strategy"]`.

In `@src/freshdata/plugins.py`:
- Around line 524-527: Update known_comparator_names() and its validation path
so only active comparator plugins are accepted, excluding
registered-but-inactive names from the returned set or otherwise raising a
configuration error before comparison. Preserve active comparator behavior, and
add coverage for configuring an inactive comparator to ensure it cannot silently
produce a None result.
- Around line 449-463: Update the comparator plugin __call__ method to reject
NaN values after converting raw to float, routing them through the existing
warning-and-return-None invalid-output path. Preserve the current clamping
behavior for finite values and exception handling for plugin failures, and
extend test_comparator_output_clamped_and_exceptions_skip or its relevant test
coverage to verify NaN is skipped.

---

Outside diff comments:
In `@src/freshdata/enterprise/config.py`:
- Around line 419-439: Update ComparisonLevel.__post_init__ to validate
non-built-in kinds against active comparator registrations using the same
active-comparator lookup used by scoring, rather than known_comparator_names().
Preserve the existing ValueError message and built-in kind handling while
rejecting inactive or unavailable comparator plugins during configuration
validation.

In `@src/freshdata/execution/backends/_polars.py`:
- Around line 406-426: Move the duplicate_subset schema validation in
_stage_drop_duplicates before the n_before < 1 early return. Ensure missing
subset columns always raise the existing ValueError, including for empty frames,
while retaining the current short-circuit after validation.

---

Nitpick comments:
In @.github/workflows/cleanbench-full.yml:
- Around line 44-65: Extract the duplicated alert logic from the failure steps
in the workflows into a reusable composite action, such as the suggested
notify-failure action, with required title and body inputs and a default label
input. Update cleanbench-full.yml, ci.yml, and perf-regression.yml to invoke the
composite action while preserving each workflow’s existing titles, bodies,
labels, and failure conditions; keep the list-by-label, find-by-title,
comment-or-create behavior centralized in the action.

In `@docs/fallback-matrix.md`:
- Line 59: Update the “What the report tells you afterwards” heading to use the
American English term “afterward,” leaving the rest of the documentation
unchanged.

In `@examples/plugins/custom_exporter/markdown_exporter.py`:
- Around line 28-34: Declare an explicit low-risk max_risk value on the
MarkdownExporter class, alongside name, uses_network, and requires, so
registered_plugins() metadata correctly identifies this local renderer as benign
and the example provides the intended template for plugin authors.

In `@src/freshdata/execution/_config.py`:
- Around line 64-68: The fallback message constructed in the execution
configuration must not append the strategy hint for every reason. Update the
message logic around the fallback `message` construction to conditionally
include that guidance only for engine/decision-related reasons, or replace it
with wording that remains accurate for module, duplicate-subset, and fix-dtypes
triggers; preserve the existing backend, step, and reason details.
- Around line 51-63: Update enforce_fallback_policy to determine the warning
stack level dynamically by skipping internal freshdata frames until reaching the
external caller, rather than using a fixed stacklevel=3. Reuse an existing
stack-level helper if available, or add the minimal package-frame detection
needed, and apply the computed level when emitting FallbackWarning across all
invocation paths.

In `@src/freshdata/execution/_plan.py`:
- Around line 117-127: Propagate the engine identity through the planning calls
in DuckDBEngine.execute() and SparkEngine.execute() by passing backend=self.name
to PlanGenerator, matching PolarsEngine.execute(). Keep the existing
configuration and plan-generation flow unchanged.

In `@src/freshdata/plugins.py`:
- Around line 517-556: The comparator and exporter name accessors use
inconsistent naming without documenting their differing active/inactive
semantics. Add brief docstrings to known_comparator_names and
active_exporter_names that explicitly state whether inactive registrations are
included and cross-reference the other accessor’s behavior, without changing
their return values.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d92ff45c-25f3-4c8e-b77d-aca20f1a2878

📥 Commits

Reviewing files that changed from the base of the PR and between 61453b2 and 37c05df.

⛔ Files ignored due to path filters (2)
  • benchmarks/data/er_labelled_5k.csv is excluded by !**/*.csv
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (49)
  • .github/workflows/ci.yml
  • .github/workflows/cleanbench-full.yml
  • .github/workflows/perf-regression.yml
  • benchmarks/bench_entity_resolution.py
  • benchmarks/er_results.json
  • benchmarks/gen_er_labelled.py
  • docs/api-reference.md
  • docs/backends.md
  • docs/benchmarks.md
  • docs/comparison.md
  • docs/fallback-matrix.md
  • docs/limitations.md
  • docs/plugins.md
  • docs/trust-claims.md
  • docs/validation.md
  • examples/plugins/custom_comparator/initials_comparator.py
  • examples/plugins/custom_exporter/markdown_exporter.py
  • examples/validation_suite_quickstart.py
  • mkdocs.yml
  • pyproject.toml
  • src/freshdata/__init__.py
  • src/freshdata/api.py
  • src/freshdata/benchmarks/RESULTS.md
  • src/freshdata/benchmarks/_harness.py
  • src/freshdata/benchmarks/run_benchmarks.py
  • src/freshdata/enterprise/__init__.py
  • src/freshdata/enterprise/cli.py
  • src/freshdata/enterprise/config.py
  • src/freshdata/enterprise/contracts.py
  • src/freshdata/enterprise/entity_resolution.py
  • src/freshdata/execution/__init__.py
  • src/freshdata/execution/_config.py
  • src/freshdata/execution/_plan.py
  • src/freshdata/execution/backends/_duckdb.py
  • src/freshdata/execution/backends/_freshcore.py
  • src/freshdata/execution/backends/_polars.py
  • src/freshdata/execution/backends/_spark.py
  • src/freshdata/pipeline.py
  • src/freshdata/plan.py
  • src/freshdata/plugins.py
  • src/freshdata/report.py
  • src/freshdata/testing.py
  • src/freshdata/validation_suite.py
  • tests/test_er_comparators.py
  • tests/test_execution/test_fallback_policy.py
  • tests/test_large_smoke.py
  • tests/test_pipeline_builder.py
  • tests/test_plugin_comparators_exporters.py
  • tests/test_validation_suite.py

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment thread benchmarks/bench_entity_resolution.py
Comment thread docs/backends.md
Comment thread docs/benchmarks.md
Comment thread src/freshdata/enterprise/entity_resolution.py
Comment thread src/freshdata/execution/backends/_freshcore.py
Comment thread src/freshdata/pipeline.py
Comment thread src/freshdata/plugins.py
Comment thread src/freshdata/plugins.py
@JohnnyWilson16
JohnnyWilson16 merged commit 1a15af4 into main Jul 12, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant