Skip to content

fix(md): keep the last cell of table rows without a trailing pipe#3817

Open
chuenchen309 wants to merge 1 commit into
docling-project:mainfrom
chuenchen309:fix/md-table-trailing-pipe
Open

fix(md): keep the last cell of table rows without a trailing pipe#3817
chuenchen309 wants to merge 1 commit into
docling-project:mainfrom
chuenchen309:fix/md-table-trailing-pipe

Conversation

@chuenchen309

Copy link
Copy Markdown

The leading and trailing pipes of a GFM table row are both optional, and the backend's own row detector only requires a leading one (md_backend.py:488):

is_table_row = "|" in snippet_text and (
    self.in_table or original_text.lstrip().startswith("|")
)

But the parser split each row with [1:-1] (:215, :220), which assumes both pipes are present. On a row written without the trailing pipe, that slice drops the last cell:

input output on main
| Character | Name in German
|---|---
| Scrooge McDuck | Dagobert Duck
| Character |
|----------------|
| Scrooge McDuck |

"Name in German" and "Dagobert Duck" are gone, silently, with no warning. Directly against the backend: the trailing-pipe form gives num_cols=2 and four cells; the same table without it gives num_cols=1 and two.

The input is legal GFM — that isn't my reading of the spec: marko, docling's own markdown dependency, renders that same table with two <th> columns.

The sibling asciidoc backend already gets this right — _split_row (asciidoc_backend.py:373) does line.split("|")[1:] and comments that it removes the leading empty string only.

Fix

Split off an empty field only when it comes from a pipe at the very edge of the row, so both spellings parse identically. Rows that do carry the trailing pipe are unaffected, and an intentionally empty cell in the middle of a row still survives.

Reachable from the public DocumentConverter().convert() (document_converter.py:152) and also from vlm_pipeline.py:302, so a VLM emitting a table without trailing pipes loses columns too.

Test Plan

Added test_convert_table_without_trailing_pipes next to the existing test_convert_table_has_no_duplicate_cells, asserting both spellings produce the same two-column table. It fails on main and passes with this change; the existing table test passes on both.

  • tests/test_backend_markdown.py — 9 passed, 1 skipped
  • tests/test_backend_asciidoc.py tests/test_backend_csv.py tests/test_backend_html.py — 44 passed
  • Wider -k "backend or markdown or md" sweep — 334 passed, 2 failed; both failures are test_backend_webp.py::test_e2e_webp_conversions raising ImportError for a missing optional OCR engine, and they reproduce identically on a pristine checkout with no changes applied.
  • ruff check / ruff format --check clean.

Checklist

  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

Disclosure: written with Claude Code. I verified the reproduction, the marko comparison, the sibling backend, and the test red/green myself.

The leading and trailing pipes of a GFM table row are both optional, and
the backend's own row detector only requires a leading one:

    is_table_row = "|" in snippet_text and (
        self.in_table or original_text.lstrip().startswith("|")
    )

But the parser split each row with [1:-1], which assumes both pipes are
there. On a row written without the trailing pipe, that slice drops the
last cell:

    | Character | Name in German      ->  | Character      |
    |---|---                               |----------------|
    | Scrooge McDuck | Dagobert Duck       | Scrooge McDuck |

"Name in German" and "Dagobert Duck" are gone, silently, with no warning.
The input is legal GFM: marko, docling's own markdown dependency, renders
that same table with two <th> columns.

The sibling asciidoc backend already gets this right -- _split_row there
does `line.split("|")[1:]` and comments that it is removing the leading
empty string only.

Split off an empty field only when it comes from a pipe at the very edge
of the row, so both spellings parse the same. Rows that do carry the
trailing pipe are unaffected, and an intentionally empty cell in the
middle of a row still survives.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: chuenchen309 <48723787+chuenchen309@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

DCO Check Passed

Thanks @chuenchen309, all your commits are properly signed off. 🎉

@mergify

mergify Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ceberam

ceberam commented Jul 17, 2026

Copy link
Copy Markdown
Member

Thanks @chuenchen309 for suggesting this fix.
Before checking in detail, I was wondering if we could extend this PR to ensure that GFM-compliant tables are fully covered by docling's markdown parser backend:

  • will this PR also allow the parsing of tables without a leading pipe?
  • could you ensure that the edge cases described in the GFM 4.10 Tables (extension) will also be correctly parsed? For instance, cases like:

    If there are a number of cells fewer than the number of cells in the header row, empty cells are inserted. If there are greater, the excess is ignored_

@chuenchen309

Copy link
Copy Markdown
Author

Thanks @ceberam! I checked both against the current backend:

Tables without a leading pipe — the split side already handles this: _split_table_row only drops an empty edge cell, so Region | Q1 splits to two cells correctly. The blocker is the row detector at md_backend.py:502, which gates on original_text.lstrip().startswith("|") to enter table mode, so a table whose header has no leading pipe isn't detected as a table at all:

"Region | Q1\n--- | ---\nNorth | 10\n"   ->  0 tables detected

So this PR alone doesn't cover it. Enabling it means relaxing that detector, which is riskier — any prose line containing | becomes a table candidate — so it would want a tighter heuristic (e.g. also require the next line to be a delimiter row).

GFM cell-count mismatch (4.10) — currently not normalized; the grid comes out ragged rather than padded/truncated:

header 3 cols, body "| 1 | 2 |"       ->  num_cols=3, but the body row yields only 2 cells   (GFM: pad an empty 3rd)
header 2 cols, body "| 1 | 2 | 3 |"   ->  num_cols=2, but the body row keeps 3 cells         (GFM: drop the excess)

Happy to extend this PR to cover both. They're fairly independent of the trailing-pipe drop, though, and the detector change carries the false-positive risk above — so would you prefer one combined PR, or keep this one scoped to the trailing-pipe fix and track full GFM 4.10 coverage as a follow-up? Either works for me.

@ceberam

ceberam commented Jul 21, 2026

Copy link
Copy Markdown
Member

Happy to extend this PR to cover both. They're fairly independent of the trailing-pipe drop, though, and the detector change carries the false-positive risk above — so would you prefer one combined PR, or keep this one scoped to the trailing-pipe fix and track full GFM 4.10 coverage as a follow-up? Either works for me.

Thanks @chuenchen309 for the detailed analysis. I think it would be better if you could tackle all those edge cases in a single PR, to ensure consistency in the code. It would be about dealing with markdown tables without leading/trailing pipes.

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