Repository navigation
fix(file-based): support multi-sheet Excel workbooks via sheet_name config - #1051
devin-ai-integration[bot] wants to merge 6 commits into
Conversation
…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 EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou 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-multisheetPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Co-Authored-By: bot_apk <apk@cognition.ai>
PyTest Results (Full)4 186 tests 4 174 ✅ 12m 19s ⏱️ Results for commit cb2d66c. ♻️ This comment has been updated with latest results. |
Review:
|
| 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
ConfigValidationErrorlisting the available worksheets - a case-mismatched worksheet name (
"sales"vs"Sales") hits the same path - a corrupt file still raises
RecordParseError - unset
sheet_namereproduces 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.
|
❌ 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.
Pushed Applied:
Tests — all six stale expectations updated and the spec snapshot in Verification — Two things still open, neither of which belongs in this PR:
Ready for review — the PR is still in draft. |
…eption'' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
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_nameconfiguration field to the Excel format. - Plumb
sheet_namethrough 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.
| 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 |
|
/prerelease
|
|
/prerelease
|
…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.
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_namefield toExcelFormat: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_namepath converts the string config into a pandas-compatiblesheet_nameargument (int,str, orNone). Whensheet_name=None(all sheets), pandas returns adict[str, DataFrame]instead of a singleDataFrame; 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_nameparameter.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,$refoverrides) cannot address this.Breaking Change Evaluation
Not a breaking change. The new
sheet_namefield 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 sheetstest_infer_schema_with_sheet_selection— parametrized for single-sheet and merged multi-sheet schema inferencetest_resolve_sheet_name— parametrized unit test for the config → pandas argument conversionAll tests use real in-memory multi-sheet Excel workbooks (via xlsxwriter) instead of mocks.
Link to Devin session: https://app.devin.ai/sessions/6a77303e47e040da803aeee819febb5b