fix(table): decide MergeSplitTokens per table, not per row - #18
Conversation
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.
Reviewer's GuideThis PR changes mergeSplitTokens to decide which column boundaries to merge once per table (across all rows) instead of per row, ensuring rectangular tables, and adds a regression test capturing this invariant. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 35 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 (2)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the
dropcomputation loop you indexrows[ri][ci]androws[ri][ci+1]without checking thatci/ci+1are withinlen(rows[ri]), which will panic for ragged tables whererowsis shorter thancellsin some columns. - The new
for ci := 0; ci < cols; ci++iteration appends empty cells for short rows, changing row length semantics; if some consumers rely on original row lengths, consider explicitly documenting or constraining this behavior (e.g., by enforcinglen(rows) == len(cells)or trimming trailing empties in the output).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `drop` computation loop you index `rows[ri][ci]` and `rows[ri][ci+1]` without checking that `ci`/`ci+1` are within `len(rows[ri])`, which will panic for ragged tables where `rows` is shorter than `cells` in some columns.
- The new `for ci := 0; ci < cols; ci++` iteration appends empty cells for short rows, changing row length semantics; if some consumers rely on original row lengths, consider explicitly documenting or constraining this behavior (e.g., by enforcing `len(rows) == len(cells)` or trimming trailing empties in the output).
## Individual Comments
### Comment 1
<location path="page.go" line_range="756" />
<code_context>
+ if l.IsZero() || r.IsZero() {
+ continue
+ }
+ if rows[ri][ci] == "" || rows[ri][ci+1] == "" {
+ continue
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential out-of-bounds access on rows[ri] when ci extends beyond that row’s length.
Because cols is based on the max row length, the outer loop can reach ci values that exceed the length of rows[ri]. In this pre-scan you access rows[ri][ci] and rows[ri][ci+1] without checking against len(rows[ri]), so shorter rows will panic. Please add bounds checks (like those used for cells) or limit this logic to ci < len(rows[ri]) - 1 when consulting rows.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if l.IsZero() || r.IsZero() { | ||
| continue | ||
| } | ||
| if rows[ri][ci] == "" || rows[ri][ci+1] == "" { |
There was a problem hiding this comment.
issue (bug_risk): Potential out-of-bounds access on rows[ri] when ci extends beyond that row’s length.
Because cols is based on the max row length, the outer loop can reach ci values that exceed the length of rows[ri]. In this pre-scan you access rows[ri][ci] and rows[ri][ci+1] without checking against len(rows[ri]), so shorter rows will panic. Please add bounds checks (like those used for cells) or limit this logic to ci < len(rows[ri]) - 1 when consulting rows.
1 participant
Follow-up to #17, found by looking at output on a real filing rather than at the unit tests.
The bug I shipped
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's 2018 10-K page 58, the header band contains the split (
December 3+1,); the data rows below it do not. So the header merged and the data rows did not:The grid sheared. The header's second date ended up above the first column of figures. That is worse than the split it set out to fix — and it is exactly the kind of damage that passes a spot check while ruining the table for anything consuming it positionally.
The fix
Decide once per boundary, across every row, then apply uniformly:
Rectangular, aligned, and the
$now reads with its value.The trade-off: 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.
Verification
go build,go vet,go test ./... -count=1 -race— green.TestMergeSplitTokensKeepsTableRectangularuses a table whose first row splits and whose second does not, and asserts equal column counts — not just the merged text. Asserting only the text is what let the shearing through the first time.Relates to HAL-548
Summary by Sourcery
Make merge-split decisions at the table level instead of per row to keep column boundaries consistent and tables rectangular.
Bug Fixes:
Enhancements:
Tests: