Skip to content

fix(html): keep the rows under a rowspan in their own columns - #2522

Open
Lukas (L4XB) wants to merge 2 commits into
microsoft:mainfrom
L4XB:fix/table-rowspan
Open

Lukas (L4XB) wants to merge 2 commits into
microsoft:mainfrom
L4XB:fix/table-rowspan

Conversation

@L4XB

Copy link
Copy Markdown

The bug

A Markdown table has no way to merge cells downward, so a cell carrying a rowspan is written once and every row it reaches into comes out one cell short. The rows below it then read one column out of step.

Measured on main (eb31b5c):

| Region | Product | Units |
| --- | --- | --- |
| EU | Cable | 12 |
| Hub | 7 |          <- two cells against a three-column header
| US | Cable | 3 |

Read back, Hub sits under Region and 7 under Product. Units is empty for that row, and the count 7 is silently attributed to the wrong column — which is worse than losing it, because nothing about the output looks wrong.

A Word table with a vertically merged cell produces exactly this. Measured end to end on a .docx built with python-docx (table.cell(1, 0).merge(table.cell(2, 0))), mammoth writes the merge as rowspan="2":

<table><tr><td><p>Region</p></td>…</tr>
       <tr><td rowspan="2"><p>EU</p></td><td><p>Cable</p></td><td><p>12</p></td></tr>
       <tr><td><p>Hub</p></td><td><p>7</p></td></tr>…</table>

markdownify pads a colspan but has nothing for rowspan, so this reaches .docx, .html, .epub and RSS alike. A region spanning several product rows, a category spanning its items, a date spanning a day's entries — merged first columns are ordinary in real documents.

The fix

convert_soup fills the rows a span reaches into with the empty cells it stands for, before markdownify lays the table out:

def convert_soup(self, soup):
    _fill_row_spans(soup)
    return super().convert_soup(soup)

_fill_row_spans walks each table once and builds the grid the way HTML defines it — a cell takes the next column not already taken by a span from above, then claims colspan columns across and rowspan rows down. Each row that ends up with claimed columns gets an empty <td> at each of them.

Two details worth calling out:

  • placeholders are inserted in descending column order, each one directly in front of the first own cell at or after its column, so no index arithmetic is needed and the order comes out right even when several land next to each other;
  • _span clamps to 1..1000, the same bounds markdownify already applies to colspan, so a hostile rowspan="999999" cannot blow up the row count.

A table with no rowspan is untouched, and colspan keeps being handled by markdownify exactly as before.

Tests

packages/markitdown/tests/test_table_rowspan.py, 7 cases, all reading the produced table back the way a reader does and asserting on the cells:

  • the three-column example above;
  • a rowspan="3" filling both rows below;
  • a span in the last column, where the placeholder has to be appended rather than inserted;
  • a cell spanning in both directions, filling two columns on the next row;
  • two guards that must not move: a plain table and a colspan-only table;
  • an end-to-end .docx with a real vertically merged Word cell.

Measured:

result
with the change 7 passed
source change stashed, tests kept 5 failed, 2 passed

Full suite: 849 passed, 14 skipped, no failures. black --check clean.

Note: #2520 and #2521 also touch this file, in different places. All three are independent; I will rebase whichever lands later.

A Markdown table has no way to merge cells down, so a cell carrying a rowspan
is written once and every row it reaches into comes out one cell short:

    | Region | Product | Units |
    | --- | --- | --- |
    | EU | Cable | 12 |
    | Hub | 7 |

Read back, `Hub` sits under Region and `7` under Product, so the value is
attributed to the wrong column and Units is empty. A Word table with a
vertically merged cell produces exactly this: mammoth writes the merge as
rowspan="2".

Fill the rows a span reaches into with the empty cells it stands for, before
markdownify lays the table out.
Two costs in the rowspan padding, both reachable from any HTML input:

- A span asked for its placeholders unbounded. One cell with
  rowspan="1000" colspan="1000" over 999 more rows requested a million of
  them: 19 KB of HTML became 3 MB of Markdown in 20 s. A rowspan now reaches
  no further than the table's last row, and a table gets placeholders only
  while they stay within 64 + 8 per real cell; past that it is converted as
  markdownify converts it, without the padding. The same input now converts
  in 0.05 s.
- Each placeholder was inserted with insert_before, which finds its anchor
  by scanning the siblings, so many placeholders in one row cost O(n^2):
  30,000 in front of one cell took 12 s. The row is now rebuilt in one pass,
  taking its children out front to back (every extract() finds its node at
  index 0) and putting them back with the placeholders in place: 1 s.

Tables within the budget, including every existing case, come out the same.
@L4XB

Copy link
Copy Markdown
Author

Pushed 1538d62 after reviewers of the same padding logic elsewhere (QwenLM/qwen-code#12039, LearningCircuit/local-deep-research#6520) measured two costs that apply here too. Both are reachable from any HTML input:

input d994a48 1538d62
one cell rowspan="1000" colspan="1000" + 999 rows (19 KB) 3 MB of Markdown, 20.7 s 17 KB, 0.05 s
30,000 rowspan="2" cells, one cell below 11.8 s 1.0 s
a region spanning three product rows unchanged unchanged
  • Unbounded placeholders. A rowspan now reaches no further than the table's last row, and a table gets placeholders only while they stay within 64 + 8 per real cell. Past that it is converted exactly as markdownify converts it today, without the padding, so the output can no longer outgrow the page by more than a small factor.
  • Quadratic insertion. insert_before finds its anchor by scanning the siblings, once per placeholder. The row is now rebuilt in one pass: its children are taken out front to back (so every extract() finds its node at index 0) and put back with the placeholders in place.

Two tests added: test_a_span_attribute_does_not_blow_up_the_output and test_many_rowspans_are_filled_in_linear_time (both under a 5 s bound; on d994a48 they take 21 s and 11.8 s). The existing cases, including the Word end-to-end one, pass unchanged. tests/: 811 passed, 14 skipped (network cases deselected). Black 23.7.0 as pinned in pre-commit leaves both files unchanged.

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