Repository navigation
Conversation
Signed-off-by: dada-yan <BinjunYann@gmail.com>
WaylandYang
left a comment
There was a problem hiding this comment.
Thank you, the attack is real and the shape is right: streaming worksheet_cells_reader for the true bounds before Range::new, ignoring the <dimension> hint, and filling only inside those bounds so set_value can never grow the grid. On dev, a two-column sheet with one stray cell at XFD1 ran for over 20 minutes at about 4 GB before being killed; with this PR it's bounded.
The problem is where the column budget is measured. SPREADSHEET_CELL_LIMIT / width uses the width of the whole sheet, so a cell that is never read still shrinks how many rows we read. Same build, dev versus this PR:
- 1,500 rows × 2 columns with one note in
AZZ1: dev reads all 1,501 rows (same text as without the note, since the renderer drops empty columns); this PR reads 145. - 2,000 rows × 150 dense columns: dev reads all 2,000; this PR reads 1,333.
Those are ordinary workbooks that import fully today, and a truncation warning doesn't make losing 90% of a ledger acceptable. Could you:
- Measure width only over the rows you are going to keep, so cells outside the 2,000-row window can't narrow it (your tall fixture should then read all 2,000 rows at width 2).
- Keep a cell budget for the genuinely wide case, but say in the comment why the number is what it is (the per-cell cost through
spreadsheet_gridand the renderer), and check whether 200k is lower than it needs to be. - Don't drop a sheet silently when
ensure!fires: the caller'scontinueswallows it with no warning. - Build the test workbooks in code like the existing
workbook()helper inspreadsheet_truncation.rs, instead of a binary fixture plus README, and keep one test per behaviour. - Fix the second-pass comment: the rows are counted in the first pass.
Happy to merge once legitimate wide and sparse sheets read the same as on dev.
Signed-off-by: dada-yan <BinjunYann@gmail.com>
Signed-off-by: dada-yan <BinjunYann@gmail.com>
WaylandYang
left a comment
There was a problem hiding this comment.
Thank you, this is the right fix. Rerunning the comparison against dev: the AZZ1 ledger now produces byte-identical output at 8 MiB instead of 346 MiB, and the XFD1 ledger, which dev only finished by being killed (240 s, about 4 GB), now reads all rows in about a second, identical to the ledger without the note. 2,000×150 dense and a tall sheet with a cell at P1000000 are identical too, and merged headers survive. Points 1, 2, 4 and 5 are resolved.
One regression is left, from how point 3 was handled. A workbook with a readable Ledger and one sheet holding a non-numeric value in a numeric cell imports the Ledger on dev; here the whole document fails with "Could not read worksheet Broken". Turning one bad sheet into losing every good sheet is worse than the silent skip, and .xls/.ods still skip anyway. The point in the review was only about the new budget ensure! dropping a sheet without saying so, and with the budget at 1M that can't happen for a real first row. Could you:
- Go back to dev's behaviour for unreadable sheets: skip the sheet, and keep
unwrap_or_default()for merges atparsers.rs:901. A warning for skipped sheets is worth having, but it needs its own warning kind and UI strings, so it belongs in a separate PR. Flipa_broken_sheet_is_reported_even_when_another_sheet_is_readableto assert that the Ledger still imports. - Remove the
ensure!s at:886and:948anda_first_row_that_exceeds_the_budget_is_reported.:856already caps a row at 16,384 cells, so they guard input we can't be given.
Optional: the re-projection loop only matters past 1M segments in one sheet. If dropping it leaves the text unchanged, the function gets a lot shorter.
Signed-off-by: dada-yan <BinjunYann@gmail.com>
Signed-off-by: dada-yan <BinjunYann@gmail.com>
Sparse XLSX files can expand into a huge rectangle of empty cells before the existing 2,000-row limit. A distant cell or a far-right header note can exhaust memory while importing an otherwise narrow ledger.
Scan row bounds and a bounded window of cell counts before retaining values. Store cells and empty spans while preserving physical column coordinates, merged headers and indentation. Budget at most one million retained source cells and rendered segments, keeping complete row prefixes and accurate existing truncation warnings. A normal 2,000×150 sheet remains intact. Keep dev's behavior for unreadable sheets and merge metadata, as well as chart-sheet handling, XLS/ODS layout, date/number formatting and the XLSX text fallback.
Recompute column and merge geometry after a segment-budget truncation: otherwise a far-right cell on a dropped row can still turn the retained title into a different header. The new regression fails when that reprojection is removed. The generated bad-sheet and bad-merge regressions also fail against the previous published revision.
The two-file diff includes the sparse replacement for the dense XLSX range, the shared row projection needed to preserve merged headers, and generated OOXML fixtures. These stay together because the fixtures verify the replacement's geometry and existing import behavior.
Validation at
5dc5baed6bc1de17233d18167a72c186df3f341e, including dev86bd5da5:Complete production-parser experiments, Linux with one CPU, 512 MiB and no swap:
All five exit successfully without OOM. The million-row sparse case reports exactly 2,000 rows read and 998,000 omitted; its cgroup peak is 5.2 MiB. The one-million-real-cell case has a 350.7 MiB cgroup peak. Normal dense/ledger text hashes match the prior dev controls.
The budget bounds XLSX grid storage; shared strings, individual cell lengths and other formats' total decoding memory remain outside this change.