Skip to content

docs: correct three claims that do not match the code - #499

Merged
kevincostner17 merged 1 commit into
mainfrom
docs/phase32-truth-up
Sep 20, 2026
Merged

kevincostner17 merged 1 commit into
mainfrom
docs/phase32-truth-up

Conversation

@kevincostner17

Copy link
Copy Markdown
Contributor

What

Phase 32 documentation truth-up. Three documented claims were checked against the code; all three were wrong, though one was less wrong than I had previously recorded.

1. StreamingCleanConfig.window_size — the docstring is factually wrong

src/freshdata/streaming/_config.py:21-24 said:

Size of the recent-window used for rolling statistics and the rolling trust score.

Both halves are false:

  • The rolling trust score is sized by a different field, rolling_trust_window (default 20) — which docs/streaming.md:100 confirms: "mean of per-batch trust scores over the last rolling_trust_window batches".
  • window_size has exactly one functional use in the entire src/ tree_cleaner.py:391, while len(seen) > self.scfg.window_size, bounding the cross-batch dedup hash window. Established by exhaustive grep, excluding docstrings, comments, the dataclass default and the validation loop.

A user reading help(StreamingCleanConfig) would raise window_size expecting more history to feed trust scoring, and nothing would change. The prose docs were already correct; the API docstring was the wrong one.

2. Plan drift refusal is head-sampled — now stated, and pinned by a test

docs/repair-plans.md described the fingerprint as "row count, column names+dtypes, content sample". That is accurate but incomplete in a way that matters: the sample is the first 512 rows, so a change after row 512 that preserves row count, column names and dtypes is not detected.

Demonstrated on a 1000-row frame — the same edit, two positions:

Edit location Result
row 10 (inside head sample) PlanDriftError raised
row 900 (beyond row 512) no error — undetected

This is a deliberate design choice: the fingerprint is documented as "cheap", and it guards against applying a plan to the wrong data, not against the data having changed. But the head restriction belongs where the guarantee is described.

tests/test_plan_drift_sampling.py (5 tests) pins the boundary exactly at _SIGNATURE_SAMPLE_ROWS: an edit at index 511 changes sample_hash, an edit at index 512 does not, and the row-count and column-name components still fire beyond the window. The documented claim is now backed by execution, and a future change to the sampling strategy has to come past a failing test.

Correction to an earlier internal note: I had recorded that neither the docs nor the docstring disclosed the sampling. That was wrong — the docs did say "content sample" and the code is honest internally (sample_hash, "a head sample"). What was missing was only that the sample is the head, and its size.

3. The README called the default Polars path "native"

docs/fallback-matrix.md opens by calling this "the single most important row":

with default options, every native engine delegates the whole pipeline to pandas […] The fully native path is strategy="conservative" with fix_dtypes=False.

The README's "Native Polars DataFrames" section showed fd.clean(pl_df) — the defaults, i.e. the configuration that delegates entirely to pandas — described as getting a Polars result "with zero pandas boilerplate". True of the type, not of the execution. That sentence now appears beside the example.

Scope

Documentation and one new test. No behaviour change, so no compatibility impact. CHANGELOG entry added under [Unreleased] / Documentation.

Verification

  • Full suite, py3.12 — 7344 passed, 22 skipped, 0 failed, coverage 95.02%
  • ruff check . clean (the repo's CI lint)
  • Streaming + repair-plan targeted subset — 224 passed, 2 skipped
  • tests/test_plan_drift_sampling.py — 5 passed

Open question, deliberately not answered here

The README claims "Polars backend delivers 2–3× throughput vs pandas at 10M rows" and "DuckDB backend consumes 200 MB peak RAM at 1M rows vs 1,046 MB for pandas". The harnesses differ in configuration — benchmarks/harness_metrics.py:240 and bench_report.py use strategy="balanced" (the default, which delegates to pandas), while only bench_native_semantic.py uses the genuinely native strategy="conservative", fix_dtypes=False. I could not determine which harness produced the README figures, so I make no claim about them and have changed nothing there. Flagging it as worth confirming separately.

Phase 32 truth-up. Each was verified against the code before being changed,
and one of the three turned out to be less wrong than previously recorded.

StreamingCleanConfig.window_size was documented as sizing "rolling statistics
and the rolling trust score". It does neither. Its only functional use in the
whole src tree is bounding the cross-batch duplicate window, and the rolling
trust score is sized by a different field, rolling_trust_window. A user
reading help(StreamingCleanConfig) would raise window_size expecting more
history to feed trust scoring, and nothing would change. The prose docs in
docs/streaming.md were already correct; the API docstring was the wrong one.

docs/repair-plans.md described the drift fingerprint as "row count, column
names+dtypes, content sample". That is accurate but incomplete in a way that
matters: the sample is the first 512 rows, so a change after row 512 that
preserves the row count, names and dtypes is not detected. Demonstrated: on a
1000-row frame the same edit raises PlanDriftError at row 10 and passes
silently at row 900. The fingerprint is deliberately cheap and this is a
design choice, not a bug -- but the head restriction belongs where the
guarantee is described. A new test pins the boundary exactly at row 512 so
the documented claim is backed by execution, and so a future change to the
sampling strategy has to come past a failing test.

The README's "Native Polars DataFrames" section showed fd.clean(pl_df) with
defaults and called it native. docs/fallback-matrix.md opens by calling this
"the single most important row": with default options every native engine
delegates the whole pipeline to pandas. You get a Polars frame back, but not
native Polars execution. That sentence now appears beside the example.

Documentation and one new test only; no behaviour change.
@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: abd8410b-e467-457d-a5f3-63cd3d4941b6


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 — performance

  • freshdata: ?
  • python: ?
  • platform: ?
fixture n_rows n_cols p50 s p95 s peak MB repair % false-repair % preserve % trust monotonic export %

Authored-code reduction (Metric 6)

@kevincostner17
kevincostner17 merged commit e1520d7 into main Sep 20, 2026
22 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