feat(table): opt-in merging of tokens split by a column boundary - #17
Merged
Merged
Conversation
The text strategy derives column boundaries by clustering word edges, so a narrow band that happens to line up down the page becomes a column even when it cuts through a value. On a real 10-K balance sheet that yields | Less: Accumulated depreciation | ( | 16,135) | | | December 3 | 1, | where the document reads "(16,135)" and "December 31,". Nothing is lost, but a consumer treating a cell as one value gets two fragments, and Table.CellsBBox then covers only part of the value -- which matters now that those bboxes drive citation highlighting. Deliberately opt-in rather than fixed in the edge derivation. Checked against pdfplumber 0.11.9 on the same page: it produces the same splits and in fact splits more, breaking the row label into "Less: Accumula" and "ted depreciation" as well. So this is faithful parity, not a defect, and changing the default would quietly break the byte-compatibility this package promises. Callers who want clean values -- feeding a table to an LLM, say -- turn it on. Merging is bounded by TextTolerance, the same threshold word grouping uses, so it only rejoins glyphs Words() would have placed in one word. A genuine column gutter is far wider than an intra-word gap, so the "$" column of a financial statement survives untouched; a test pins that, since collapsing real columns would be a worse failure than the split it fixes. Cell bboxes are merged alongside the text.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideAdds an opt-in TableSettings.MergeSplitTokens flag and supporting merge logic to recombine cells whose column boundary splits a token, merges their bboxes, and thoroughly tests the behavior while documenting it in the changelog. Flow diagram for MergeSplitTokens cell merging behaviorflowchart TD
A[assembleTableText] --> B{MergeSplitTokens enabled?}
B -->|false| C[Return Table with original Rows and CellsBBox]
B -->|true| D[mergeSplitTokens]
D --> E[Iterate each row]
E --> F[Iterate each cell in row]
F --> G{Can merge with previous cell?\ntext non-empty, bboxes non-zero, boundarySplitsToken}
G -->|yes| H[Concatenate text into previous cell\nUnion bboxes]
G -->|no| I[Append new cell text and bbox]
H --> J[Build outRows and outCells]
I --> J
J --> K[Return Table with merged Rows and CellsBBox]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
hallelx2
added a commit
that referenced
this pull request
Aug 2, 2026
Follow-up to #17, caught by looking at the output on a real filing rather than at the unit tests. Merging was decided row by row. A column boundary is a property of the TABLE, so that was wrong in a way the tests could not see: on 3M 2018 10-K page 58 the header band contains the split ("December 3" + "1,") while the data rows below it do not. The header merged, the data rows did not, and the rows came out with different column counts -- so the header second date sat above the first column of figures. before | | December 31, | December 31, | <- 3 cells | Cash | $ | 2,853 | $ | 3,053 | <- 5 cells A sheared grid is a worse outcome than the split it set out to fix, and it is exactly the kind of damage that looks fine in a spot check and ruins the table for anything consuming it positionally. Now the decision is made once per boundary across every row, then applied uniformly, so the table stays rectangular: after | | December 31, | December 31, | | Cash and cash equivalents | $2,853 | $3,053 | The trade is that one row containing a split collapses that boundary for the whole table. That is the right direction: rectangularity matters more to a consumer than per-cell purity, and the merged result reads correctly anyway. TestMergeSplitTokensKeepsTableRectangular pins it with a table whose first row splits and whose second does not, asserting equal column counts rather than only the merged text -- the shearing is what the previous tests missed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
TableSettings.MergeSplitTokens, off by default.The problem
The
textstrategy derives column boundaries by clustering word edges, so a narrow band that happens to align down the page becomes a column — even when it cuts through a value. On 3M's 2018 10-K balance sheet:The document reads
(16,135)andDecember 31,. Nothing is lost, but a consumer treating a cell as one value gets two fragments, andTable.CellsBBoxcovers only part of the value — which now matters, since those bboxes drive citation highlighting (#15).Why opt-in, not a fix to the edge derivation
I checked against the reference rather than assuming. pdfplumber 0.11.9 on the same page:
So this is faithful parity, not a defect — and pdfplumber actually splits more, severing the row label too. Changing the default would silently break the byte-compatibility this package promises and would fail the goldens for the wrong reason.
(Incidentally that output also shows pdfplumber losing the closing paren on
16,048, which #14 fixed here — pdftable is now strictly better than the reference on that row.)Behaviour
Note what does not change:
The
$column survives, because there is a real 53pt gutter between the symbol and the number. That is a genuine column on the page, and collapsing it would be a worse failure than the split this fixes.How the distinction is made
Merging is bounded by
TextTolerance— the same threshold word grouping uses — so it only ever rejoins glyphs thatWords()would have placed in one word. In the split case the(ends at 436.90 and the1begins at 436.92: 0.02pt apart. A real gutter is orders of magnitude wider.Cell bboxes are merged alongside the text, so a highlight drawn from a merged cell covers the whole value.
Verification
go build,go vet,go test ./... -count=1 -race— all green.Closes HAL-548
Summary by Sourcery
Add an opt-in table setting to merge adjacent cells when a column boundary splits a single token, and wire it into table extraction while preserving existing behavior by default.
New Features:
Documentation:
Tests: