[Harbor404]

Related Issues

• fixes XLSXToDocument assigns hyperlinks to the wrong cells with skiprows or usecols #13041

Proposed Changes:

XLSXToDocument stored hyperlinks using their original zero-based Excel coordinates but applied those coordinates directly to the filtered pandas DataFrame. When read_excel_kwargs contained skiprows or usecols, rows and columns shifted and hyperlinks were attached to the wrong cells.

This change maps original row positions through skiprows and derives selected column positions from the original integer column labels retained by pandas before replacing them with A/B column names. The unfiltered path keeps its existing identity mapping.

Regression tests cover external URL, internal sheet cell, and range-target hyperlinks with skiprows, usecols="B", and usecols="B:C".

This PR includes AI-assisted changes. The final diff was reviewed and tested locally.

How did you test it?

• uvx hatch run test:unit test/components/converters/test_xlsx_to_document.py -q - 32 passed
• uvx hatch run test:unit test/components/converters -q - 316 passed, 17 deselected, 1 warning
• uvx hatch run test:types - no issues in 550 source files
• uvx hatch run fmt-check - passed

Notes for the reviewer

skiprows supports integer, list-like, and callable forms. Column mapping uses the integer labels pandas retains with header=None, so string/list/callable usecols selections follow pandas without duplicating its parsing logic. When names replaces those labels, the code preserves the previous positional behavior.

Checklist

• I have read the contributors guidelines and the code of conduct.
• I have linked the related issue.
• I have added unit tests and updated the docstrings.
• I've used a conventional commit type for the PR title.
• I have documented my code.
• I have added a release note file.
• I have run pre-commit hooks and fixed any issue.

[vercel]

@Harbor404 is attempting to deploy a commit to the deepset Team on Vercel.

A member of the Team first needs to authorize it.

[amna-cpp]

Hi @Harbor404, thanks for working on this! I was also investigating #13041 and tested a few additional cases against commit a6d6dce.

I found that custom column names (names) and pandas’ comment option can still cause hyperlinks to attach to the wrong cells. In particular, names=[1, 0] swaps the links even without filtering, which currently works correctly on main.

I have tests and a local fix covering these cases. Would you be open to collaborating on adding that coverage and addressing them in this PR, or in a follow-up PR if you and the maintainers prefer?

Reproduction: expected vs. actual on a6d6dce

Three cases: names=[1, 0], names=["second"], usecols="B", and comment="#". The first passes on main; the other two also fail on main.

from io import BytesIO

from openpyxl import Workbook

from haystack.components.converters import XLSXToDocument
from haystack.dataclasses import ByteStream


def xlsx(rows: list[list[str]], links: dict[str, str]) -> ByteStream:
    workbook = Workbook()
    sheet = workbook.active
    for row in rows:
        sheet.append(row)
    for cell, url in links.items():
        sheet[cell].hyperlink = url
    buffer = BytesIO()
    workbook.save(buffer)
    return ByteStream(data=buffer.getvalue())


people = xlsx(
    [["Alice", "Bob"], ["Carol", "Dave"]], {"A1": "https://example.com/alice", "B1": "https://example.com/bob"}
)
notes = xlsx(
    [["a1", "b1"], ["# note", "b2"], ["a3", "b3"], ["a4", "b4"]],
    {"A3": "https://example.com/a3", "A4": "https://example.com/a4"},
)

cases = [
    (people, {"names": [1, 0]}, ",A,B\n1,[Alice](https://example.com/alice),[Bob](https://example.com/bob)\n2,Carol,Dave\n"),
    (people, {"names": ["second"], "usecols": "B"}, ",A\n1,[Bob](https://example.com/bob)\n2,Dave\n"),
    # ideal output; at minimum a4 must not receive a3's link
    (notes, {"comment": "#"}, ",A,B\n1,a1,b1\n2,[a3](https://example.com/a3),b3\n3,[a4](https://example.com/a4),b4\n"),
]
for source, read_excel_kwargs, expected in cases:
    converter = XLSXToDocument(link_format="markdown", read_excel_kwargs=read_excel_kwargs)
    actual = converter.run(sources=[source])["documents"][0].content
    print(read_excel_kwargs, "OK" if actual == expected else "MISMATCH")
    print("  expected:", repr(expected))
    print("  actual:  ", repr(actual))

Output on a6d6dce (pandas 3.0.6, openpyxl 3.1.5):

{'names': [1, 0]} MISMATCH
  expected: ',A,B\n1,[Alice](https://example.com/alice),[Bob](https://example.com/bob)\n2,Carol,Dave\n'
  actual:   ',A,B\n1,[Alice](https://example.com/bob),[Bob](https://example.com/alice)\n2,Carol,Dave\n'
{'names': ['second'], 'usecols': 'B'} MISMATCH
  expected: ',A\n1,[Bob](https://example.com/bob)\n2,Dave\n'
  actual:   ',A\n1,[Bob](https://example.com/alice)\n2,Dave\n'
{'comment': '#'} MISMATCH
  expected: ',A,B\n1,a1,b1\n2,[a3](https://example.com/a3),b3\n3,[a4](https://example.com/a4),b4\n'
  actual:   ',A,B\n1,a1,b1\n2,a3,b3\n3,[a4](https://example.com/a3),b4\n'

[HaystackBot]

Hi @Harbor404, thanks a lot for your contribution! 🙏

We noticed that the Contributor License Agreement (CLA) check (license/cla) hasn't passed yet, so we've temporarily moved this PR to draft and paused the review assignment.

To get your PR reviewed, please sign the CLA via the link in the license/cla check below (or in the CLA bot comment). As soon as the check turns green, this PR will automatically be marked ready for review again and a reviewer will be re-assigned.

[Harbor404]

Thanks for the detailed reproduction and triage, @amna-cpp! I've incorporated all three cases into this PR:

• names now keeps hyperlinks aligned even when labels reorder the displayed columns (names=[1, 0]).
• names + usecols now resolves the selected source column (names=["second"], usecols="B"), with additional coverage for named-column and multi-range usecols.
• comment now keeps row coordinates aligned, including the comment + usecols interaction where excluded comment cells affect pandas output.

The implementation delegates coordinate resolution back to pandas using a tokenized copy of the sheet, so skiprows/names/usecols/comment interactions are not reimplemented. The regressions and fix are pushed in 5534abd; your reproduction is credited in 786a151.

If you'd like to co-author the follow-up or have your local patch carried directly, please share the Co-authored-by: Name <email> identity you want used and any commit/branch. You're also very welcome to review the new tests and coordinate-probe approach here.

[CLAassistant]

All committers have signed the CLA.

[HaystackBot]

Thanks for signing the CLA, @Harbor404! 🎉 This PR is now ready for review again and the reviewer has been re-assigned.

[amna-cpp]

Thanks @Harbor404, I appreciate you incorporating the cases and crediting the reproduction! I’d be happy to collaborate on additional tests and review the updated approach. I’ll check it against my local cases and share any focused findings or changes here.

[amna-cpp]

Thanks again @Harbor404. I reviewed the coordinate-probe approach at 5534abd and tested it against my local cases using Python 3.12.14, pandas 3.0.6, and openpyxl 3.1.5.

What holds up

• The three cases from my earlier comment now pass.
• Additional cases passed for integer, list, and callable skiprows; letter, range, integer, and callable usecols; several names variants; comment combined with usecols or names; multiple sheets; both link formats; and repeated cell text with different URLs.
• Without selection options, output matched main for the typed-column, NA, dtype, and converters cases I tried. I found no output differences in those cases.
• The probe also preserves correct links in cases where my local implementation omitted them, including comment and a short-names case. I think this is the better approach.

Two regressions I found

Both arise from writing a token into every cell:

1. Merged cells: writing to a MergedCell raises AttributeError: 'MergedCell' object attribute 'value' is read-only. The exception is caught in run(), and the file is skipped. This happens with link extraction enabled, even without selection options or hyperlinks. The file converts successfully on main.
2. Formatted empty cells: cells stored without a value become non-empty in the probe. This can make pandas see additional columns and, with names, infer an index column that shifts the hyperlink mapping. The reproduction below works correctly on main.

Reproduction: expected vs. actual on 5534abd

from io import BytesIO

from openpyxl import Workbook
from openpyxl.styles import Font

from haystack.components.converters import XLSXToDocument
from haystack.dataclasses import ByteStream


def convert(workbook: Workbook, **read_excel_kwargs: object) -> str | None:
    buffer = BytesIO()
    workbook.save(buffer)
    converter = XLSXToDocument(
        link_format="markdown",
        read_excel_kwargs=read_excel_kwargs,
    )
    documents = converter.run(
        sources=[ByteStream(data=buffer.getvalue())]
    )["documents"]
    return documents[0].content if documents else None


# 1. A merged range, without selection options
workbook = Workbook()
sheet = workbook.active
sheet.append(["Team", None])
sheet.append(["Alice", "Profile"])
sheet.merge_cells("A1:B1")
sheet["B2"].hyperlink = "https://example.com/alice"

print(
    "expected:",
    repr(",A,B\n1,Team,\n2,Alice,[Profile](https://example.com/alice)\n"),
)
print("actual:  ", repr(convert(workbook)))

# 2. A formatted empty cell next to the table, combined with names
workbook = Workbook()
sheet = workbook.active
sheet.append(["Alice", "Bob"])
sheet["A1"].hyperlink = "https://example.com/alice"
sheet["B1"].hyperlink = "https://example.com/bob"
sheet["C1"].font = Font(bold=True)

print(
    "expected:",
    repr(",A,B\n1,[Alice](https://example.com/alice),[Bob](https://example.com/bob)\n"),
)
print("actual:  ", repr(convert(workbook, names=["first", "second"])))

Output on 5534abd:

expected: ',A,B\n1,Team,\n2,Alice,[Profile](https://example.com/alice)\n'
actual:   None

expected: ',A,B\n1,[Alice](https://example.com/alice),[Bob](https://example.com/bob)\n'
actual:   ',A,B\n1,[Alice](https://example.com/bob),Bob\n'

Proposed changes

I prepared two commits on [a branch based on your head](https://github.com/amna-cpp/haystack/commits/collab/pr-13059-xlsx-probe-cells):

• [6a0c961](https://github.com/amna-cpp/haystack/commit/6a0c961a9c967d9db0dde39701aafb066aa83838) — correctness fix: leave empty cells, including merged-cell placeholders, untouched in the probe. This avoids the write error and the extra-column problem. The implementation adds four lines, plus three tests that fail on 5534abd and pass with the fix.
• [e6af05d](https://github.com/amna-cpp/haystack/commit/e6af05d25713716da88e1dd92a71cc4f3e16c56b) — optional performance improvement: skip the probe when there are no hyperlinks or no position-related options. The probe adds a workbook save and a second parse; in my local benchmark on a 3,000 × 8 sheet without read_excel_kwargs, conversion took 0.76 s versus 0.46 s on main.

On that branch, all 41 XLSX tests and 325 converter tests pass. fmt-check and mypy on the two changed files also pass.

Limitation: the first commit leaves hyperlinks on cells without a value unmapped when the probe runs. The optional second commit preserves the existing behavior when no position-related options are supplied. I could not create an empty linked cell using openpyxl, so that case remains untested.

Two additional issues outside these commits

• Numeric comment markers can collide with coordinates in the tokens. In my cases, comment="0" dropped all links, while "1" and "2" dropped some. Combining comment="1" with skiprows=[0] caused a probe ParserError, and the file was skipped.
• Callable skiprows and usecols are evaluated twice. This may matter for stateful callables.

Please feel free to cherry-pick 6a0c961 for the correctness fix. e6af05d is an optional performance improvement on top.

If you incorporate the changes another way, my co-author identity is:

Co-authored-by: Amna Hameed <183917584+amna-cpp@users.noreply.github.com>

[Harbor404]

Integrated 6a0c961 and e6af05d in 386cbf6, with co-authorship credited:

• Empty and merged cells now stay out of the coordinate probe.
• The probe is skipped when no position-related read_excel_kwargs are present.
• Coordinate tokens also avoid numeric and separator comment-marker collisions.

Local verification: test_xlsx_to_document.py passed 44 tests; Ruff check and format passed.

[bogdankostic]

Thanks for working on this @Harbor404!
The approach looks good to me, but the PR is bigger than it needs to be. Almost half of the code added to xlsx.py is there for comment, which isn't part of #13041. Let's drop it and keep the rest close to the existing code. Details are in the inline comments.

[bogdankostic]

Let's drop comment from this PR. It isn't part of #13041 and is already broken on main. Passing it to the second parse is what makes _get_position_token_parts, _encode_position, _decode_position, _make_position_token, _parse_position_token and use_letters necessary, so without it all of these can go. The three comment tests can go with it. If someone needs comment together with hyperlinks, we can handle it in a follow-up.

Suggested change

	_POSITION_KWARGS = ("skiprows", "nrows", "skipfooter", "usecols", "names", "comment", "index_col")
	_POSITION_KWARGS = ("skiprows", "nrows", "skipfooter", "usecols", "names", "index_col")

[bogdankostic]

Without comment, a plain tag is enough. If this returns the hyperlinks re-keyed to their DataFrame positions instead of a separate mapping, the rest of _extract_tables barely needs to change. I tried something like this locally, and it passes all tests in this PR except test_link_extraction_with_comment, which also fails on main:

_CELL_TAG = "__haystack_cell__"

def _map_hyperlinks_to_dataframe(
    self, workbook: Any, hyperlinks_by_sheet: dict[str | int | None, dict[tuple[int, int], str]]
) -> dict[str | int | None, dict[tuple[int, int], str]]:
    for sheet_key in hyperlinks_by_sheet:
        for row in self._get_worksheet(workbook, sheet_key).iter_rows():
            for cell in row:
                # Empty cells must stay empty: pandas sizes the sheet by its non-empty cells, and the placeholder
                # cells of merged ranges are read-only.
                if cell.value not in (None, ""):
                    cell.value = f"{_CELL_TAG}{cell.row - 1},{cell.column - 1}"
    tagged_bytes = io.BytesIO()
    workbook.save(tagged_bytes)
    position_kwargs = {key: self.read_excel_kwargs[key] for key in _POSITION_KWARGS if key in self.read_excel_kwargs}
    tagged_sheets = pd.read_excel(
        io=tagged_bytes, sheet_name=self.sheet_name, header=None, engine="openpyxl", **position_kwargs
    )
    if isinstance(tagged_sheets, pd.DataFrame):
        tagged_sheets = {self.sheet_name: tagged_sheets}

    mapped_hyperlinks = {}
    for sheet_key, cell_links in hyperlinks_by_sheet.items():
        positions = {}
        for row_idx, values in enumerate(tagged_sheets[sheet_key].itertuples(index=False, name=None)):
            for col_idx, value in enumerate(values):
                if isinstance(value, str) and value.startswith(_CELL_TAG):
                    excel_row, excel_col = value.removeprefix(_CELL_TAG).split(",")
                    positions[(int(excel_row), int(excel_col))] = (row_idx, col_idx)
        mapped_hyperlinks[sheet_key] = {positions[cell]: url for cell, url in cell_links.items() if cell in positions}
    return mapped_hyperlinks

And in _extract_tables:

if any(hyperlinks_by_sheet.values()) and any(key in self.read_excel_kwargs for key in _POSITION_KWARGS):
    hyperlinks_by_sheet = self._map_hyperlinks_to_dataframe(wb, hyperlinks_by_sheet)

[bogdankostic]

Let's drop this identity mapping. If the hyperlinks are re-keyed directly, hyperlinks_by_sheet can just stay as it is when no option moves cells.

[bogdankostic]

This comment is left over from the first commit, which used pandas' integer column labels. With the hyperlinks re-keyed as in my comment on _get_cell_positions, the original loop works as it is, so let's keep it where it was and drop _apply_hyperlinks.

[bogdankostic]

Let's merge these into one parametrized test over read_excel_kwargs. A lot of the new lines are the same workbook setup, and the internal and range links don't test anything new here, since the link target doesn't affect where the link ends up. Something like:

@pytest.mark.parametrize(
    ("read_excel_kwargs", "expected_content"),
    [
        pytest.param({"skiprows": 1}, "...", id="skiprows"),
        pytest.param({"usecols": "B:C"}, "...", id="usecols"),
        pytest.param({"names": [...]}, "...", id="names"),
        pytest.param({"names": [...], "usecols": "B"}, "...", id="names-and-usecols"),
    ],
)
def test_link_extraction_with_read_excel_kwargs(
    self, tmp_path: Path, read_excel_kwargs: dict[str, Any], expected_content: str
) -> None:

The merged-cell and formatted-empty-cell tests are worth keeping as they are.

[bogdankostic]

Let's keep comment out of here (see my comment on _POSITION_KWARGS) and drop the link types, since the fix doesn't depend on them.

Suggested change

	Fixed ``XLSXToDocument`` assigning hyperlinks to the wrong cells when
	``read_excel_kwargs`` includes ``skiprows``, ``names``, ``usecols``, or
	``comment``. Hyperlinks now remain attached to the selected cells,
	including external, internal, and range links.
	Fixed ``XLSXToDocument`` attaching hyperlinks to the wrong cells when
	``read_excel_kwargs`` includes ``skiprows``, ``usecols``, or ``names``.

[HaystackBot]

Hi @Harbor404, 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 license/cla check, and this PR will automatically be marked ready for review.

[Harbor404]

@bogdankostic all six inline comments are addressed in 2a78e9f. Net -246 lines (91 insertions, 337 deletions).

• _POSITION_KWARGS: comment is gone. That was carrying _get_position_token_parts, _encode_position, _decode_position, _make_position_token, _parse_position_token and the use_letters branch, so all of them are deleted.
• _map_hyperlinks_to_dataframe (replaces _get_cell_positions): non-empty cells get a single plain _CELL_TAG placeholder, pandas resolves where they land, and the hyperlinks are re-keyed to those DataFrame positions directly — essentially your snippet, including the empty-cell check that keeps merged placeholders alone.
• Identity mapping: dropped. When no positional option is in play, hyperlinks_by_sheet is left exactly as it was read.
• Apply loop: back where it was, after the column rename, so _apply_hyperlinks is gone.
• Tests: the read_excel_kwargs cases are now one parametrized test (skiprows, usecols, names, names-and-usecols); the three comment tests and the redundant internal/range-link variants are removed. The merged-cell and formatted-empty-cell tests are untouched.
• Release note: reworded to your suggestion.

Two CI notes:

• check-imports was failing with NameError: name 'pd' is not defined — _apply_hyperlinks annotated pd.DataFrame in the class body, and pd is imported lazily. Removing that method fixes it; python .github/utils/check_imports.py now imports all 270 modules locally.
• cla-pending: the license/cla status is green on every commit authored by me, but it is absent on the merge commit e3a16ef that was created when the branch was updated from main, which is what the draft gate keyed on. It should re-evaluate against the new head.

Local: test/components/converters/test_xlsx_to_document.py → 36 passed; ruff check + ruff format --check clean.

[HaystackBot]

Thanks for signing the CLA, @Harbor404! 🎉 This PR is now ready for review again and the reviewer has been re-assigned.

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
haystack/components/converters
xlsx.py					170, 172, 195
haystack/core/pipeline
pipeline.py
Project Total

This report was generated by python-coverage-comment-action