Skip to content

perf: add linked format checks to skip redundant field tests - #265

Open
bolinocroustibat wants to merge 10 commits into
mainfrom
feat/linked-checks
Open

perf: add linked format checks to skip redundant field tests#265
bolinocroustibat wants to merge 10 commits into
mainfrom
feat/linked-checks

Conversation

@bolinocroustibat

@bolinocroustibat bolinocroustibat commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Supersedes #155, re-implemented on the current Format / FormatsManager architecture.

Description

Instead of testing all formats for all columns, we introduce a parent-child relationship between formats, for instance: float > latitude_wgs > latitude_wgs_fr_metropole, with a parent attribute on specialized format modules.

For each column:

  • perform the tests that are not parent of another format (leaf formats)
  • for each of these tests, walk up to the parent and:
    • if the child's score is 0, perform the parent test
    • if the child's score is > 0, set the parent score to the child's score (label/header testing can still override later, so we can't leave the parent at 0)
    • repeat for all parents in the chain

This way, if a column is latitude_wgs_fr_metropole, we only run one full-column float check instead of three.

This is not perfect: copying the child's score to the parent can be misleading (many floats are invalid latitudes), but it saves significant processing time. A safer alternative would be to test parents first and only run children when the parent succeeds.

Benchmark comparison

Run count

Branch Sessions Runs per session Total runs
feat/linked-checks 4 3 12
main 4 3 12

All benchmark sessions

Date (UTC) Branch Commit Scenario Run 1 Run 2 Run 3 Median
2026-07-29 13:13:05 feat/linked-checks 33891c4 big file 46.08 43.57 43.25 43.57
2026-07-29 13:13:05 feat/linked-checks 33891c4 big file + profile 43.47 45.17 43.22 43.47
2026-07-27 10:21:30 feat/linked-checks 6d9c101 big file 40.46 42.27 42.92 42.27
2026-07-27 10:21:30 feat/linked-checks 6d9c101 big file + profile 45.17 38.11 37.19 38.11
2026-07-27 09:59:16 feat/linked-checks 6d9c101 big file 43.62 34.83 34.95 34.95
2026-07-27 09:59:16 feat/linked-checks 6d9c101 big file + profile 36.72 36.19 36.40 36.40
2026-07-27 10:07:57 feat/linked-checks 6d9c101 big file 41.41 39.13 40.27 40.27
2026-07-27 10:07:57 feat/linked-checks 6d9c101 big file + profile 40.97 42.76 38.39 40.97
2026-07-27 10:10:47 main e21b750 big file 43.10 42.78 38.51 42.78
2026-07-27 10:10:47 main e21b750 big file + profile 42.46 39.34 38.76 39.34
2026-07-27 10:34:35 main e21b750 big file 49.36 43.60 43.07 43.60
2026-07-27 10:34:35 main e21b750 big file + profile 49.24 46.10 47.58 47.58
2026-07-29 13:12:59 main e21b750 big file 48.31 43.42 43.41 43.42
2026-07-29 13:12:59 main e21b750 big file + profile 48.19 44.65 41.20 44.65
2026-07-17 15:47:58 main 4626c25 big file 42.04 40.29 39.59 40.29
2026-07-17 15:47:58 main 4626c25 big file + profile 39.60 40.77 40.97 40.77

All times in seconds.

Summary

Branch Scenario Total runs Median Mean Min Max
feat/linked-checks big file 12 41.8 s 41.2 s 34.8 s 46.1 s
feat/linked-checks big file + profile 12 39.7 s 40.3 s 36.2 s 45.2 s
main big file 12 42.9 s 43.1 s 38.5 s 49.4 s
main big file + profile 12 41.8 s 43.2 s 38.8 s 49.2 s

feat/linked-checks vs main

Scenario main feat/linked-checks Delta
big file 42.9 s 41.8 s −1.1 s (−2.5%)
big file + profile 41.8 s 39.7 s −2.1 s (−5%)

@bolinocroustibat
bolinocroustibat requested a review from Pierlou July 27, 2026 10:04

@Pierlou Pierlou 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.

Happy to see this improvement in the pipes, thanks for the work 💪
Just a couple suggestions/open questions to discuss, but the logic looks good to me, nice to see the benchmark numbers 📉

Comment thread csv_detective/parsing/columns.py
Comment thread csv_detective/parsing/columns.py Outdated
Comment thread csv_detective/parsing/columns.py
Comment thread tests/test_linked_checks.py
@bolinocroustibat

bolinocroustibat commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@Pierlou I'm wondering if I should also create a PR with the other logic you suggested, where we test parents first and only run children when the parent succeeds, and then run the benchmarks on this second PR as well? It's quite fast to do so with LLMs.
I guess it would be less performing, but more safe, than this PR's logic?

I'm not sure if our big synthetic big file for performance benchmarking will be adapted to fully test those cases though.

@Pierlou

Pierlou commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@Pierlou I'm wondering if I should do create PR with the other logic you suggested, where we test parents first and only run children when the parent succeeds, and then run the benchmarks on this second PR as well? It's quite fast to do so with LLMs. I guess it would be less performing, but more safe, than this PR's logic?

I'm not sure if our big synthetic big file for performance benchmarking will be adapted to fully test those cases though.

We could do that but I believe the "children first then propagate to parents" is better, because we have early stops in test_col_val so that we don't test the whole column if the first values are not valid. For instance:

  • if a column contains valid latitude_wgs:

    • children first:
      1. test latitude_wgs_fr_metropole => invalid (can be assessed very early as we check the first values before the whole column)
      2. test latitude_wgs (whole column) => valid
      3. propagate to float
    • parents first:
      1. test float (whole column) => valid
      2. test latitude_wgs (whole column) => valid
      3. test latitude_wgs_fr_metropole => invalid (can be assessed very early as we check the first values before the whole column)
  • if a column contains whatever invalid values:

    • children first:
      1. test latitude_wgs_fr_metropole => invalid (can be assessed very early as we check the first values before the whole column)
      2. test latitude_wgs (whole column) => invalid (can be assessed very early as we check the first values before the whole column)
      3. test float => invalid (can be assessed very early as we check the first values before the whole column)
    • parents first:
      1. test float => invalid (can be assessed very early as we check the first values before the whole column)
      2. test latitude_wgs => invalid (can be assessed very early as we check the first values before the whole column)
      3. test latitude_wgs_fr_metropole => invalid (can be assessed very early as we check the first values before the whole column)

For invalid values, early stops allow not to spend time checking the whole column in both cases.
For valid values, starting with the children truely bypasses parent tests, while additional tests still have to be done for children if we start with parents.
Does that make sense?

@bolinocroustibat
bolinocroustibat requested a review from Pierlou July 29, 2026 12:58

@Pierlou Pierlou 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.

Very nice, long-awaited improvement 👏

Comment thread csv_detective/parsing/columns.py Outdated
@bolinocroustibat
bolinocroustibat requested a review from Pierlou July 30, 2026 09:12
@bolinocroustibat

Copy link
Copy Markdown
Contributor Author

@Pierlou FYI, also added this add "float" as parent of "money" and "percent" to the PR before merging

@bolinocroustibat

Copy link
Copy Markdown
Contributor Author

I guess it would be less performing, but more safe, than this PR's logic?

I'm not sure if our big synthetic big file for performance benchmarking will be adapted to fully test those cases though.

Reverted after discussion

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.

2 participants