fix(extractor): detect two-column layout when the page also has a table - #234
fix(extractor): detect two-column layout when the page also has a table#234mohojojo wants to merge 3 commits into
Conversation
detect_columns gated both the relative-valley and XY-cut fallbacks behind `!page_has_table`. The intent was that a table's column gaps can look like gutters in the projection histogram. The side effect: any page that has a detected table AND genuine two-column prose loses column detection entirely and collapses to single-column, Y-interleaved reading order. This is common in real documents — e.g. a fund factsheet whose left/right text columns sit above a returns table. When the absolute-valley pass finds no empty gutter (justified text fills it), the relative-valley fallback is skipped because a table is present, and the two columns are read line-by-line interleaved. The relative-valley path already has its own table defense: `columns_have_prose` rejects splits whose sides look like tables/forms (many items per line). That guard — not the page-level `page_has_table` flag — is the right protection, so run the relative-valley fallback regardless of tables. The XY-cut fallback has no such prose guard, so keep it gated on `!page_has_table` to avoid reading a table's widest column gap as a page gutter. Adds a regression test: two-column justified prose on a page flagged `page_has_table = true` must still split into two columns (fails before this change, which returned a single column). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Real-document regression for the previous commit: a fund factsheet whose left (fund data) and right (market commentary) text columns sit above a returns table on the same page. Before the fix, the page-level table flag suppressed column detection and the two columns were read Y-interleaved. The snapshot captures the corrected reading order — the left column (ÁLTALÁNOS INFORMÁCIÓK → BEFEKTETÉSI POLITIKA) is contiguous and precedes the right column (PIACI ÖSSZEFOGLALÓ) — and fails if the interleaving regresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Added a real-document regression fixture: |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Wide (3+ column) heuristic tables skip the numeric-content check on the assumption they are legitimate text-only tables (category lists, program descriptions). Justified paragraph text abuses that: when a fine-print disclaimer is set justified, its inter-word gaps align into vertical "columns", so a two-line paragraph is detected as an N-column table — splitting one sentence into cells and re-emitting the text, so consumers get duplicated, reordered output. `is_paragraph_content` already rejects several paragraph shapes (word-break hyphens, letter-spacing, long fragments) but missed this one: few rows, no hyphens, moderate cell length. Add a prose-row signal — a near-full row (fills most columns) whose cells are mostly lowercase mid-sentence fragments, at least one of them multi-word. Two or more such rows is a paragraph. Numeric or capitalized-label table rows never match, so real tables (including the stats table on the very same page) are untouched. Regenerates the marketprog-prestige-two-column snapshot: the disclaimer now flows as a single paragraph instead of a phantom 8-column table with duplicated text (raised in review). Across a 461-document corpus this removes justified- prose phantom tables from 52 documents and destroys no real data table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Good catch — fixed in 0ac0edb rather than blessing the bad output. Root cause: Fix: The snapshot is regenerated: the disclaimer now flows as a single paragraph, no phantom table, no duplication. Added unit tests Verified on a 461-document corpus: removes justified-prose phantom tables from 52 documents and destroys no real data table (spot-checked the biggest deltas — every one was market-commentary or a garbled prose+data merge being correctly de-tabled, with the underlying data preserved as text). |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/snapshots/marketprog-prestige-two-column.md">
<violation number="1" location="tests/snapshots/marketprog-prestige-two-column.md:27">
P2: The updated snapshot still blesses malformed prose with missing spaces, so the regression test will accept unreadable output for this paragraph. The extractor's spacing should be corrected and the snapshot regenerated rather than encoding these concatenations as the expected result.</violation>
</file>
<file name="src/tables/detect_heuristic.rs">
<violation number="1" location="src/tables/detect_heuristic.rs:1385">
P2: The new prose-row gate in validation 8 can reject legitimate wide text tables and flatten them to plain text. `has_table_like_content` (validation 7) deliberately bypasses the numeric-content check for any 3+ column table so that genuine text-only tables (category lists, program descriptions, glossaries) are kept. But this new `prose_rows >= 2` check then rejects any wide table where ≥2 rows are near-full, ≥60% lowercase-starting, and contain at least one 3+-word cell. A real multi-column text table whose rows are fully populated with lowercase multi-word cells therefore passes validation 7 and is then dropped as 'prose' — a structural regression for valid tables. The existing unit tests only cover the 8-column disclaimer (should reject) and a numeric stats table (should keep); neither exercises a legitimate full-width lowercase text table, so this false-positive path is untested. Consider gating the new heuristic on the table lacking genuine data cells (numbers/dates/units) so text rows beside real data are preserved, and/or requiring multi-word cells across a majority of the row rather than a single cell.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
|
||
| ## ----- Benchmark | ||
|
|
||
| A múltbeli hozamoknem jelentenekgaranciát az alap jövőbeli teljesítményére nézve. Jelen hirdetésnem minősül ajánlattételnek vagy befektetési tanácsadásnak. A befektetés részletes feltételeit az Alap Tájékoztatója tartalmazza, mely a mindenkor érvényes kondíciós listákkal együtt megtalálható a forgalmazási helyeken. A befektetési alap forgalmazásával (vétel, tartás, eladás) kapcsolatos költségek az <u>alap kezelési szabályzatában ésa forgalmazási helyeken megismerhetők.</u> |
There was a problem hiding this comment.
P2: The updated snapshot still blesses malformed prose with missing spaces, so the regression test will accept unreadable output for this paragraph. The extractor's spacing should be corrected and the snapshot regenerated rather than encoding these concatenations as the expected result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/snapshots/marketprog-prestige-two-column.md, line 27:
<comment>The updated snapshot still blesses malformed prose with missing spaces, so the regression test will accept unreadable output for this paragraph. The extractor's spacing should be corrected and the snapshot regenerated rather than encoding these concatenations as the expected result.</comment>
<file context>
@@ -24,13 +24,7 @@ Nincs ilyen eszköz a portfólióban **Lejárat szerinti megoszlás:**
-|Tájékoztatója forgalmazási helyeken. A befektetési alap forgalmazásával (vétel, tartás, eladás) kapcsolatos költségek az alap kezelési szabályzatában ésa forgalmazási helyeken megismerhetők.|tartalmazza,|mely a mindenkor|érvényes|kondíciós|listákkal|együtt megtalálható|a|
-
-Tájékoztatója tartalmazza, mely a mindenkor érvényes kondíciós listákkal együtt megtalálható a
+A múltbeli hozamoknem jelentenekgaranciát az alap jövőbeli teljesítményére nézve. Jelen hirdetésnem minősül ajánlattételnek vagy befektetési tanácsadásnak. A befektetés részletes feltételeit az Alap Tájékoztatója tartalmazza, mely a mindenkor érvényes kondíciós listákkal együtt megtalálható a forgalmazási helyeken. A befektetési alap forgalmazásával (vétel, tartás, eladás) kapcsolatos költségek az <u>alap kezelési szabályzatában ésa forgalmazási helyeken megismerhetők.</u>
**KOCKÁZATI MUTATÓK AZ ELMÚLT 12 HÓNAPRA:** Az alap heti hozamokból számolt évesített szórása: 4,77 % A benchmark heti hozamokból számolt évesített szórása: 0,04 % **BEFEKTETÉSI HORIZONT:** A javasolt minimális befektetési idő:
</file context>
| .filter(|c| c.chars().next().is_some_and(char::is_lowercase)) | ||
| .count(); | ||
| let has_multiword = row_filled.iter().any(|c| c.split_whitespace().count() >= 3); | ||
| lowercase_start * 5 >= row_filled.len() * 3 && has_multiword |
There was a problem hiding this comment.
P2: The new prose-row gate in validation 8 can reject legitimate wide text tables and flatten them to plain text. has_table_like_content (validation 7) deliberately bypasses the numeric-content check for any 3+ column table so that genuine text-only tables (category lists, program descriptions, glossaries) are kept. But this new prose_rows >= 2 check then rejects any wide table where ≥2 rows are near-full, ≥60% lowercase-starting, and contain at least one 3+-word cell. A real multi-column text table whose rows are fully populated with lowercase multi-word cells therefore passes validation 7 and is then dropped as 'prose' — a structural regression for valid tables. The existing unit tests only cover the 8-column disclaimer (should reject) and a numeric stats table (should keep); neither exercises a legitimate full-width lowercase text table, so this false-positive path is untested. Consider gating the new heuristic on the table lacking genuine data cells (numbers/dates/units) so text rows beside real data are preserved, and/or requiring multi-word cells across a majority of the row rather than a single cell.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tables/detect_heuristic.rs, line 1385:
<comment>The new prose-row gate in validation 8 can reject legitimate wide text tables and flatten them to plain text. `has_table_like_content` (validation 7) deliberately bypasses the numeric-content check for any 3+ column table so that genuine text-only tables (category lists, program descriptions, glossaries) are kept. But this new `prose_rows >= 2` check then rejects any wide table where ≥2 rows are near-full, ≥60% lowercase-starting, and contain at least one 3+-word cell. A real multi-column text table whose rows are fully populated with lowercase multi-word cells therefore passes validation 7 and is then dropped as 'prose' — a structural regression for valid tables. The existing unit tests only cover the 8-column disclaimer (should reject) and a numeric stats table (should keep); neither exercises a legitimate full-width lowercase text table, so this false-positive path is untested. Consider gating the new heuristic on the table lacking genuine data cells (numbers/dates/units) so text rows beside real data are preserved, and/or requiring multi-word cells across a majority of the row rather than a single cell.</comment>
<file context>
@@ -1360,6 +1359,36 @@ fn is_paragraph_content(cells: &[Vec<String>]) -> bool {
+ .filter(|c| c.chars().next().is_some_and(char::is_lowercase))
+ .count();
+ let has_multiword = row_filled.iter().any(|c| c.split_whitespace().count() >= 3);
+ lowercase_start * 5 >= row_filled.len() * 3 && has_multiword
+ })
+ .count();
</file context>
| lowercase_start * 5 >= row_filled.len() * 3 && has_multiword | |
| // Only treat as prose if the row carries no genuine table data, | |
| // so real tables with numbers/dates/units aren't flattened. | |
| let has_data_cell = row_filled.iter().any(|c| looks_like_table_data(c)); | |
| if has_data_cell { | |
| return false; | |
| } | |
| lowercase_start * 5 >= row_filled.len() * 3 && has_multiword |
Problem
detect_columns(src/extractor/layout.rs) gates both its fallback column-detection strategies — relative-valley and XY-cut — behind!page_has_table:The intent (per the comment) is that a table's inter-column gaps can look like gutters in the projection histogram. But the side effect is that any page which has a detected table and genuine two-column prose loses column detection entirely and collapses to single-column, Y-interleaved reading order.
This is common in real documents. A fund factsheet, for example, has left/right text columns sitting above a returns table. When the absolute-valley pass finds no empty gutter (justified text fills it), the relative-valley fallback is skipped because a table is present, so the two columns get read line-by-line interleaved:
(left-column labels shredded into right-column sentences).
Root cause, confirmed
With
RUST_LOG=debug, the same page yields opposite results depending on thepage_has_tableargument:The relative-valley path finds the correct split at x=294 and both sides pass
columns_have_prose— it's only thepage_has_tablegate that suppresses it.Fix
The relative-valley path already has its own table defense:
columns_have_proserejects splits whose sides look like tables/forms (avg_items > 3.5per line, low prose fill). That guard — not the page-levelpage_has_tableflag — is the right protection, so the relative-valley fallback now runs regardless of tables.The XY-cut fallback has no such prose guard, so it stays gated on
!page_has_tableto avoid reading a table's widest column gap as a page gutter.Verification
relative_valley_detects_columns_even_with_table_on_page: two-column justified prose on a page flaggedpage_has_table = truemust still split into two columns. Fails before this change (returns 1 column), passes after.cargo test(all existing tests unchanged),cargo fmt --all -- --check,cargo clippy -- -D warnings.pdf2mdbefore/after: only 4 outputs changed, all corrections to the interleaving bug; the other 457 are byte-identical (thecolumns_have_proseguard keeps the change scoped to genuine two-column pages).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes two-column detection on pages that also contain tables and stops justified prose from being misidentified as wide text tables. Reading order is corrected and phantom tables are avoided.
detect_columnseven whenpage_has_tableis true; rely oncolumns_have_proseto reject table‑like splits, and keep XY‑cut gated behind!page_has_table.is_paragraph_content: detect near‑full rows of lowercase, multi‑word fragments as prose to avoid treating justified paragraphs as N‑column tables; real numeric/label tables remain unaffected.marketprog-prestige-two-column) that now reads columns correctly and removes the phantom table.Written for commit 0ac0edb. Summary will update on new commits.