Skip to content

feat(spiraldb): preserve Arrow table- and column-level metadata via native KV store (ITL-471)#217

Merged
eywalker merged 20 commits into
mainfrom
eywalker/itl-471-spiraldbconnector-preserve-arrow-table-and-column-level
Jul 8, 2026
Merged

feat(spiraldb): preserve Arrow table- and column-level metadata via native KV store (ITL-471)#217
eywalker merged 20 commits into
mainfrom
eywalker/itl-471-spiraldbconnector-preserve-arrow-table-and-column-level

Conversation

@kurodo3

@kurodo3 kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Arrow metadata preservation in SpiralDB: SpiralDBConnector now serializes schema-level (schema.metadata) and per-field (field.metadata) Arrow metadata into SpiralDB's native KV store (Table.set_metadata / Table.get_metadata) on write, and restores it on read. Nested struct/list field metadata is handled recursively.
  • Per-connector validate_records hook: Replaces the blanket extension-type guard in ConnectorArrowDatabase.add_records with a validate_records delegation call. SpiralDBConnector overrides it as a no-op (metadata preserved); SQLiteConnector and PostgreSQLConnector retain the rejecting behavior.
  • Full round-trip support: Arrow extension types (metadata-only representation: ARROW:extension:name / ARROW:extension:metadata in field metadata) now survive a full write→read cycle through SpiralDBConnector.

Implementation

Four private helpers in spiraldb_connector.py:

  • _serialize_field_meta_tree / _serialize_arrow_metadata — recursive serialization to a single "__arrow_metadata__" JSON blob in the KV store
  • _load_arrow_metadata / _restore_field — recursive deserialization, reattaching metadata bottom-up through struct/list type trees

Single blob encoding (base64 k/v pairs under a reserved key) avoids KV namespace collisions and is backward compatible — tables written before this change return (None, {}) from _load_arrow_metadata and behave identically to before.

Files changed

File Change
src/orcapod/databases/spiraldb_connector.py Four helpers; modified upsert_records (write metadata after write) and iter_batches (load + restore metadata); no-op validate_records
src/orcapod/protocols/db_connector_protocol.py Abstract validate_records method
src/orcapod/databases/sqlite_connector.py Rejecting validate_records
src/orcapod/databases/postgresql_connector.py Rejecting validate_records
src/orcapod/databases/connector_arrow_database.py Replace inline guard with self._connector.validate_records(records)
tests/test_databases/test_spiraldb_metadata_helpers.py 34 pure-function tests for the 4 helpers
tests/test_databases/test_spiraldb_connector.py TestValidateRecords, TestUpsertRecordsMetadata, TestIterBatchesMetadata
tests/test_databases/test_connector_arrow_database.py MockDBConnector.validate_records; test_permissive_connector_allows_extension_types
tests/test_databases/test_spiraldb_connector_integration.py TestArrowMetadataRoundTrip (6 tests, gated on SPIRAL_INTEGRATION_TESTS=1)
tests/test_core/sources/test_db_table_source.py MockDBConnector.validate_records (protocol conformance fix)
DESIGN_ISSUES.md Updated CA1; added CA2 (extension-detection duplication)

Test plan

  • All 4232 unit tests pass (uv run pytest tests/ --ignore=tests/test_databases/test_spiraldb_connector_integration.py)
  • 9 integration tests skipped without SPIRAL_INTEGRATION_TESTS=1
  • Run integration tests with SPIRAL_INTEGRATION_TESTS=1 against dev project to validate full round-trips

Fixes ITL-471

🤖 Generated with Claude Code

kurodo3 Bot and others added 15 commits July 8, 2026 01:58
…etadata helpers (ITL-471)

Implement pure serialization helpers that encode Arrow table- and
field-level metadata (including nested struct/list fields) into a
JSON blob stored under __arrow_metadata__ in the SpiralDB KV store.
Add stub placeholders for _load_arrow_metadata and _restore_field
(to be implemented in Task 3) so test imports resolve cleanly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_tree (ITL-471)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pers (ITL-471)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…servation test (ITL-471)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ectorArrowDatabase guard (ITL-471)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… (ITL-471)

Calls _serialize_arrow_metadata(records) and tbl.set_metadata(meta_kv) after
each tbl.write() in upsert_records; skips set_metadata when meta_kv is None
or when no novel rows were written (skip_existing path).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_restore_field (ITL-471)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-471)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and extension-detection duplication (ITL-471)
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.04762% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/orcapod/databases/postgresql_connector.py 50.00% 10 Missing ⚠️

📢 Thoughts on this report? Let us know!

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 adds Arrow schema- and field-metadata round-tripping support to the SpiralDB backend by serializing metadata into SpiralDB’s native table KV store on write and restoring it on read, enabling Arrow “metadata-only” extension types to survive a full write→read cycle. It also replaces ConnectorArrowDatabase’s hard-coded extension-type rejection with a per-connector validate_records hook so connectors can opt into permissive behavior when they preserve metadata.

Changes:

  • Persist Arrow schema.metadata and field.metadata (recursively for struct/list) in SpiralDB table KV metadata and restore it in iter_batches.
  • Introduce validate_records on the connector protocol and delegate validation from ConnectorArrowDatabase to the connector implementation.
  • Add unit + integration tests covering helper serialization/deserialization, connector read/write behavior, and end-to-end round trips (gated).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_databases/test_spiraldb_metadata_helpers.py New pure-function unit tests for metadata helper functions.
tests/test_databases/test_spiraldb_connector.py Adds tests for validate_records no-op and metadata write/read behavior.
tests/test_databases/test_spiraldb_connector_integration.py Adds gated integration tests validating metadata round trips.
tests/test_databases/test_connector_arrow_database.py Updates mock connector + tests for delegated validate_records.
tests/test_core/sources/test_db_table_source.py Updates mock connector to include validate_records for protocol conformance.
superpowers/specs/2026-07-08-itl-471-spiraldb-arrow-metadata-preservation-design.md Design spec describing encoding, restoration, and validation-hook approach.
superpowers/plans/2026-07-08-itl-471-spiraldb-arrow-metadata-preservation.md Implementation plan/checklist for the feature and tests.
src/orcapod/protocols/db_connector_protocol.py Adds validate_records to the connector protocol.
src/orcapod/databases/sqlite_connector.py Adds rejecting validate_records implementation for SQLite.
src/orcapod/databases/spiraldb_connector.py Implements metadata KV serialization/restoration + validate_records no-op.
src/orcapod/databases/postgresql_connector.py Adds rejecting validate_records implementation for PostgreSQL.
src/orcapod/databases/connector_arrow_database.py Replaces inline extension guard with self._connector.validate_records(records).
DESIGN_ISSUES.md Updates design notes and documents duplicated extension-detection logic.

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

Comment thread tests/test_databases/test_spiraldb_metadata_helpers.py
@kurodo3

kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary

One review comment was raised by Copilot and has been addressed:

Comment Action
Unused import pytest in tests/test_databases/test_spiraldb_metadata_helpers.py (Ruff F401) Removed the import in commit e6196ba2. The module uses only assert statements — no pytest.raises or other pytest APIs — so the import was dead code. All 34 tests continue to pass.

conn.rollback()
raise

def validate_records(self, records: pa.Table) -> None:

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.

let's generally issue an error for ANY metadata found as those won't be preserved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 29748ba. Both SQLiteConnector and PostgreSQLConnector now reject any Arrow field or schema metadata, not just extension types:

  • Per-field: raises if isinstance(field.type, pa.ExtensionType) or field.metadata is non-empty (any key-value pair)
  • Schema-level: raises if records.schema.metadata is non-empty

The error message names which fields have metadata and whether schema-level metadata is present.

The mirroring MockDBConnector in the test file was updated to match. The TestExtensionTypeWriteGuard class was renamed TestMetadataWriteGuard and extended with two new tests: one for arbitrary field metadata rejection and one for schema-level metadata rejection.

…cords (ITL-471)

Broaden SQLiteConnector and PostgreSQLConnector validate_records from
rejecting only Arrow extension-typed columns to rejecting any field or
schema metadata, since neither backend preserves Arrow metadata at all.
Update MockDBConnector and TestMetadataWriteGuard tests to match, adding
coverage for arbitrary field metadata and schema-level metadata rejection.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kurodo3

kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary

One review comment addressed:

Comment Action
validate_records should reject ANY metadata, not just extension types (eywalker) Commit 29748bac: broadened both SQLiteConnector and PostgreSQLConnector to reject any non-empty field metadata or schema-level metadata. Extension types are still caught via the isinstance(field.type, pa.ExtensionType) check (needed for in-memory extension types whose metadata hasn't been serialised to field-level yet). MockDBConnector in the test file mirrors the same logic. TestExtensionTypeWriteGuard renamed TestMetadataWriteGuard; two new tests added: arbitrary field metadata rejection and schema-level metadata rejection. All 4234 tests pass.

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

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Comment on lines +375 to +381
import pyarrow as _pa # noqa: PLC0415

problem_fields: list[str] = []
for field in records.schema:
if isinstance(field.type, _pa.ExtensionType) or field.metadata:
problem_fields.append(field.name)
has_schema_meta = bool(records.schema.metadata)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 0b43d99. Added a module-level _field_has_metadata(field) helper to sqlite_connector.py that recursively walks struct and list type trees (struct, list, large_list, fixed_size_list) in addition to checking the top-level field.metadata. validate_records now delegates to this helper for each top-level column, so nested child metadata is caught too. Two new tests cover the nested cases: one for a struct with an inner field carrying metadata, one for a list whose value field carries metadata.

Comment on lines +492 to +498
import pyarrow as _pa # noqa: PLC0415

problem_fields: list[str] = []
for field in records.schema:
if isinstance(field.type, _pa.ExtensionType) or field.metadata:
problem_fields.append(field.name)
has_schema_meta = bool(records.schema.metadata)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 0b43d99. Same change as for sqlite_connector.py: added _field_has_metadata(field) to postgresql_connector.py that recursively walks struct and list type trees. validate_records now uses it so nested child metadata (struct inner fields, list value fields) is detected and rejected rather than silently dropped on write.

Comment on lines +149 to +153
if _ARROW_METADATA_KEY not in kv:
return (None, {})

blob = json.loads(kv[_ARROW_METADATA_KEY].decode("utf-8"))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 0b43d99. _load_arrow_metadata now wraps the .decode("utf-8") and json.loads() calls in try/except (json.JSONDecodeError, UnicodeDecodeError). On failure it logs a warning at WARNING level (naming the exception type and message) and returns (None, {}) — the same value as the "key absent" path — so iter_batches reads the table normally without metadata restoration rather than raising. Two new unit tests cover the invalid-JSON and invalid-UTF-8 cases.

…eful KV decode (ITL-471)

validate_records in SQLiteConnector and PostgreSQLConnector now recursively
walks struct and list type trees to detect field metadata on nested children
(struct members, list value fields, large_list, fixed_size_list). Previously
only top-level field.metadata was checked, allowing writes with nested metadata
that would be silently dropped on read.

_load_arrow_metadata now catches JSONDecodeError and UnicodeDecodeError from
a corrupt or unexpected KV value, logs a warning, and returns (None, {}) so
iter_batches can continue without the metadata restoration — preventing a hard
failure that would make the whole table unreadable.

Tests: added two nested-metadata rejection tests (struct child, list value
field) and two corrupt-KV resilience tests (invalid JSON, invalid UTF-8).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kurodo3

kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary (commit 0b43d996)

Three Copilot comments addressed:

Comment Action
validate_records misses nested struct/list child metadata (SQLite) Added _field_has_metadata(field) module-level helper that recursively walks struct and list type trees (struct, list, large_list, fixed_size_list). validate_records now delegates to it.
validate_records misses nested struct/list child metadata (PostgreSQL) Same _field_has_metadata helper added to postgresql_connector.py.
_load_arrow_metadata raises hard on corrupt KV data Wrapped the .decode("utf-8") + json.loads() in try/except (json.JSONDecodeError, UnicodeDecodeError). On failure, logs a WARNING and returns (None, {}) so iter_batches reads the table without metadata restoration instead of failing entirely.

Tests added: nested struct child metadata rejection, nested list value field metadata rejection, corrupt-JSON KV resilience, invalid-UTF-8 KV resilience. All 4252 tests pass.

Cover previously untested paths across four test files:

- spiraldb_connector.py 100% (was 96%): _restore_field fixed_size_list
  branch, delete_table method
- sqlite_connector.py 100% (was 92%): validate_records (field/schema/nested
  metadata), _field_has_metadata recursive branches, _arrow_type_to_sqlite_sql
  unsupported-type fallback, iter_batches with NULL cursor.description and
  unquoted table name
- connector_arrow_database.py 99% (was 96%): invalid record_path components,
  get_all_records returns None when iter_batches is empty, add_records raises
  for missing/wrong-type record_id_column, skip_duplicates=True with pending
  conflicts (partial and full filter paths)
- test_spiraldb_metadata_helpers.py: _restore_field fixed_size_list child
  metadata restoration

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@eywalker eywalker merged commit d713937 into main Jul 8, 2026
11 checks passed
@eywalker eywalker deleted the eywalker/itl-471-spiraldbconnector-preserve-arrow-table-and-column-level branch July 8, 2026 19:50
@kurodo3

kurodo3 Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary

eywalker approved the PR (no new inline comments). All previous review comments have been addressed:

  • eywalker: reject ANY Arrow metadata in validate_records (not just extension types) — fixed in 29748ba
  • Copilot: unused import pytest — fixed in e6196ba
  • Copilot: missing nested struct/list metadata detection in validate_records — fixed in 0b43d99
  • Copilot: _load_arrow_metadata hard-fails on corrupt KV data — fixed in 0b43d99

Additional coverage work: spiraldb_connector.py and sqlite_connector.py both at 100%; connector_arrow_database.py at 99%. Ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants