Competitive-gap closure: validation suites, fallback policy, ER accuracy evidence, 25M benchmarks, CI honesty#134
Conversation
… 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 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. |
📝 WalkthroughWalkthroughThis 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. ChangesExecution and API workflows
Validation suites and contracts
Entity resolution and plugins
Benchmarks, CI, and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
FreshData benchmark report —
|
| 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)
There was a problem hiding this comment.
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 winReject inactive comparator names in
ComparisonLevelknown_comparator_names()includes inactive registrations, but scoring only usesget_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 winSubset-column validation is skipped for empty frames.
n_before < 1triggers an earlyreturn lf(line 415-416) before theduplicate_subsetexistence check (lines 417-426) runs. A nonexistentduplicate_subsetcolumn therefore silently passes validation whenever the frame reaching this stage happens to have 0 rows, but correctly raisesValueErrorfor 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 valuePrefer "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 winMissing explicit
max_risk— defaults to"high".Unlike the sibling
InitialsComparatorexample (max_risk = "low"), this exporter doesn't declaremax_risk, so_check_metadata'sgetattr(plugin, "max_risk", "high")fallback marks it "high" risk inregistered_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 valueNaming asymmetry between comparator and exporter accessors.
known_comparator_names()returns all registered names (active or not) whileactive_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 winDuplicated "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 takingtitle/body/labelinputs 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
backendparam not propagated by DuckDB/Spark callers.
PlanGeneratornow takes an optionalbackend, but onlyPolarsEngine.execute()passesbackend=self.name.DuckDBEngine.execute()(_duckdb.py:97) andSparkEngine.execute()(_spark.py:104) both callPlanGenerator(config).plan(...)without abackendargument.No current bug —
Noneand"duckdb"/"spark"produce the same (fallback) outcome today since_SUBSET_DEDUP_NATIVE_BACKENDSonly 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 toPlanGenerator. Recommend passingbackend=self.nameconsistently 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 winGeneric 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 tostrategy:
_freshcore.py's"module"step ("freshdata_freshcore is not installed") — switching strategy won't fix a missing extension.duplicate_subset+ a non-polars backend (seetest_subset_dedupe_still_falls_back_on_duckdb) — the fix is switching to Polars, notstrategy="conservative".fix_dtypesdefaults toTrueindependent ofstrategy, sostrategy="conservative"alone (without also disablingfix_dtypes) still triggers a fallback, per theNATIVEtest fixture requiring bothstrategy="conservative"andfix_dtypes=False.Consider conditioning this hint on whether
reasonis 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 winHard-coded
stacklevel=3doesn't consistently reach the user's call site.
enforce_fallback_policyis invoked from differing call depths across this PR:
- Directly inside
execute()for DuckDB/Polars/Spark (e.g._duckdb.pyline 100).- Nested one level deeper, inside
_fallback(), for FreshCore (_freshcore.pyline 95).- Directly inside
run_with_engine()'s semantic branch (execution/__init__.pyline 173).A fixed
stacklevel=3therefore attributes theFallbackWarningto 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
⛔ Files ignored due to path filters (2)
benchmarks/data/er_labelled_5k.csvis excluded by!**/*.csvuv.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.github/workflows/ci.yml.github/workflows/cleanbench-full.yml.github/workflows/perf-regression.ymlbenchmarks/bench_entity_resolution.pybenchmarks/er_results.jsonbenchmarks/gen_er_labelled.pydocs/api-reference.mddocs/backends.mddocs/benchmarks.mddocs/comparison.mddocs/fallback-matrix.mddocs/limitations.mddocs/plugins.mddocs/trust-claims.mddocs/validation.mdexamples/plugins/custom_comparator/initials_comparator.pyexamples/plugins/custom_exporter/markdown_exporter.pyexamples/validation_suite_quickstart.pymkdocs.ymlpyproject.tomlsrc/freshdata/__init__.pysrc/freshdata/api.pysrc/freshdata/benchmarks/RESULTS.mdsrc/freshdata/benchmarks/_harness.pysrc/freshdata/benchmarks/run_benchmarks.pysrc/freshdata/enterprise/__init__.pysrc/freshdata/enterprise/cli.pysrc/freshdata/enterprise/config.pysrc/freshdata/enterprise/contracts.pysrc/freshdata/enterprise/entity_resolution.pysrc/freshdata/execution/__init__.pysrc/freshdata/execution/_config.pysrc/freshdata/execution/_plan.pysrc/freshdata/execution/backends/_duckdb.pysrc/freshdata/execution/backends/_freshcore.pysrc/freshdata/execution/backends/_polars.pysrc/freshdata/execution/backends/_spark.pysrc/freshdata/pipeline.pysrc/freshdata/plan.pysrc/freshdata/plugins.pysrc/freshdata/report.pysrc/freshdata/testing.pysrc/freshdata/validation_suite.pytests/test_er_comparators.pytests/test_execution/test_fallback_policy.pytests/test_large_smoke.pytests/test_pipeline_builder.pytests/test_plugin_comparators_exporters.pytests/test_validation_suite.py
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)
strict_columnswas declared but never read; datetime ranges silently no-op'd (pd.to_numericcoerced datetimes to NaN and the check passed).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.if: schedule+continue-on-error: true— it could not fail even when it ran).What landed (one commit per phase)
fd.ValidationSuite/fd.ColumnRule/fd.CrossColumnRule/raise_if_failed()as a facade compiling onto the existingDataContractengine (one engine, two front doors;from_contract()migrates). New primitives on that engine: GX-stylemostly=tolerance, string lengths, datetime bounds (+ regression fix for the silent no-op),dtype_exact, datasetmin_rows/max_rows/compound_unique, livestrict_columns,enforce_contract(). CLIfreshdata validatewith exit codes 0/1/2. Validation stays read-only; non-pandas inputs record their materialization.fallback_policy="allow"|"warn"|"error"onEngineConfig/fd.clean/CLI;"error"raises before any pandas materialization at every backend delegation point.CleanReportgainsrequested_backend,peak_memory,rows_materialized.freshcoreadded to CLI engine choices.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.token_set+metaphonecomparators;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.jsonwith P/R/F1, FP/FN, false-merge counts, blocking reduction per config, plus the pandas exact-dedupe baseline (recall 0.054).fd.plan()(per-column choices plus the backend/fallback verdict before execution) +fd.applyalias;fd.pipeline()immutable fluent builder compiling 1:1 toCleanConfig(same fallback matrix, reports, audit trail; JSON round-trip).freshdata.comparators+freshdata.exportersentry-point groups with the existing safety discipline (reserved names, output clamping, exception isolation),fd.export(report, format=...), contract helpers, worked examples.src/freshdata/benchmarks/RESULTS.md).nightly-online-largecan now actually fail (removedcontinue-on-error) and failures open a pinnednightly-failureissue (same for cleanbench-full & perf-regression); newlarge-data-statusjob on every PR asserts the lane still collects tests and states in the step summary that it runs nightly; new ~1M-rowlarge_smoketests run in the PR lane.docs/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 fromPlanGenerator.fallback_reason()), ER accuracy table indocs/benchmarks.md, trust-claims additions.What this PR deliberately does NOT do
strategy="balanced"(would fork the decision engine per backend) — documented loudly instead, andfallback_policymakes the fallback refusable.optimize_memorynative port (pandas-specific downcasting by design).Verification
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.ColumnContract/DataContractfields appended with defaults (old JSON loads unchanged);ComparisonLevel.kindrelaxed fromLiteralto validatedstr(runtime behavior unchanged for built-ins); no public signature removed or changed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes