Skip to content

fix(file-based): support multi-sheet Excel workbooks via sheet_name config - #1051

Draft
devin-ai-integration[bot] wants to merge 6 commits into
mainfrom
devin/1781264638-fix-excel-multisheet
Draft

devin-ai-integration[bot] wants to merge 6 commits into
mainfrom
devin/1781264638-fix-excel-multisheet

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

The file-based CDK Excel parser previously relied on the pandas default sheet_name=0, so only the first worksheet was ever read from multi-sheet workbooks. This caused silent data loss for connectors like source-microsoft-sharepoint when users had Excel files with multiple sheets.

This PR adds an optional sheet_name field to ExcelFormat:

class ExcelFormat(BaseModel):
    sheet_name: str = Field(default="0", ...)

Accepted values: "0" (default, first sheet — preserves existing behavior), a sheet name string, a zero-indexed position, or "*" to read all sheets.

The new _parse_excel_file → _resolve_sheet_name path converts the string config into a pandas-compatible sheet_name argument (int, str, or None). When sheet_name=None (all sheets), pandas returns a dict[str, DataFrame] instead of a single DataFrame; the parser iterates over all returned DataFrames for both record emission and schema inference (merging columns across sheets).

Both Calamine-first and OpenPyXL fallback paths now accept and forward the sheet_name parameter.

Resolves https://github.com/airbytehq/oncall/issues/12598:

Supersedes #1024 (stale draft from a prior session).

Declarative-First Evaluation

The affected connectors (source-microsoft-sharepoint, etc.) are Python file-based CDK sources, not manifest-only connectors. The fix operates at the Excel parsing layer before any records exist, so declarative features (RecordFilter, AddFields, RemoveFields, paginators, routers, error handlers, $ref overrides) cannot address this.

Breaking Change Evaluation

Not a breaking change. The new sheet_name field is optional and defaults to "0", preserving the previous first-sheet-only behavior. No streams, fields, primary keys, cursors, existing config fields, or state formats are removed or changed.

Test Coverage

New tests added to test_excel_parser.py:

  • test_parse_records_selects_configured_sheet — parametrized for default first sheet, named sheet, and all sheets
  • test_infer_schema_with_sheet_selection — parametrized for single-sheet and merged multi-sheet schema inference
  • test_resolve_sheet_name — parametrized unit test for the config → pandas argument conversion

All tests use real in-memory multi-sheet Excel workbooks (via xlsxwriter) instead of mocks.

Link to Devin session: https://app.devin.ai/sessions/6a77303e47e040da803aeee819febb5b

…onfig

Add an optional sheet_name field to ExcelFormat config that controls
which worksheet(s) the parser reads:
- '0' (default): first sheet only (preserves existing behavior)
- '<name>': a specific sheet by name
- '*': all sheets in the workbook

Both Calamine and OpenPyXL fallback paths now pass sheet_name through
to pandas. Schema inference merges columns across selected sheets.

Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

Copy link
Copy Markdown

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

Testing This CDK Version

You can test this version of the CDK using the following:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1781264638-fix-excel-multisheet#egg=airbyte-python-cdk[dev]' --help

# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1781264638-fix-excel-multisheet

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment
📚 Show Repo Guidance

Helpful Resources

📝 Edit this welcome message.

@github-actions

github-actions Bot commented Jun 12, 2026 •

Copy link
Copy Markdown

PyTest Results (Fast)

4 183 tests  +25   4 171 ✅ +25   7m 54s ⏱️ +7s
    1 suites ± 0      12 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit cb2d66c. ± Comparison against base commit e79d50e.

♻️ This comment has been updated with latest results.

Co-Authored-By: bot_apk <apk@cognition.ai>
@github-actions

github-actions Bot commented Jun 12, 2026 •

Copy link
Copy Markdown

PyTest Results (Full)

4 186 tests   4 174 ✅  12m 19s ⏱️
    1 suites     12 💤
    1 files        0 ❌

Results for commit cb2d66c.

♻️ This comment has been updated with latest results.

@pnilan

Copy link
Copy Markdown
Collaborator

🤖 This comment was generated by an AI Agent.

Review: sheet_name config for multi-sheet Excel workbooks

The diagnosis here is correct and the approach is the right one. I verified the root cause independently on current main: both parse paths call pd.ExcelFile(...).parse() with no sheet_name, pandas defaults to sheet_name=0, and every worksheet after the first is silently dropped. Against a three-sheet workbook on unmodified main:

Workbook sheets: ['Sales', 'Inventory', 'Adjustments']
records emitted: 2                                       # Sales only
schema inferred: {region, revenue}                       # Sales only

Good news on staleness: nothing has touched excel_parser.py, excel_format.py, or their tests on main since this PR was opened on 2026-06-12, so it rebases clean. The one red check (Check: source-google-drive) is a pre-existing harness bug — SourceGoogleDrive.__init__() missing 3 required positional arguments — not caused by this diff. Don't let it block you.

Three things need to change before this merges, and there's downstream work the PR description doesn't mention that is required before the originating issue is actually fixed. I checked out this branch and implemented the changes below; everything I'm prescribing is verified — ruff format, ruff check, mypy, and 796 passing tests in unit_tests/sources/file_based/.


1. Blocking — worksheets with numeric names are unreachable

_resolve_sheet_name coerces any decimal-looking string to an integer, which pandas interprets as a positional index rather than a name. Worksheets named for fiscal years, quarters, or months are extremely common, and this makes all of them unaddressable. Reproduced on this branch:

workbook sheets: ['2026', '2025']
sheet_name='2026' -> RecordParseError: Error parsing record. This could be due to a mismatch
                     between the config's file type and the actual file type...

The user asked for the worksheet named 2026 and got worksheet index 2026, which doesn't exist.

The root problem is one string field carrying three meanings (index, name, wildcard). Index-based selection isn't in the originating request and isn't worth the collision — drop it. Use unset for "first worksheet", which also expresses the pre-existing default more honestly than the magic string "0":

sheet_name: Optional[str] = Field(
    None,
    title="Sheet Name",
    description=(
        "The worksheet to read from each workbook. Leave empty to read only the first "
        'worksheet. Enter an exact worksheet name to read that worksheet, or "*" to read '
        "every worksheet in the workbook. Worksheet names are case-sensitive."
    ),
    examples=["Sheet1", "*"],
)
def _resolve_sheet_name(self, excel_format: ExcelFormat) -> Union[int, str, None]:
    """Converts the config value to a pandas-compatible `sheet_name` argument.

    Unset means the first worksheet, which is the behavior that predates this option.
    `*` means every worksheet; pandas spells that `None`. Any other value is used as a
    literal worksheet name -- notably, a numeric-looking name like "2026" stays a name
    rather than becoming a positional index.
    """
    value = excel_format.sheet_name
    if value is None:
        return 0
    if value == self.ALL_SHEETS:
        return None
    return value

"*" is safe as a sentinel with zero collision risk — Excel rejects * [ ] : ? / \ in worksheet names, so no real workbook can contain a sheet named *. Verified:

sheet named * REJECTED: InvalidWorksheetName Invalid Excel character '[]:*?/\' in sheetname '*'

Making sheet_name optional also improves the spec: the field lands with no default key and stays out of required, so the change is purely additive.

2. Blocking — "*" emits no worksheet provenance

With sheet_name="*" every worksheet flattens into one stream and nothing records where a row came from:

{'region': 'West', 'revenue': 100}
{'region': 'East', 'revenue': 200}
{'sku': 'A1', 'qty': 5}

A consumer can only guess the origin from which columns happen to be non-null, and any two worksheets sharing a column layout become genuinely indistinguishable. That also makes primary keys unsafe: identical rows on different worksheets collide. Since "*" is brand-new, nobody has an existing schema for it, so emitting a provenance field is not a breaking change and makes the mode substantially more useful.

Emit _ab_source_sheet_name, matching the existing _ab_source_file_url / _ab_source_file_last_modified convention. Add the class attribute:

class ExcelParser(FileTypeParser):
    ENCODING = None
    ALL_SHEETS = "*"
    ab_sheet_name_col = "_ab_source_sheet_name"

Iterate .items() instead of .values() in parse_records so the worksheet name is in scope:

        include_sheet_name = self._reads_all_sheets(excel_format)

        try:
            with stream_reader.open_file(file, self.file_read_mode, self.ENCODING, logger) as fp:
                for sheet, df in self._parse_excel_file(fp, excel_format, logger, file).items():
                    # DataFrame.to_dict() returns pandas.Timestamp values not serializable by orjson.
                    # DataFrame.to_json() serializes datetimes to iso8601 with microseconds.
                    records = orjson.loads(
                        df.to_json(orient="records", date_format="iso", date_unit="us")
                    )
                    if include_sheet_name:
                        for record in records:
                            record[self.ab_sheet_name_col] = sheet
                    yield from records

Declare it in infer_schema, right after the column-merging loop:

        if self._reads_all_sheets(excel_format):
            fields[self.ab_sheet_name_col] = "string"
def _reads_all_sheets(self, excel_format: ExcelFormat) -> bool:
    """Whether the config selects every worksheet in the workbook."""
    return excel_format.sheet_name == self.ALL_SHEETS

Keep this scoped to "*". When a user selects a single named worksheet there's no ambiguity to resolve, and adding the field there would perturb schemas for no benefit.

I confirmed this reaches the destination rather than getting filtered: DefaultFileBasedStream.get_json_schema merges the parser-inferred schema with the stream's own _ab_source_* columns, so a parser-emitted field flows through both discover and read.

3. Blocking — a mistyped worksheet name reports the wrong problem

A missing worksheet raises ValueError from pandas, which the broad except Exception converts into RecordParseError:

RecordParseError: Error parsing record. This could be due to a mismatch between the config's
file type and the actual file type, or because the file or record is not parseable.

That sends the user to check whether their .xlsx is really an .xlsx, when they actually made a one-character config typo. Worksheet names are case-sensitive, so sales vs Sales lands here too — an easy mistake with no hint that case is the issue.

Catch it in _parse_excel_file and raise a config error that names the bad worksheet and lists the real ones:

def _parse_excel_file(
    self,
    fp: Union[IOBase, str, Path],
    excel_format: ExcelFormat,
    logger: logging.Logger,
    file: RemoteFile,
) -> Dict[Union[int, str], pd.DataFrame]:
    """Parses an Excel file and returns a mapping of worksheet name to DataFrame.

    When reading every worksheet, pandas keys the mapping by the real worksheet names in
    workbook order. When reading a single worksheet it returns a bare DataFrame, which we
    wrap so callers always see the same shape.
    """
    sheet_name = self._resolve_sheet_name(excel_format)
    try:
        parsed = self.open_and_parse_file(fp, logger, file, sheet_name)
    except ValueError as exc:
        # pandas raises ValueError("Worksheet named 'x' not found") for both engines.
        if "not found" not in str(exc):
            raise
        raise ConfigValidationError(
            f"Worksheet {excel_format.sheet_name!r} was not found in the workbook. "
            f"{self._describe_available_sheets(fp, logger)}"
            "Worksheet names are case-sensitive. "
            f'Set the "Sheet Name" option to an exact worksheet name, or to "*" to read '
            "every worksheet.",
            filename=file.uri,
        ) from exc
    if isinstance(parsed, pd.DataFrame):
        return {sheet_name if isinstance(sheet_name, str) else 0: parsed}
    return parsed

@staticmethod
def _describe_available_sheets(fp: Union[IOBase, str, Path], logger: logging.Logger) -> str:
    """Best-effort listing of the worksheets present, for the not-found error message."""
    try:
        if hasattr(fp, "seek"):
            fp.seek(0)  # type: ignore [union-attr]
        names = pd.ExcelFile(fp, engine="calamine").sheet_names  # type: ignore [arg-type]
    except BaseException as exc:
        logger.info(f"Could not list worksheets for the error message: {exc}")
        return ""
    return f"Available worksheets: {', '.join(repr(str(n)) for n in names)}. "

ConfigValidationError must be re-raised ahead of the catch-all in parse_records, otherwise it gets swallowed back into RecordParseError:

        except ConfigValidationError:
            # A missing worksheet is a configuration problem, not an unparseable record.
            raise
        except Exception as exc:
            raise RecordParseError(
                FileBasedSourceError.ERROR_PARSING_RECORD, filename=file.uri
            ) from exc

Resulting message:

Worksheet 'Sales2026' was not found in the workbook. Available worksheets: 'Sales', 'Inventory'.
Worksheet names are case-sensitive. Set the "Sheet Name" option to an exact worksheet name, or
to "*" to read every worksheet.

I verified a genuinely corrupt file still raises RecordParseError — the narrower catch doesn't mask real parse failures.

4. Non-blocking — _parse_excel_file keys the single-sheet case by the raw config string

return {excel_format.sheet_name: parsed} uses the config value rather than the resolved worksheet name. Harmless while only .values() is consumed, but wrong the moment provenance depends on that key. The rewrite in §3 fixes it.


Test updates

Running the suite with these changes, exactly six tests fail, all added by this PR, all encoding behavior the review asks you to change. Every pre-existing test passes, and the full file-based suite is 796 passed / 7 failed with no unrelated regressions.

Test Why it fails Fix
test_resolve_sheet_name[zero_index] "0" is now a worksheet name Change to None → 0
test_resolve_sheet_name[numeric_index] index selection removed by design Delete; replace with a case asserting "2026" → "2026"
test_parse_records_selects_configured_sheet[default_first_sheet] passes sheet_name="0" Omit sheet_name entirely
test_parse_records_selects_configured_sheet[all_sheets] records now carry provenance Add _ab_source_sheet_name to expectations
test_infer_schema_with_sheet_selection[first_sheet_schema] passes sheet_name="0" Omit sheet_name entirely
test_infer_schema_with_sheet_selection[all_sheets_merged_schema] schema now declares provenance Add _ab_source_sheet_name: {"type": "string"}

The seventh failure is test_file_based_spec[single_csv_scenario] — the spec snapshot in csv_scenarios.py needs re-updating for the new field definition. It should become:

"sheet_name": {
    "title": "Sheet Name",
    "description": "The worksheet to read from each workbook. Leave empty to read only the first worksheet. Enter an exact worksheet name to read that worksheet, or \"*\" to read every worksheet in the workbook. Worksheet names are case-sensitive.",
    "examples": ["Sheet1", "*"],
    "type": "string",
},

Note there is no "default" key now, and sheet_name stays out of required.

Please also add coverage for the cases this review is built on, none of which exist today:

  • a worksheet whose name is numeric ("2026") resolves by name, not position
  • "*" tags each record with its originating worksheet
  • "*" preserves workbook order deterministically
  • a missing worksheet raises ConfigValidationError listing the available worksheets
  • a case-mismatched worksheet name ("sales" vs "Sales") hits the same path
  • a corrupt file still raises RecordParseError
  • unset sheet_name reproduces first-worksheet behavior exactly, including no provenance field

Downstream work this PR does not cover

The PR description says connectors "will need a CDK version bump to pick up this fix." That understates it, and as written the originating issue stays open even after this merges. ExcelFormat is spec-facing and shared, so seven connectors inherit the new field — source-s3, source-gcs, source-azure-blob-storage, source-google-drive, source-sftp-bulk, source-microsoft-sharepoint, and source-sharepoint-enterprise (the last in airbyte-enterprise).

Commit 57ca4e19 updates only the CDK-internal snapshot. Each connector carries its own recorded spec that still shows Excel Format with filetype as its sole property, and those live in different repos entirely — they will not be fixed by anything in this PR. For the connector that prompted the issue, the follow-up is: poetry update airbyte-cdk (its constraint >=7.11.1,<8 already permits the fix, so no constraint edit), regenerate integration_tests/spec.json, bump the version, and update the docs page. The other six each need their own spec regeneration whenever they next bump the CDK — worth flagging to their owners so it doesn't surface as a mystery acceptance-test failure.

Please add a release note or docs update covering the new option, including the case-sensitivity rule and the _ab_source_sheet_name field.

One design point worth stating explicitly

The PR frames "*" as the multi-sheet answer, but the more useful capability is the one it enables incidentally: named sheet_name plus several stream configs over the same glob gives one stream per worksheet. A stream sales with glob **/*.xlsx and sheet_name: Sales, alongside inventory with the same glob and sheet_name: Inventory, produces cleanly separated streams with independent schemas.

For workbooks whose worksheets hold unrelated data — the common case, and the shape described in the originating issue — that's a much better outcome than "*" flattening everything into one wide sparse table. It needs no architectural change and is worth calling out in the docs as the recommended pattern, with "*" positioned as the option for workbooks that are one dataset split across worksheets. Whether provenance-on-"*" is must-have or merely nice-to-have depends on which shape users actually want, but §2 should ship regardless, because "*" is not safely usable without it.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

❌ Cannot revive Devin session - the session is too old. Please start a new session instead.

Resolves three defects in the sheet_name implementation.

Worksheets with numeric names were unreachable. The config value was
coerced to an int when it looked decimal, so pandas read it as a
positional index -- a worksheet named "2026" resolved to sheet index
2026 and failed. Numeric worksheet names are common (fiscal years,
quarters, months). Positional selection is dropped entirely and
sheet_name is now Optional, where unset means the first worksheet.
This also expresses the pre-existing default more honestly than the
magic string "0".

Reading all worksheets carried no provenance. With sheet_name="*" every
worksheet flattened into one stream with nothing recording the origin of
a row, making worksheets that share a column layout indistinguishable
and primary keys unsafe across them. Records now carry
_ab_source_sheet_name, matching the existing _ab_source_file_url and
_ab_source_file_last_modified convention, and infer_schema declares it.
This is scoped to "*" only; selecting a single named worksheet has no
ambiguity to resolve.

A mistyped worksheet name reported the wrong problem. The broad
exception handler turned it into ERROR_PARSING_RECORD, telling users to
check whether their file type matched when they had made a config typo.
Worksheet names are case-sensitive, so "sales" vs "Sales" landed here
too. A missing worksheet now raises ConfigValidationError naming the bad
worksheet and listing the ones actually present.

Also keys the single-worksheet mapping by the resolved name rather than
the raw config string, which matters now that provenance depends on it.
@pnilan

Copy link
Copy Markdown
Collaborator

🤖 This comment was generated by an AI Agent.

Pushed fda9d11d implementing everything from the review above, since the Devin session that opened this PR is no longer active.

Applied:

  • §1 — dropped the isdecimal() → int() coercion; sheet_name is now Optional[str] where unset means the first worksheet. A worksheet named 2026 resolves by name.
  • §2 — records read with "*" now carry _ab_source_sheet_name, and infer_schema declares it. Scoped to "*" only.
  • §3 — a missing or case-mismatched worksheet raises ConfigValidationError naming the bad worksheet and listing the real ones, instead of the misleading ERROR_PARSING_RECORD.
  • §4 — single-worksheet mapping is keyed by the resolved name, not the raw config string.

Tests — all six stale expectations updated and the spec snapshot in csv_scenarios.py corrected. Added the coverage the review asked for: numeric worksheet names, provenance tagging, deterministic workbook ordering, missing worksheet, case mismatch, corrupt file still raising RecordParseError, and unset sheet_name adding no provenance field.

Verification — 811 passed, 1 skipped in unit_tests/sources/file_based/, plus ruff format --check, ruff check, and mypy all clean.

Two things still open, neither of which belongs in this PR:

  1. The seven connector spec snapshots in airbyte and airbyte-enterprise still describe Excel Format with filetype as its only property. They live in other repos and need their own PRs.
  2. Docs for the new option — the case-sensitivity rule, the _ab_source_sheet_name field, and the one-stream-per-worksheet pattern described in the review.

Ready for review — the PR is still in draft.

Comment thread airbyte_cdk/sources/file_based/file_types/excel_parser.py Fixed
…eption''

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@pnilan
Patrick Nilan (pnilan) marked this pull request as ready for review July 31, 2026 20:14
@pnilan
Patrick Nilan (pnilan) requested a review from a team as a code owner July 31, 2026 20:14
Copilot AI review requested due to automatic review settings July 31, 2026 20:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the file-based CDK’s Excel parsing layer to support selecting a specific worksheet (or all worksheets) from multi-sheet Excel workbooks, avoiding silent data loss when only the first sheet is read.

Changes:

  • Add an optional sheet_name configuration field to the Excel format.
  • Plumb sheet_name through both Calamine-first parsing and OpenPyXL fallback, including multi-sheet iteration and merged schema inference.
  • Add unit tests covering sheet selection, all-sheets parsing (with provenance tagging), and schema inference across sheets.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
airbyte_cdk/sources/file_based/config/excel_format.py Adds sheet_name option to the Excel format config.
airbyte_cdk/sources/file_based/file_types/excel_parser.py Implements sheet selection / all-sheets parsing, forwards sheet_name to pandas engines, and merges schema across sheets when needed.
unit_tests/sources/file_based/file_types/test_excel_parser.py Adds regression and unit tests for sheet selection behavior, all-sheets provenance tagging, schema inference, and missing-sheet error handling.
unit_tests/sources/file_based/scenarios/csv_scenarios.py Updates expected spec scenario to include the new sheet_name property.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +21 to +30
sheet_name: Optional[str] = Field(
None,
title="Sheet Name",
description=(
"The worksheet to read from each workbook. Leave empty to read only the first "
'worksheet. Enter an exact worksheet name to read that worksheet, or "*" to read '
"every worksheet in the workbook. Worksheet names are case-sensitive."
),
examples=["Sheet1", "*"],
)
Comment on lines +328 to +341
def _resolve_sheet_name(self, excel_format: ExcelFormat) -> Union[int, str, None]:
"""Converts the config value to a pandas-compatible `sheet_name` argument.

Unset means the first worksheet, which is the behavior that predates this option.
`*` means every worksheet; pandas spells that `None`. Any other value is used as a
literal worksheet name -- notably, a numeric-looking name like "2026" stays a name
rather than becoming a positional index.
"""
value = excel_format.sheet_name
if value is None:
return 0
if value == self.ALL_SHEETS:
return None
return value
@pnilan

Patrick Nilan (pnilan) commented Jul 31, 2026 •

Copy link
Copy Markdown
Collaborator

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/30664038147

@pnilan

Patrick Nilan (pnilan) commented Jul 31, 2026 •

Copy link
Copy Markdown
Collaborator

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/30668220049

…o end

Reading every worksheet asked pandas for all of them at once
(sheet_name=None), which builds one frame per worksheet before a single
record is emitted. Peak memory therefore tracked the whole workbook
rather than its largest sheet, and grew with every worksheet added.

_parse_excel_file becomes _iter_worksheets, a generator that holds the
workbook handle open and parses one worksheet at a time. parse_records
already streams, so each frame is collectible once its records are out.
Measured through the parser on 20k-row worksheets, peak memory is flat
at ~14.6 MB regardless of worksheet count, against 19.6 MB at four
worksheets and 65.3 MB at thirty-two. The crossover is around three:
below that, holding the workbook handle open costs slightly more than
the frames it saves. Bounding the worst case is the point -- the
failure mode worth designing against is the large workbook, not the
two-sheet one.

The engine fallback now only covers the first worksheet. Once an
earlier worksheet's records are downstream, re-reading the workbook
with openpyxl would emit them twice, so a later failure is left to
propagate. The stream logs it and retries the file on the next sync,
which is how every other streaming parser already behaves on a mid-file
error.

Tests: two parser tests assert laziness directly by spying on the parse
calls, which is deterministic where a memory threshold would be flaky.
The in-memory scenario file source now accepts a mapping of worksheet
name to rows, so three new scenarios run multi-worksheet workbooks
through the full stack -- spec, discover and read via the real
DefaultFileBasedStream -- covering unset, a named worksheet, and "*".
That closes the gap between the parser tests and a live connection,
notably confirming _ab_source_sheet_name survives schema validation and
the record transform.
@pnilan
Patrick Nilan (pnilan) marked this pull request as draft August 2, 2026 23:04

This branch was previously deployed

2 inactive (outdated) deployments
DockerHub — e0b15ff9 Deployed Jul 31, 2026 by octavia-bot[bot] via Publish SDM to DockerHub #261
PyPi — e0b15ff9 Deployed Jul 31, 2026 by octavia-bot[bot] via Publish CDK version to PyPI #261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants