fix(sheet): keep blank-looking cells out of the sheet extent - #159
fix(sheet): keep blank-looking cells out of the sheet extent#159vaibhavdabas16 wants to merge 2 commits into
Conversation
Producers fill a whole column with a single space down to the sheet floor. Those cells render as nothing, but they counted as populated, so the extent stretched from the content block to row 1,048,576 and the grid budget was charged for millions of positions that hold no content. A 6-column sheet with one such column needs 6,291,456 slots and does not convert at all. Skip cells whose text is entirely whitespace when recording content. The extent then follows the real content block, and output is unchanged either way: a whitespace-only cell already rendered blank. Applied to all three readers, which share `build_table` and so share the extent: xlsx, the legacy xls path, and xlsb. Addresses the whitespace half of firecrawl#156. The `max_xml_nodes` shape in that report is a separate cost, paid while parsing the sheet part before any of this runs.
There was a problem hiding this comment.
1 issue found across 3 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="src/formats/sheet/xlsx.rs">
<violation number="1" location="src/formats/sheet/xlsx.rs:305">
P2: When a whitespace-only cell is the origin of a merge beside real content, this filter removes the origin before `build_table` computes bounds, so the merge and its blank columns disappear. Preserve merge anchors for extent processing while still ignoring standalone whitespace cells.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // to the sheet floor, and counting those as populated stretches | ||
| // the extent over millions of empty positions. A cell that | ||
| // renders as nothing is nothing. | ||
| if !text.trim().is_empty() { |
There was a problem hiding this comment.
P2: When a whitespace-only cell is the origin of a merge beside real content, this filter removes the origin before build_table computes bounds, so the merge and its blank columns disappear. Preserve merge anchors for extent processing while still ignoring standalone whitespace cells.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/formats/sheet/xlsx.rs, line 305:
<comment>When a whitespace-only cell is the origin of a merge beside real content, this filter removes the origin before `build_table` computes bounds, so the merge and its blank columns disappear. Preserve merge anchors for extent processing while still ignoring standalone whitespace cells.</comment>
<file context>
@@ -297,7 +297,12 @@ fn read_sheet(
+ // to the sheet floor, and counting those as populated stretches
+ // the extent over millions of empty positions. A cell that
+ // renders as nothing is nothing.
+ if !text.trim().is_empty() {
out.cells.insert((cr, cc), text);
}
</file context>
A merge whose origin renders as nothing and which sits clear of the content block no longer widens the sheet, so that row is dropped along with it. Nothing in it rendered, but the shape does change, so pin both sides: the blank banner goes, a banner carrying text keeps its span.
|
Checked the cubic finding. It is real, and narrower than the wording suggests — but it does contradict a claim I made above, so correcting that. I probed three shapes, merge origin whitespace-only in each case, comparing
So B and C are untouched. The merge survives whenever it intersects the content block, because Only shape A changes: a merge anchored on a blank cell and sitting entirely clear of the content is no longer kept, so its row goes with it. What disappears is blank structure — an empty merged row, or trailing blank columns if the merge ran wider than the data. Nothing that rendered is lost, and I think that is the right behavior for this change and consistent with its premise, so I have covered it rather than special-cased it: Correcting myself on one point in the description: I wrote "output is unchanged either way". That is true for standalone whitespace cells but not for shape A, where an empty merged row is dropped. I have edited the description to say so. If you would rather blank merges keep their extent, the alternative is to preserve merge anchors through the filter and only ignore standalone whitespace cells — happy to switch it if that is your preference. |
Addresses the whitespace half of #156.
What happens
Producers fill a whole column with a single space down to the sheet floor. Those cells render as nothing, but
parse_sheetrecorded any cell whose text was not the empty string, so they counted as populated and the extent stretched from the real content block to row 1,048,576.The grid budget is then charged for millions of positions that hold no content. A 6-column sheet with one such column needs 6,291,456 slots and does not convert at all:
That is the shape reported in #156, where the content block is ~67 columns wide and one column of single-space cells runs to the sheet floor.
The change
Skip cells whose text is entirely whitespace when recording content. The extent then follows the real content block.
Applied to all three readers, which share
build_tableand therefore share the extent:xlsx.rs, the legacyxls.rspath (whoseputalready documented itself as matching the xlsx reader), andxlsb.rs.Effect on output
For a standalone whitespace cell, nothing changes — it already rendered blank, and
GridBuilderalready trimmed a trailing blank row, so the cost was paid on the budget without ever reaching the Markdown.One shape does change. A merge anchored on a whitespace-only cell that sits entirely clear of the content block is no longer retained, so its row goes with it — an empty merged row, or trailing blank columns if the merge ran wider than the data. A merge that intersects the content block is unaffected, because
build_tableretains merges against the bounds and then widens to them; a merge carrying real text keeps its full span. Both sides are pinned by tests.Tests
whitespace_fill_does_not_stretch_the_extent— the shape above. Onmainit fails withmax_grid_slotsat 6,291,456 positions; here the content block converts.a_blank_merged_row_outside_the_content_block_is_dropped— the behavior change, made explicit.a_merged_banner_with_real_text_is_untouched— a merged banner keeps its span, so that cannot silently regress.Full suite green (299 tests, no snapshot changes),
cargo fmt --checkandcargo clippy --workspace --all-targets --all-features -D warningsclean.Scope
This does not close #156. That report has two shapes and this fixes one of them:
max_xml_nodes(the tracker workbooks) is charged while parsing the sheet part into a DOM, before any of this code runs. A sheet part that decompresses to ~130 MB blows the node cap whatever the cells hold, so that one needs the sheet read without materializing every node — a much larger change I did not want to fold in here.max_grid_slotsfrom a lone real cell (theSUM(P54)checksum at column ~16,370) is genuine content at a genuine coordinate, so it is untouched by this. Dropping outlier columns would be a heuristic and a judgement call for you rather than something to slip into a bug fix — happy to take direction if you want it pursued.