Repository navigation
fix(docreader): preserve numeric rows in PDF text extraction - #3228
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved parser issues affect glyph filtering, table/artifact detection, numeric preservation, and end-to-end regression coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR improves built-in PDF extraction of numeric table values, row relationships, and hidden-text handling.
Changes:
- Preserves kerning-separated digits and aligned numeric columns.
- Makes page and chart cleanup more context-aware.
- Adds regression coverage for numeric tables and filtering behavior.
File summaries
| File | Reviewed changes |
|---|---|
docreader/tests/test_pdf_router.py |
Adds regression tests for numeric rows, tables, and filtering. |
docreader/parser/pdf_parser.py |
Updates numeric-aware extraction, filtering, fallback, and cleanup logic. |
Review details
Suppressed comments (5)
docreader/parser/pdf_parser.py:853
- This early return bypasses
_is_artifact_columnfor every column. A page that has a valid aligned numeric table plus an arXiv/sidebar watermark will therefore keep the watermark glyphs, because the table hint is enough to skip the existing margin filter. Preserve the matched label/value columns as one stream, but continue filtering unrelated artifact columns.
if _looks_like_numeric_table_columns(chars, scale, width, cols):
# Keep aligned table cells in one visual stream so rows such as
# ``Aster 124`` are not emitted as a label block followed by a value
# block. ``chars`` has already passed hidden/off-page filtering.
return [chars]
docreader/parser/pdf_parser.py:1820
- The reported regression is only exercised through helper tests; no test feeds the 3,303-byte repro through
PDFParser.parse_into_textand verifies all four values and their row relationship. The route combines table detection, plain/layout arbitration, and post-processing, so these unit tests can pass while the end-to-end loss remains. Add an integration regression fixture or inline PDF test for the parser route.
layout, filtered_text, table_hint = _extract_layout_text(
page, pdfium_r, return_metadata=True
)
if filtered_text:
# The plain layer may contain hidden or
# off-page prompt-injection text. Once the
# layout path proves that filtering occurred,
# never re-introduce that layer as a fallback.
text = layout
elif layout and not _should_prefer_plain(
plain, layout, allow_structured_rows=table_hint
):
docreader/parser/pdf_parser.py:1215
- When the layout preserves the same number of row-shaped records but substitutes or loses one value,
missing_valuesis positive while the row counts are equal (for example, plain rowsAster 124/Willow 237versus layout rowsAster 124/Willow 999). This condition then selects the layout and drops the missing value. The value-set mismatch is already the intended signal, so do not gate it on the row count.
if missing_values and len(layout_rows) < len(plain_rows):
docreader/parser/pdf_parser.py:946
aligned >= 1is too weak for a branch that returns[chars]and disables normal column/artifact filtering. A narrow side column headedNoteswith numeric lines can share one baseline with the left column; one coincidental match then collapses an ordinary two-column page and interleaves its reading order. Require multiple or majority value rows to align before treating the page as a numeric table.
aligned = sum(
any(abs(y - other_y) <= tolerance for other_y in label_ys)
for y in value_ys
)
if aligned >= 1:
return True
docreader/parser/pdf_parser.py:274
- Chart context alone does not distinguish axis labels from a nearby numeric table. For example,
Quantity\n124\n237\n68\n91\ntraining erroris grouped as chart debris and all four values are removed, despite the table header. Before deleting a numeric run, also exclude a run with a table-header/row signal so ambiguous document data is retained.
has_chart_context = any(
_CHART_CONTEXT_RE.search(lines[k])
for k in range(context_start, context_end)
)
if j - i >= 3 and has_chart_context:
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except Exception: | ||
| source_count += 1 | ||
| continue |
| plain_well_formed = _plain_is_well_formed(plain) | ||
| if plain_well_formed and not _page_has_invisible_text( | ||
| page, pdfium_r | ||
| ): | ||
| text = plain |
| def _tail_looks_like_numeric_table(lines: list) -> bool: | ||
| """Detect labelled or alternating label/value rows at the text tail.""" | ||
| tail = lines[-10:] | ||
| if len(_structured_numeric_rows("\n".join(tail))) >= 2: |
| def _tail_looks_like_numeric_table(lines: list) -> bool: | ||
| """Detect labelled or alternating label/value rows at the text tail.""" | ||
| tail = lines[-10:] | ||
| if len(_structured_numeric_rows("\n".join(tail))) >= 2: |
…ra-pdf-3223 # Conflicts: # docreader/parser/pdf_parser.py
|
@copilot review |
|
Follow-up for the prior Copilot findings: the pushed commits now keep matched numeric label/value columns while continuing artifact-column filtering, require majority baseline alignment, preserve plain rows on any numeric value-set mismatch, and retain table-shaped numeric runs even in chart context. The focused parser regression suite covers these branches; please refresh the review on head 1d0d4c3 for the current diff. |
|
Merged current main while retaining its CropBox/rotation coordinates, numeric-evidence checks and source locators. The plain fast path's glyph preflight was already present on the prior head; I added an actual cropped-PDF regression to verify it. With the old invisible-only probe it fails because Rechecking the outdated table-tail finding showed it was still valid. The helper now requires numeric rows to reach the non-empty tail; labelled rows, a numeric column and alternating label/value rows all retain their data while the intervening The focused PDF/image/source-location suites ran 87 tests: 84 passed, 3 skipped for missing external samples, using actual PDFium without parser-import stubs. Current main already preserves the four labelled rows in the original issue PDF; it still drops the separate standalone |
|
Current-head CI follow-up for The completed local PDF regression results remain in the test plan, but full official integration is still unverified. Could a maintainer rerun the cancelled DocReader job? I have no upstream Actions write permission and have not changed the workflow timeout or test assertions. Correction (2026-10-15): the original wording missed the Python suite summary immediately before cancellation. This clarification uses the saved log; it is not a new test run. |
Description
Preserve numeric body rows and item/value relationships in the built-in PDF parser. Numeric-only table values should survive cleanup unless page-number or chart context supports removing them. Keep aligned numeric columns together while retaining normal reading order for prose and artifact-column filtering.
The merged main branch now preserves the original issue's four labelled rows through its numeric-evidence fallback. This PR still addresses the issue's separate
Quantity\n124\n237\n68\n91cleanup failure and table/layout edge cases. It retains main's CropBox/rotation handling, numeric multiset checks and source-location mapping.Filtering metadata distinguishes intentionally excluded glyphs from unreadable glyph boxes. The plain-text fast path checks excluded glyphs before bypassing layout, and figure-caption cleanup requires the table signal to reach the current non-empty tail, so an intervening diagram label is still removed.
Type of Change
Related Issue
Fixes #3223
Testing
python -m unittest docreader.tests.test_pdf_router docreader.tests.test_source_locator_pdf docreader.tests.test_pdf_embedded_images docreader.tests.test_source_locator -v: 87 tests, 84 passed and 3 skipped for unavailable PDF samples. Uses actual PDFium and the existing DocReader environment, with no parser-import stubs.identitydiagram label.git diff --checkpasses.Checklist
Prepared with AI assistance. Screenshots are not applicable to this parser change.