Repository navigation
fix(xlsx): skip worksheets that have no columns - #2718
OUAO-FRANK wants to merge 2 commits into
Conversation
|
OUAO-FRANK please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Bicheng (Kenneth) (BichengWang)
left a comment
There was a problem hiding this comment.
Reviewed head 640f843ed3d99495757806eb6bb36bd33190ea08.
[P2] Preserve image-only worksheets before skipping the sheet — packages/markitdown/src/markitdown/converters/_xlsx_converter.py:157-158
columns.empty only describes cell data. A worksheet containing an embedded image and no cell values can still produce content through the supported _image_to_html extension. This continue bypasses images.to_html(...) below, so the hook is never called and the entire sheet silently disappears.
I reproduced this through MarkItDown.convert_stream() using an XLSX created by openpyxl:
import io
from openpyxl import Workbook
from openpyxl.drawing.image import Image as SheetImage
from PIL import Image
from markitdown import MarkItDown, StreamInfo
from markitdown.converters import XlsxConverter
class CaptionedXlsx(XlsxConverter):
def _image_to_html(self, image_stream, stream_info, **kwargs):
return "<p>Quarterly revenue chart</p>"
png = io.BytesIO()
Image.new("RGB", (2, 2), "red").save(png, "PNG")
png.seek(0)
wb = Workbook()
wb.active.title = "ChartOnly"
wb.active.add_image(SheetImage(png), "A1")
source = io.BytesIO()
wb.save(source)
source.seek(0)
md = MarkItDown()
md.register_converter(CaptionedXlsx())
result = md.convert_stream(source, stream_info=StreamInfo(extension=".xlsx"))
assert "Quarterly revenue chart" in result.markdownThe assertion passes on current main (4cc9fa17653d695d64fb9eee5b33d4de55ff84e8) and fails on this head, where result.markdown == "". An instrumented hook is called once on main and zero times here.
Please skip the empty table separately from image processing, and omit the worksheet heading only if neither path produces content. Add a regression for an image-only worksheet with an overridden hook alongside the existing genuinely empty-sheet cases.
Validation on Python 3.12.15/macOS: the existing XLSX suite passes (29 tests), as do the selected library/CLI vectors (33 passed, 6 remote tests skipped). Black 23.7.0 accepts both changed files. The separate image-only regression above fails on the PR and passes on main; the current tests do not cover it.
|
Thanks for the precise repro — you're right that Fixed in
Added Verification: $ pytest tests/test_xlsx.py -q
30 passed
$ black --check src/markitdown/converters/_xlsx_converter.py tests/test_xlsx.py
2 files would be left unchangedMutation check: restoring the old The |
Summary
A completely empty worksheet is rendered as a heading followed by two lines of broken table syntax, because
to_html()on a frame with no columns produces a table with no header, andmarkdownifyturns that into|\n| |:The last two lines are not a table — there is no delimiter row — so a renderer emits them as literal text, and any consumer of the Markdown sees a malformed document.
sheets[s].columns.empty), before the heading is written. That is the XLSX path and the byte-identical XLS path, which had the same loop.## HeaderOnly\n| col |\n| --- |), and that is the behaviour worth keeping. Only the no-columns case is broken.existingClaim-style options and the.xls/.xlsxselection are untouched.Testing
Four new cases at the end of
tests/test_xlsx.py(no existing case covered a sheet with zero columns):'## Empty\n|\n| |''''## HeaderOnly\n| col |\n| --- |''...| v |\n\n## Empty\n|\n| |''## HasData\n| col |\n| --- |\n| v |''## HasData\n| col |\n| --- |\n| v |'Mutation check: with the guard removed, 3 of the 4 cases fail (
test_completely_empty_sheet_is_skipped...,test_data_sheet_followed_by_an_empty_sheet,test_empty_sheet_does_not_hide_the_next_sheet), so the regression bites. The header-only case passes both ways by design.Reproduced end-to-end first with the snippet from the issue (
MarkItDown().convert_stream(..., StreamInfo(extension=".xlsx"))) againstmainat4cc9fa1, which produced exactly the malformed output quoted above.Closes #2470