Conversation
DOCX table HTML built one <td> per grid position that a gridSpan/vMerge merge visually covered, repeating that cell's text into each one with no colspan/rowspan attribute. Track the underlying tc element's identity across grid positions and collapse matching runs into a single <td> carrying the appropriate colspan/rowspan, so merged-cell text appears exactly once and the merge geometry is recoverable from the HTML.
htmlify_matrix_of_spanned_cell_texts() suppressed rows with no originating cells, but such a row can legitimately represent a real grid-row entirely covered by a rowspan from an earlier row. Since HTML rowspan counts actual <tr> elements rather than "rows that had content", dropping the row shifted the column-placement of every subsequent row. Add a regression test through the public partition_docx() API, plus a combined collapse-and-render test covering a full-width (every-column) vertical merge -- the collapse and render steps were previously only unit-tested in isolation, which is how this went unnoticed.
The table chunker split purely on row text length, with no awareness of an active `rowspan`. A split falling inside one left the origin chunk claiming more rows than were present, and shifted every cell in the continuation chunk into the wrong column (a fresh standalone `<table>` has no earlier row to carry the span forward). Rows spanned by an earlier row's `rowspan` are now grouped and chunked as one atomic unit, computed via the standard overlapping-interval merge over each row's declared span reach. A group that doesn't fit even alone is emitted as one (necessarily oversized) chunk, the same tolerance already granted a single oversized row or cell.
The rowspan-aware chunking boundary could silently lose an entire table's trailing rows when a declared rowspan named more rows than the table had (the group-closing index was never reached, so the final group was never yielded), and coerced rowspan="0" (HTML's "span every remaining row") to 1, letting a chunk boundary fall through an active maximal span. Both now resolve to "the rest of the table."
…ot row-group A rowspan="0" cell (or any positive rowspan) was resolved as reaching to the end of the whole table rather than the end of its own row-group, so a short header section could swallow an entire, otherwise-boundable body section into one unbounded chunk. Rows are now grouped by the identity of their actual containing <thead>/<tbody>/<tfoot> element (or the table itself, for a row with no section wrapper), and a span can no longer bind rows across a real section boundary.
…0" cell The chunk accumulator packed row-groups together purely by character budget, so a rowspan="0" header bound to its own one-row <thead> could still end up in the same chunk as following <tbody> rows once section wrappers are stripped from the emitted HTML -- reintroducing the column-shift corruption this feature exists to prevent. A row-group change is now a hard boundary specifically when a rowspan="0" row is involved; ordinary rows, and rows with a positive rowspan meant to carry across a boundary (e.g. a repeated header), are unaffected -- an earlier version of this fix flushed on every row-group change unconditionally and broke that intentional behavior.
A positive rowspan declaring more rows than its own thead/tbody/tfoot row-group actually has is clipped when grouping rows for chunking, but its emitted HTML still carries the original, uncorrected value. If that group got packed into the same chunk as a following row-group's rows, the value would legitimately reach into rows it was never meant to bind once section wrappers are stripped. A row-group change is now also a hard chunk boundary when the preceding group was clipped this way. A thead row's positive span is exempt, since it's already handled separately as a repeated/carried-forward header.
…alues
Replace chunk-boundary bookkeeping ("is this group unsafe to extend
across a row-group boundary") with a structural safety net: at the
point each chunk's rows are finalized, any cell whose declared
rowspan (including rowspan="0") would claim more rows than are
actually present in that same chunk gets its rowspan rewritten to
match reality. An emitted span can no longer overreach regardless of
what else gets packed into the same chunk, closing this class of bug
by construction rather than by enumerating unsafe cases one at a time.
The existing row-group-boundary flush logic is kept (it still avoids
visually merging a clipped group's own row-group with unrelated
content), but two real gaps in it are fixed along the way:
- An overdeclared <thead> rowspan was unconditionally exempted from
clipping, even when header repetition isn't actually configured/
active for it -- nothing else protects such a row, so it's now only
exempted when it's a genuine, active carried-forward header row.
- The accumulator compared a candidate group's row-group identity
against the FIRST row it had ever accumulated rather than the most
recently accumulated one, which could miss a real transition when
an earlier and later row happened to share the same row-group
identity (e.g. two direct rows around an explicit <tbody>).
…nd the thead row's original occurrence correctly html_clipped_to_rows() previously reconstructed each cell from its plain text when correcting an overreaching rowspan, discarding any nested table, hyperlink, image, or other markup/attributes the source cell carried. It now deep-copies the real <tr> and mutates only the rowspan attribute on cells that need correction, leaving everything else byte-for-byte unchanged. Separately, the carried-header exemption in _iter_rowspan_bound_row_groups was keyed on whether header repetition was configured at all, which incorrectly also exempted the thead row's own ORIGINAL, wrapper-less occurrence -- not just an actual repeated/carried copy (built separately by _as_header_row_html, wrapped in its own real <thead>, and never routed through this bound at all). Every row is now bounded to its own row-group uniformly; only a genuinely repeated copy escapes correction, because it was never subject to it in the first place.
A rowspan-bound row too large to fit any chunk even alone was handed to the cell splitter with its original, uncorrected rowspan attribute -- the self-correcting rewrite only applied to rows going through the normal row accumulator. reconstruct_table_from_chunks() can reassemble separately-emitted chunks, at which point the stale span reaches into rows from a later chunk. The row's own bound is now applied via row_clipped_to_rows() before it's split cell-by-cell.
Each review-fix round bumped __version__ and added its own CHANGELOG entry as part of that round's commit, leaving 9 separate release-note entries and a 9-patch version jump for what is one user-facing fix. Consolidated into a single 0.27.6 entry and a one-increment version bump.
…ract Comments that compared a test's current assertion to what it used to be (e.g. "4 chunks, not 3", "now correctly recognized", "the pre-fix behavior") are rewritten to describe only the current contract, or removed where the assertion/code already made the point without a comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
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="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:5">
P2: The 0.27.6 release note overstates the chunking fix: boundaries between rows sharing a rowspan can still misattribute spanned rows, according to the stated scope of this PR. Remove the chunking claim or qualify it to describe only the cases actually fixed.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
|
||
| ### Fixes | ||
|
|
||
| - **Stop duplicating merged-cell text in DOCX `text_as_html`.** A merged cell (`gridSpan`/`vMerge`) was repeated into every `<td>` its merge visually covered, with no `colspan`/`rowspan` attribute marking the merge; merged cells are now emitted once, with `colspan`/`rowspan` reflecting the true geometry. Since DOCX tables can now carry real spans, table chunking was also made rowspan-aware, so a chunk boundary can no longer split a table in a way that misattributes a spanned cell's rows to the wrong columns. |
There was a problem hiding this comment.
P2: The 0.27.6 release note overstates the chunking fix: boundaries between rows sharing a rowspan can still misattribute spanned rows, according to the stated scope of this PR. Remove the chunking claim or qualify it to describe only the cases actually fixed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CHANGELOG.md, line 5:
<comment>The 0.27.6 release note overstates the chunking fix: boundaries between rows sharing a rowspan can still misattribute spanned rows, according to the stated scope of this PR. Remove the chunking claim or qualify it to describe only the cases actually fixed.</comment>
<file context>
@@ -1,3 +1,9 @@
+
+### Fixes
+
+- **Stop duplicating merged-cell text in DOCX `text_as_html`.** A merged cell (`gridSpan`/`vMerge`) was repeated into every `<td>` its merge visually covered, with no `colspan`/`rowspan` attribute marking the merge; merged cells are now emitted once, with `colspan`/`rowspan` reflecting the true geometry. Since DOCX tables can now carry real spans, table chunking was also made rowspan-aware, so a chunk boundary can no longer split a table in a way that misattributes a spanned cell's rows to the wrong columns.
+
## 0.27.5
</file context>
| - **Stop duplicating merged-cell text in DOCX `text_as_html`.** A merged cell (`gridSpan`/`vMerge`) was repeated into every `<td>` its merge visually covered, with no `colspan`/`rowspan` attribute marking the merge; merged cells are now emitted once, with `colspan`/`rowspan` reflecting the true geometry. Since DOCX tables can now carry real spans, table chunking was also made rowspan-aware, so a chunk boundary can no longer split a table in a way that misattributes a spanned cell's rows to the wrong columns. | |
| - **Stop duplicating merged-cell text in DOCX `text_as_html`.** A merged cell (`gridSpan`/`vMerge`) was repeated into every `<td>` its merge visually covered, with no `colspan`/`rowspan` attribute marking the merge; merged cells are now emitted once, with `colspan`/`rowspan` reflecting the true geometry. Table chunking now preserves rowspan relationships where supported; splitting between rows that share a rowspan remains a known limitation. |
…ble branch, and harden tests Nested tables inside a DOCX cell are flattened to text; that flattening duplicated a merged cell's text at every grid position it covered, the same class of bug this PR already fixed for top-level tables. Also removes a table-chunking branch in _iter_rowspan_bound_row_groups that can never execute (the last row's own row-group always closes on the loop's final iteration), makes the chunking test module collectible without the optional pandas extra, drops a verbatim-duplicate test, and strengthens a chunking regression test to check exact per-chunk HTML instead of only word presence.
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Summary
1. (High) Rowspan crossing a section boundary is clipped, shifting every following columnWhere: What's wrong: A positive Reproduced: For <table>
<thead><tr><th rowspan="2">Name</th><th>Field</th></tr></thead>
<tbody>
<tr><td>Age</td></tr>
<tr><td>person00</td><td>val00 extra padding text</td></tr>
<tr><td>person01</td><td>val01 extra padding text</td></tr>
</tbody>
</table>
Proposed fix: Stop clamping a positive Note on the ambiguous case: An overdeclared header span whose body rows are already full-width (a cell in every column) is genuinely different from a span that legitimately covers a short row, but the two cannot be distinguished from row-group membership alone. Binding by true reach is the safe choice: it never shifts columns. The trade-off is that an overdeclared span may keep a slightly larger group together than strictly necessary, which is acceptable since correctness outranks split granularity. The existing in-tree assertions only cover the overdeclared-with-full-body case, so they will need to be updated to reflect the corrected behavior. 2. (High) Repeated headers re-emit the pre-clip
|
…n groups A rowspan may legitimately reach from a <thead> into a <tbody> (or <tbody> into <tfoot>); table chunking previously clipped any positive rowspan to its own row-group, silently truncating a valid span and shifting later rows into the wrong column. It's now bound only by the table's actual last row (rowspan "0" still clips to its own row-group, since that's what the HTML spec scopes it to). Repeating a table header on continuation chunks could re-inject a header cell's rowspan uncorrected, letting it reach past the repeated header block into the continuation's own body content. The repeated copy is now clipped to the number of header rows actually carried over. Finally, a rowspan-bound group too large to fit in one chunk was previously emitted whole, violating the chunk size limit -- increasingly likely once a positive rowspan's larger legitimate reach (above) is accounted for. It's now split on a row boundary like an ordinary oversized row, with the covering cell's rowspan rewritten per fragment and re-materialized in any fragment that doesn't include the row that originally declared it.
There was a problem hiding this comment.
1 issue found across 3 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="unstructured/chunking/base.py">
<violation number="1" location="unstructured/chunking/base.py:1417">
P2: When an oversized rowspan-bound group is split, this text-only materialization drops nested markup and image-only content from `text_as_html`. Preserve the source cell HTML alongside the span metadata when emitting continuation cells.</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.
Re-trigger cubic
| if next_span is not None and next_span.col == col: | ||
| if materialize: | ||
| remaining = next_span.reach_idx - idx + 1 | ||
| cells.append(_format_td(next_span.text, next_span.colspan, remaining)) |
There was a problem hiding this comment.
P2: When an oversized rowspan-bound group is split, this text-only materialization drops nested markup and image-only content from text_as_html. Preserve the source cell HTML alongside the span metadata when emitting continuation cells.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unstructured/chunking/base.py, line 1417:
<comment>When an oversized rowspan-bound group is split, this text-only materialization drops nested markup and image-only content from `text_as_html`. Preserve the source cell HTML alongside the span metadata when emitting continuation cells.</comment>
<file context>
@@ -1367,6 +1381,114 @@ def _group_last_idx(rows: Sequence[HtmlRow]) -> list[int]:
+ if next_span is not None and next_span.col == col:
+ if materialize:
+ remaining = next_span.reach_idx - idx + 1
+ cells.append(_format_td(next_span.text, next_span.colspan, remaining))
+ if next_span.text:
+ texts.append(next_span.text)
</file context>
…ng empty spanned cells Oversized-cell text splitting now keeps the cell's colspan on each emitted fragment, and HtmlCell.html now preserves colspan/rowspan for empty cells instead of collapsing them to a bare <td/>.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…cells The fixed-overhead assumption for an oversized cell's split fragments didn't account for a colspan attribute's own characters, or for the fact that escaping cell text (&, <, >) can make the formatted fragment longer than the raw text budgeted for it. Overhead is now derived from the cell's actual colspan, and each split verifies the formatted fragment against the limit, shrinking and re-splitting (falling back to a raw truncation in pathological cases) until it fits.
There was a problem hiding this comment.
1 issue 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="test_unstructured/chunking/test_base.py">
<violation number="1" location="test_unstructured/chunking/test_base.py:3056">
P2: This escaping test does not verify preservation of the escaped character. Compare the joined chunk text with the original `text` and assert at least one emitted fragment contains `&`, otherwise regressions can pass while corrupting cell content or escaping.</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.
Re-trigger cubic
| for _, html in chunks: | ||
| assert len(html) <= 50 | ||
| # -- no text lost or duplicated across the split -- | ||
| assert " ".join(text for text, _ in chunks).replace(" & ", " ").split() == ["x"] * 20 |
There was a problem hiding this comment.
P2: This escaping test does not verify preservation of the escaped character. Compare the joined chunk text with the original text and assert at least one emitted fragment contains &, otherwise regressions can pass while corrupting cell content or escaping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test_unstructured/chunking/test_base.py, line 3056:
<comment>This escaping test does not verify preservation of the escaped character. Compare the joined chunk text with the original `text` and assert at least one emitted fragment contains `&`, otherwise regressions can pass while corrupting cell content or escaping.</comment>
<file context>
@@ -3021,6 +3021,55 @@ def and_it_preserves_colspan_when_splitting_an_oversized_cell(self):
+ for _, html in chunks:
+ assert len(html) <= 50
+ # -- no text lost or duplicated across the split --
+ assert " ".join(text for text, _ in chunks).replace(" & ", " ").split() == ["x"] * 20
+
+ def and_it_accounts_for_colspan_and_escaping_together_when_splitting_an_oversized_cell(self):
</file context>
| assert " ".join(text for text, _ in chunks).replace(" & ", " ").split() == ["x"] * 20 | |
| assert " ".join(chunk_text for chunk_text, _ in chunks) == text | |
| assert any("&" in html for _, html in chunks) |
The test compared only the parallel plain-text tuple element across fragments, never inspecting the emitted HTML itself, so a regression that broke escaping in the HTML specifically (while leaving the plain text correct) would have passed unnoticed.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
2 participants
Summary
DOCX table extraction produced two different representations of a merged cell:
.textincluded its content once, but.metadata.text_as_htmlrepeated the content into every<td>the merge visually covered, with nocolspan/rowspanattribute marking that a merge had happened at all.Root cause:
_convert_table_to_htmlbuilt its HTML frompython-docx'srow.cells, which resolves both horizontal (gridSpan) and vertical (vMerge="continue") merges by yielding the same underlying cell content at every grid position the merge spans. That already-expanded grid was serialized straight into HTML with one<td>per matrix position and no span-collapsing step.Fix: track the underlying
tcXML element's identity (not cell text) when building the matrix — the sametcobject appears at every grid position a merge covers — then collapse runs of identical identity into(colspan, rowspan)before emitting<td>s. DOCX only permits rectangular merges, so there's no irregular-region case to handle.A new shared helper (
collapse_matrix_of_keyed_cells_to_spans) does the collapsing; the existinghtmlify_matrix_of_cell_texts(still used unchanged by the pptx and HTML-parser partitioners) now shares its cell-escaping logic with the new span-aware path via an extracted_format_tdhelper.Test plan
partition_docxonexample-docs/docx-tables.docx(the merged-cell fixture) now producestext_as_htmlwith correctcolspan/rowspanand no duplicated cell text — added a behavioral regression test through the publicpartition_docxAPI.test_docx.pyfixtures pinning the old duplicated-text/no-span output updated to the new expected output.unstructured/metrics/table/table_extraction.py's span-aware grid reconstruction already handlescolspan/rowspancorrectly (written for other span-producing sources) — traced by hand against the merged-cell fixture.max_charactersvalues. Found one pre-existing (not introduced here) limitation: splitting between rows that share arowspancan drop that cell's data in the later chunk — this was always latent for any spanned HTML source, just never exercised for DOCX before since DOCX never emitted spans until now. Not fixed in this PR; flagging as a possible follow-up.