Conversation
…olumns Add SpiralDBTableSource — a read-only RootSource backed by a SpiralDB table. PK (key-schema) columns are used as tag columns by default; explicit tag_columns override this at construction time. Tables with no key schema and no explicit tag_columns raise ValueError. The class follows the same pattern as SQLiteTableSource: it opens a SpiralDBConnector, delegates to DBTableSource for fetching and stream building, then closes the connector immediately (eager load). Config round-trip (to_config / from_config) and serialization registration under the "spiraldb_table" key are included. 50 unit and integration tests added, all passing. Closes PLT-1073 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds first-class support for reading SpiralDB tables as OrcaPod sources, integrating the new source into the existing source ecosystem and pipeline serialization.
Changes:
- Introduces
SpiralDBTableSource, a read-onlyDBTableSourcewrapper that opens aSpiralDBConnector, eagerly loads a table, and closes the connector immediately. - Registers the new source type as
"spiraldb_table"in the pipeline source registry and exports it fromorcapod.core.sources(and thusorcapod.sources). - Adds a comprehensive new test suite covering schema/tag behavior, error cases, hashing, config round-trip, and pipeline integration.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/orcapod/core/sources/spiraldb_table_source.py |
New SpiralDB-backed table source with config serialization and connector lifecycle handling. |
src/orcapod/core/sources/__init__.py |
Exports SpiralDBTableSource via module imports and __all__. |
src/orcapod/pipeline/serialization.py |
Registers "spiraldb_table" in SOURCE_REGISTRY so pipelines can deserialize it. |
tests/test_core/sources/test_spiraldb_table_source.py |
New test coverage validating behavior and integration (mocked connector). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def to_config(self) -> dict[str, Any]: | ||
| """Serialize source configuration to a JSON-compatible dict.""" | ||
| base = super().to_config() | ||
| base.pop("connector", None) | ||
| return { | ||
| **base, | ||
| "source_type": "spiraldb_table", | ||
| "project_id": self._project_id, | ||
| "dataset": self._dataset, | ||
| "overrides": self._overrides, | ||
| } |
There was a problem hiding this comment.
SpiralDBTableSource.to_config() currently calls super().to_config(), which invokes DBTableSource.to_config() and therefore calls self._connector.to_config(). SpiralDBConnector.to_config() raises RuntimeError after the connector is closed, but SpiralDBTableSource closes the connector in init. This makes to_config (and thus pipeline serialization) fail for real SpiralDBTableSource instances. Fix by overriding to_config to build the dict without calling DBTableSource.to_config()/connector.to_config(), or capture the connector config while still open and reuse it without requiring an open connector.
There was a problem hiding this comment.
Fixed. SpiralDBTableSource.to_config() no longer calls super().to_config() (and thus no longer calls the closed connector). The method now builds the dict directly from the already-captured instance attributes (self._table_name, self._tag_columns, etc.) plus self._identity_config(), bypassing the connector entirely. See spiraldb_table_source.py for the updated docnote explaining the reasoning.
| class TestConfigSerialization: | ||
| def test_to_config_source_type(self): | ||
| from orcapod.core.sources import SpiralDBTableSource | ||
|
|
||
| connector = _make_mock_connector() | ||
| with _patch_connector(connector): | ||
| src = SpiralDBTableSource(_PROJECT_ID, _TABLE_NAME) | ||
| assert src.to_config()["source_type"] == "spiraldb_table" | ||
|
|
||
| def test_to_config_has_no_connector_key(self): | ||
| from orcapod.core.sources import SpiralDBTableSource | ||
|
|
||
| connector = _make_mock_connector() | ||
| with _patch_connector(connector): | ||
| src = SpiralDBTableSource(_PROJECT_ID, _TABLE_NAME) | ||
| assert "connector" not in src.to_config() | ||
|
|
There was a problem hiding this comment.
The to_config tests currently use a plain MagicMock connector, so they won't catch that the real SpiralDBConnector.to_config() raises once the connector is closed (and SpiralDBTableSource closes it during init). Consider using a small fake connector (or configuring the mock) where close() flips a flag and to_config() raises when closed, and assert SpiralDBTableSource.to_config() still works and does not call connector.to_config().
There was a problem hiding this comment.
Fixed. _make_mock_connector() now simulates the real SpiralDBConnector lifecycle: close() flips a _closed flag and to_config() raises RuntimeError when closed. Added test_to_config_works_after_connector_is_closed as an explicit regression guard — it asserts the connector is closed after __init__ and then calls to_config() to verify it does not raise.
SpiralDBTableSource.to_config() was delegating to super().to_config() (DBTableSource), which called self._connector.to_config(). Because SpiralDBConnector.to_config() raises RuntimeError once closed, and the connector is always closed in __init__ after the eager data load, every call to to_config() on a real SpiralDBTableSource instance would raise. Fix: override to_config() to build the dict directly from the already- captured instance attributes, bypassing the closed connector entirely. Also update _make_mock_connector() in the test suite so close() flips a flag and to_config() raises RuntimeError when closed, matching real SpiralDBConnector behaviour. Add test_to_config_works_after_connector_is_closed as an explicit regression guard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review round 1 — changes madeTwo issues addressed in commit Bug fix —
Test — lifecycle-aware mock Updated All 51 tests pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
SpiralDBTableSource, a read-onlyRootSourcebacked by a SpiralDB tabletag_columnsoverride at construction timetag_columnsraiseValueError(no implicit ROWID fallback — SpiralDB has no equivalent)to_config/from_config) supported; registered as"spiraldb_table"in the pipeline serialization registrySQLiteTableSourcepattern: opensSpiralDBConnector, delegates toDBTableSource, closes connector immediately after eager loadFiles changed
src/orcapod/core/sources/spiraldb_table_source.pySpiralDBTableSourceclasssrc/orcapod/core/sources/__init__.py__all__entrysrc/orcapod/pipeline/serialization.py"spiraldb_table"in source registrytests/test_core/sources/test_spiraldb_table_source.pyTest plan
Closes PLT-1073
🤖 Generated with Claude Code