Repository navigation
fix(converters): preserve literal NA strings in XLSXToDocument (#12945) - #13044
HarshitR2004 wants to merge 8 commits into
Conversation
|
@HarshitR2004 is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @HarshitR2004, thanks for your interest in contributing to Haystack! 🙏 This is an automated message to help us keep the review queue healthy. |
bogdankostic
left a comment
There was a problem hiding this comment.
Thanks for working on this @HarshitR2004!
Defaulting to keep_default_na=False fixes #12945, but it breaks empty-cell handling for users who pass their own na_values. On main, these are added to pandas' default list, which includes "". With this PR, they replace the [""] default, so empty cells are no longer treated as missing:
# sheet with an empty cell in column B and "foo" in column A
converter = XLSXToDocument(
read_excel_kwargs={"na_values": ["foo"]},
table_format_kwargs={"na_rep": "<missing>"},
)
# main:
# 2,x,<missing>
# 3,<missing>,y
# this PR:
# 2,x,
# 3,<missing>,ySame for missingval with table_format="markdown". For lists, adding "" to the user-provided na_values fixes this.
Apart from this, let's add the upgrade note and the test for passing na_values.
| By default, `keep_default_na` is set to False and `na_values` to `[""]` so literal text | ||
| values like "NA" and "N/A" are preserved while empty cells remain empty. |
There was a problem hiding this comment.
Let's also mention what happens when na_values is passed. Right now this reads as if it's added to the [""] default, but it replaces it (see my comment on _extract_tables).
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| fixes: | |||
There was a problem hiding this comment.
This changes the output for everyone using the default settings, e.g. cells containing "NULL" or "None" now end up in the content instead of being emptied. Let's add an upgrade section explaining how to get the old behavior back.
| if table_format == "csv": | ||
| rows = [row[1:] for row in list(csv.reader(io.StringIO(content)))[1:]] | ||
| assert rows == [ | ||
| ["Code", "Status", "Blank"], | ||
| ["NA", "OK", ""], | ||
| ["N/A", "pending", ""], | ||
| ["North America", "active", ""], | ||
| ] | ||
| else: | ||
| rows = [[cell.strip() for cell in row.split("|")[2:-1]] for row in content.splitlines()[2:]] | ||
| assert rows == [ | ||
| ["Code", "Status", "Blank"], | ||
| ["NA", "OK", ""], | ||
| ["N/A", "pending", ""], | ||
| ["North America", "active", ""], | ||
| ] |
There was a problem hiding this comment.
nit: the expected rows are the same for both formats, so let's assert once after parsing:
| if table_format == "csv": | |
| rows = [row[1:] for row in list(csv.reader(io.StringIO(content)))[1:]] | |
| assert rows == [ | |
| ["Code", "Status", "Blank"], | |
| ["NA", "OK", ""], | |
| ["N/A", "pending", ""], | |
| ["North America", "active", ""], | |
| ] | |
| else: | |
| rows = [[cell.strip() for cell in row.split("|")[2:-1]] for row in content.splitlines()[2:]] | |
| assert rows == [ | |
| ["Code", "Status", "Blank"], | |
| ["NA", "OK", ""], | |
| ["N/A", "pending", ""], | |
| ["North America", "active", ""], | |
| ] | |
| if table_format == "csv": | |
| rows = [row[1:] for row in list(csv.reader(io.StringIO(content)))[1:]] | |
| else: | |
| rows = [[cell.strip() for cell in row.split("|")[2:-1]] for row in content.splitlines()[2:]] | |
| assert rows == [ | |
| ["Code", "Status", "Blank"], | |
| ["NA", "OK", ""], | |
| ["N/A", "pending", ""], | |
| ["North America", "active", ""], | |
| ] |
| ["North America", "active", ""], | ||
| ] | ||
|
|
||
| def test_run_keep_default_na_override(self, tmp_path: Path) -> None: |
There was a problem hiding this comment.
Let's also add a test that passes na_values via read_excel_kwargs and checks that empty cells still get na_rep/missingval. That covers the case from my comment on _extract_tables.
|
@bogdankostic Thanks for the review! I’ve fixed empty-cell handling with custom na_values, added regression tests, updated the docs and upgrade note, and simplified the assertion as suggested. All new tests and quality checks pass. One existing line-ending test still fails on the unchanged code, which I’ve noted in the PR. |
|
Thanks for the updates @HarshitR2004! The list case works now, but other forms of na_values still break empty cells, and the docs need to be moved to the right version |
| """ | ||
| file_bytes = io.BytesIO(bytestream.data) | ||
| resolved_read_excel_kwargs = { | ||
| "keep_default_na": False, |
There was a problem hiding this comment.
Two behaviour changes the upgrade note should mention (the suggestion above avoids the second one):
- read_excel_kwargs={"na_values": [...]} used to add to pandas' default NA strings. Now it replaces them, so NA/NULL text stays in the output.
- read_excel_kwargs={"keep_default_na": False}: we now also add na_values=[""], so empty cells become NaN and numeric columns with gaps turn into floats.
| "engine": "openpyxl", # Use openpyxl as the engine to read the Excel file | ||
| } | ||
| na_values = resolved_read_excel_kwargs["na_values"] | ||
| if isinstance(na_values, list) and "" not in na_values: |
There was a problem hiding this comment.
"" only gets added when na_values is a list. If someone passes a tuple, set, string, dict or None, empty cells come through as "" strings and na_rep/missingval won't apply to them.
XLSXToDocument(read_excel_kwargs={"na_values": ("foo",)}) # blank cells no longer rendered as missingA simpler way to handle every case: only set our defaults when the user passes neither key, and otherwise leave their kwargs untouched.
if "keep_default_na" not in self.read_excel_kwargs and "na_values" not in self.read_excel_kwargs:
resolved_read_excel_kwargs.update(keep_default_na=False, na_values=[""])
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
| ["North America", "active", ""], | ||
| ] | ||
|
|
||
| @pytest.mark.parametrize("table_format", ["csv", "markdown"]) |
There was a problem hiding this comment.
Could you parametrize this test over a tuple, a dict and None for na_values as well? Those are the cases that break right now.
|
|
||
| To see the additional parameters that you can specify with the component initialization, check out the [API Reference](/reference/converters-api#xlsxtodocument). | ||
|
|
||
| ### Missing values |
There was a problem hiding this comment.
Please revert this file. The change ships in 3.5, so 3.3 users would read about behaviour they don't have. Updating docs-website/docs/ is enough.
|
Hi @HarshitR2004, thanks a lot for your contribution! 🙏 We noticed that the Contributor License Agreement (CLA) check ( To get your PR reviewed, please sign the CLA via the link in the |
|
Hi @HarshitR2004, just a friendly reminder: this PR is still in draft because the Contributor License Agreement (CLA) hasn't been signed yet. We'd love to review your contribution! Please sign the CLA via the link in the |
|
Thanks for signing the CLA, @HarshitR2004! 🎉 This PR is now ready for review again and the reviewer has been re-assigned. |
|
@davidsbatista Thanks for the feedback! I’ve applied the suggested conditional defaults, preserving pandas behavior whenever either NA option is supplied. Added tests for all supported input forms, explicit overrides, and numeric formatting; updated the docstring and upgrade note and reverted the 3.3 docs change. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
davidsbatista
left a comment
There was a problem hiding this comment.
thanks for the contribution!
|
@bogdankostic have a look when you have the time |
Related Issues
Proposed Changes
XLSXToDocumentpreserves literal NA strings when callers have not configured NA parsing, while retaining pandas' existing behavior for explicit NA settings.keep_default_na=Falseandna_values=[""]only when neither option is present inread_excel_kwargs. This keeps literal"NA","N/A","NULL", and"None"while still recognizing empty cells as missing.na_valuesalone supplement pandas' built-in defaults; explicitkeep_default_na=Falsekeeps its existing empty-cell and numeric-formatting behavior.Nonein CSV and Markdown. Test explicit overrides,na_filter=False, unrelated read options, numeric columns with gaps, and unchanged caller configuration.How did you test it?
Validation used a workspace-local Hatch environment with Python 3.12.10, pandas 3.0.6, openpyxl 3.1.5, and tabulate 0.10.0.
hatch -e review run pytest test/components/converters/test_xlsx_to_document.py -q --tb=short: 66 passed, 1 failed, including all 38 NA-parsing cases passing.test_run_markdown_escapes_cell_content[windows-line-break]case: two spaces are produced instead of one. It was reproduced using the upstream converter from current basec41c45de0648e279efbd6d988eae638d1b85f7dfin the same environment.mypy --follow-imports=silent haystack/components/converters/xlsx.py test/components/converters/test_xlsx_to_document.py: passed.Notes for the reviewer
The review follow-up was implemented with AI assistance. The local results above distinguish the passing NA-parsing coverage from the reproduced upstream line-ending failure.
Checklist