feat(types): UUID Arrow type migration — pa.binary(16) for all UUID storage (PLT-1162)#161
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Formalises pa.binary(16) as the canonical Arrow type for all UUID values in OrcaPod — both system identifiers (UUID_ARROW_TYPE) and user-facing semantic round-trips (UUID_STRUCT_ARROW_TYPE). Documents the two-layer design, no-truncation rule, known Polars write_json limitation, and full implementation scope. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… (PLT-1162) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y via module __getattr__ (PLT-1162) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…uct round-trips (PLT-1162) Implements UUIDStructConverter in semantic_struct_converters.py following the existing PythonPathStructConverter/UPathStructConverter pattern. Stores UUIDs as fixed 16-byte binary in a single-field Arrow struct. Duck-typed to also accept uuid_utils.UUID objects (uuid7 etc.) which do not inherit from uuid.UUID but expose .bytes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ter (PLT-1162) Adds `can_handle_python_type`, `can_handle_struct_type`, and `hash_struct_dict` to `UUIDStructConverter` so it fully satisfies `SemanticStructConverterProtocol`. Also adds 7 new tests covering the new methods.
…c registry (PLT-1162) Register UUIDStructConverter in get_versioned_semantic_arrow_hasher so that uuid.UUID values are handled by the default semantic type registry. Add three tests verifying the registration by python type, struct signature, and semantic type name.
…th binary Arrow storage (PLT-1162) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…LT-1162) - Change record_id system tag column type from pa.large_string() to UUID_ARROW_TYPE (pa.binary(16)) in arrow_utils.add_system_tag_columns - Import UUID_ARROW_TYPE from orcapod.types in arrow_utils.py - Update stream_builder.py to generate deterministic UUID v5 bytes for record_id values (derived from source_id + provenance token), replacing the previous string-based provenance tokens passed to the system tag column - Add fixed_size_binary support to UniversalTypeConverter.arrow_type_to_python_type so UUID_ARROW_TYPE columns map to Python bytes - Fix _predict_system_tag_schema in Join and MergeJoin operators to carry through the actual column type (bytes for record_id) rather than hardcoding str - Fix CachedSource to exclude random system-tag record_id columns from the row hash used for deduplication (record_ids are now deterministic UUIDs per source, so dedup is stable) - Update tests that passed string record_ids to pass UUID bytes instead Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…D binary migration (PLT-1162) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- datagram.py: datagram_id property returns bytes (uuid7().bytes), update record_id parameter and internal method signatures from str|None to bytes|None - data_function.py: generate uuid7() once, use .bytes for record_id Arrow storage and .hex for source_info provenance strings (source_info stays large_string) - logging_observer.py: log_id uses uuid7().bytes with pa.binary(16) column type - status_observer.py: status_id uses uuid7().bytes with pa.binary(16) column type in both _ContextualizedStatusObserver and _ScopedStatusObserver - Cascade updates to maintain consistency: - protocols/core_protocols/datagrams.py: datagram_id protocol return type bytes - tag_data.py: record_id parameter annotations bytes|None - function_node.py: data_record_id bytes, DATA_RECORD_ID stored as pa.binary(16) - databases: _ensure_record_id_column, add_record accept str|bytes in all impls - database_protocols.py: add_record protocol updated to str|bytes - Fix two tests broken by the type change: source_info UUID format (dashes→hex) and data_record_id column type (str→bytes with len==16) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nary(16)) (PLT-1162) - Move uuid out of the grouped string branch in _pg_type_to_arrow so it returns UUID_ARROW_TYPE = pa.binary(16) instead of pa.large_string() - Add _coerce_pg_value helper to convert psycopg uuid.UUID objects to bytes before Arrow array construction in iter_batches - Update test_uuid in TestPgTypeToArrow to assert pa.binary(16) - Import _coerce_pg_value in test file for completeness
kurodo3
Bot
force-pushed
the
eywalker/plt-1162-design-spike-uuid-arrow-type-mapping-large_string-vs
branch
from
414d87e to
0c922b7
Compare
eywalker
left a comment
eywalker
left a comment
There was a problem hiding this comment.
Reviewing through, I don't think that the use of UUID TYPE constant is actually helping with anything -- let's just make that fixed to be defined as binary type and drop use of constants as we as the getattr trick.
| c for c in live_table.column_names | ||
| if c.startswith(constants.SYSTEM_TAG_RECORD_ID_PREFIX) | ||
| ] | ||
| hash_table = ( |
There was a problem hiding this comment.
was this some kind of a bug that was present before?
There was a problem hiding this comment.
Yes, this was a latent pre-existing bug exposed by the migration.
Before this PR, was str(uuid7()) — a fresh random string on every run. Since the dedup hash included the record_id column and that value changed every invocation, the cached-source dedup never actually recognised repeat rows as duplicates. It was silently broken.
The migration made it visible: deterministic UUID v5 record_ids meant the hash would be stable across runs, which is correct — but we also don't need to include the record_id in the hash at all, since it's derived from the source_id + provenance token already captured in the hash. So the fix excludes record_id tag columns from the dedup hash (removing redundant information) while making the dedup work correctly for the first time.
|
|
||
| # Namespace UUID for source record IDs — all source record IDs are derived | ||
| # from this namespace and the row's source_id + provenance token. | ||
| _SOURCE_RECORD_ID_NAMESPACE = uuid.UUID("7f8e9d0c-1b2a-3c4d-5e6f-7a8b9c0d1e2f") |
There was a problem hiding this comment.
how is this value chosen?
There was a problem hiding this comment.
It was arbitrary — chosen once by the implementer with no principled derivation, which is why it looked opaque.
Fixed: replaced with a UUID v5 derived from uuid.NAMESPACE_URL + a stable OrcaPod-specific URL string, so the value is reproducible from its inputs rather than being a magic hex literal:
_SOURCE_RECORD_ID_NAMESPACE = uuid.uuid5(
uuid.NAMESPACE_URL,
"https://orcapod.io/namespaces/source-record-id",
)Added a doc comment noting this value must never change once deployed, since changing it would invalidate all existing stored record IDs.
|
|
||
| @property | ||
| def datagram_id(self) -> str: | ||
| def datagram_id(self) -> bytes: |
There was a problem hiding this comment.
wouldn't this be cleaner to return an actual UUID object instead?
There was a problem hiding this comment.
Agreed — done. datagram_id now returns uuid.UUID throughout the protocol, Datagram, and Tag/Data. Generation uses uuid7() normalised to stdlib uuid.UUID (for cross-library equality). Arrow array construction calls .bytes at the point of actual serialisation, keeping the Python interface ergonomic.
|
|
||
|
|
||
| def __getattr__(name: str) -> object: | ||
| """Lazy resolution for module-level constants that depend on pyarrow. |
There was a problem hiding this comment.
which constant would have triggered the invocation of pyarrow and why?
There was a problem hiding this comment.
Accessing either constant triggered it. The __getattr__ in types.py called import pyarrow as _pa to construct the value on first access. So any from orcapod.types import UUID_ARROW_TYPE or from orcapod import UUID_ARROW_TYPE call would load the full pyarrow C-extension immediately — which made the __init__.py eager import of the constant just as non-lazy. The __init__.py __getattr__ was needed to defer even that.
This whole mechanism is removed now per the overall review direction: the constants are gone, both __getattr__ functions are removed, and pa.binary(16) is used inline at each site.
| assert len(values) == result_table.num_rows | ||
| for val in values: | ||
| assert isinstance(val, str) | ||
| # source_id columns are large_string (str), record_id columns are |
There was a problem hiding this comment.
perform a more thorough test based on the target column
There was a problem hiding this comment.
Done. The assertion now checks per column: record_id columns (matching SYSTEM_TAG_RECORD_ID_PREFIX) assert isinstance(val, bytes) and len(val) == 16; all other system tag columns assert isinstance(val, str). This catches any accidental type regression in either direction.
…atagram_id, per-column tests (PLT-1162) - Remove UUID_ARROW_TYPE and UUID_STRUCT_ARROW_TYPE constants and their lazy __getattr__ resolvers from orcapod.types and orcapod.__init__; use pa.binary(16) / pa.struct([...]) inline everywhere - Replace _SOURCE_RECORD_ID_NAMESPACE opaque hex literal with a principled uuid.uuid5 derivation with doc comment explaining it must never change - Change datagram_id property return type from bytes to uuid.UUID throughout (protocol, Datagram, Tag, Data); generation via uuid7() normalised to stdlib uuid.UUID to ensure cross-library equality; call sites that build pa.binary(16) arrays use .bytes - Strengthen system-tag column type assertion in test_operators to per-column checks: record_id columns asserted as bytes+len==16, source_id columns asserted as str Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review round 1 — addressed in commit
|
| """Return (or lazily generate) the datagram's unique ID.""" | ||
| if self._datagram_id is None: | ||
| self._datagram_id = str(uuid7()) | ||
| # uuid7() returns a uuid_utils.UUID; normalise to stdlib uuid.UUID |
There was a problem hiding this comment.
Is there a good reason to mix uuid_utils and stdlib for handling uuid? Does uuid_utils offer richer set of functions for uuid generation/handling?
There was a problem hiding this comment.
Agreed — is only here for UUIDv7 generation (time-ordered, monotonic), which isn't in the stdlib before Python 3.12. However and are distinct types that don't compare equal even for identical bit patterns, so we normalise immediately to on creation. The comment has been expanded to make this explicit.
| f"{stream.pipeline_hash().to_hex(n_char)}:{idx}" | ||
| ) | ||
| system_tag_fields[new_name] = str | ||
| system_tag_fields[new_name] = col_type |
There was a problem hiding this comment.
We should include tests to ensure that the removal force convresion into str is not creating a new error?
There was a problem hiding this comment.
Done. Added in . It iterates over all system-tag columns in the predicted schema after a two-way join and asserts that record_id columns map to (from ) and source_id columns map to — verifying that is carried through unchanged.
| ) | ||
|
|
||
| # Compute per-row record hashes for dedup: hash(full row) | ||
| # Compute per-row record hashes for dedup: hash(full row excluding |
There was a problem hiding this comment.
I acutally don't understand the logic here. For system tag, the source_id is the unique specifier of the nature/identity of the source and the record_id is a unique specifier about the origin of the specific record. If we are deeming record_id to be truly redundant with record_hash, then we should use record_id as the record_hash. Shouldn't it be the case that we'd actually include everything that's part of the identity of the row as part of the record hash so that in case two entries with everything identical BUT the record_id, suggesting that they originated from two distinct entries in the source that happens to carry identical values, we can correctly distinguish them? Otherwise, of course the inclusion of the rest of the fields are important to detect situation where same record_id entries end up having distinct values but we still want to track it.
There was a problem hiding this comment.
Agreed — I reverted the exclusion. The record_id system tag encodes row origin (source identity + provenance token), so two rows that are byte-identical in their data columns but originate from different positions in the source will have distinct record_id values and must be stored as separate entries. Hashing the full live_table (including the record_id column) is correct. The dedup logic now hashes all system tag columns.
| # record IDs. | ||
| _SOURCE_RECORD_ID_NAMESPACE = uuid.uuid5( | ||
| uuid.NAMESPACE_URL, | ||
| "https://orcapod.io/namespaces/source-record-id", |
There was a problem hiding this comment.
What is this typically set to? Also be sure to use Orcapod instead of OrcaPod throughout the codebase. If this is meant to be a namespace URL that we would maintain, make it https://orcapod.org/namespaces/source-record-id
There was a problem hiding this comment.
Fixed:
- URL corrected to
https://orcapod.org/namespaces/source-record-id(washttp://orcapod.ai/...) - Capitalisation corrected to "Orcapod"
- Added the pre-computed value as a comment:
uuid.UUID('877ec89b-6645-5852-ba37-a94604043f5e')
| return f"row_{row_index}" | ||
|
|
||
|
|
||
| def _make_record_id_bytes(source_id: str, provenance_token: str) -> bytes: |
There was a problem hiding this comment.
let's instead return uuid.UUID for consistency?
There was a problem hiding this comment.
Fixed — _make_record_id_bytes renamed to _make_record_id and now returns a uuid.UUID rather than raw bytes. The call site does .bytes to get the 16-byte value for Arrow storage. This gives callers a real UUID object (with full UUID API surface) instead of an opaque byte string.
| self._scoped_path = _scoped_path | ||
| self._pending_batches: dict[str, pa.Table] = _shared_pending_batches if _shared_pending_batches is not None else {} | ||
| self._pending_record_ids: dict[str, set[str]] = _shared_pending_record_ids if _shared_pending_record_ids is not None else defaultdict(set) | ||
| self._pending_record_ids: dict[str, set[str | bytes]] = _shared_pending_record_ids if _shared_pending_record_ids is not None else defaultdict(set) |
There was a problem hiding this comment.
why do we still have to take in str?
There was a problem hiding this comment.
Dropped str entirely. compute_pipeline_entry_id now returns bytes (a method-prefixed digest via ContentHash.to_prefixed_digest()), and UUID-based record IDs are stored as 16-byte raw values — both are bytes. There is no longer a caller path that passes a str record ID, so the str union arm was just dead code.
| self, arrow_data: pa.Table, record_id: str | bytes | ||
| ) -> pa.Table: | ||
| if self.RECORD_ID_COLUMN not in arrow_data.column_names: | ||
| arrow_type = pa.binary(16) if isinstance(record_id, bytes) else pa.large_string() |
There was a problem hiding this comment.
why is this necessary? What currently can yield incompatible str record_id?
There was a problem hiding this comment.
Cleaned up — record_id is now bytes only and _ensure_record_id_column always creates a pa.large_binary() column (no conditional branch on type). pa.large_binary() is used uniformly for all database record ID columns: it accommodates both fixed-size UUID bytes (16 bytes) and variable-length method-prefixed pipeline entry digests without requiring a schema change if sizes shift.
| if self.RECORD_ID_COLUMN not in arrow_data.column_names: | ||
| # Add record_id column at the beginning | ||
| key_array = pa.array([record_id] * len(arrow_data), type=pa.large_string()) | ||
| arrow_type = pa.binary(16) if isinstance(record_id, bytes) else pa.large_string() |
There was a problem hiding this comment.
we should drop support for large_string
There was a problem hiding this comment.
Dropped large_string support entirely. _ensure_record_id_column now always creates a pa.large_binary() column unconditionally, and all str | bytes / str type annotations in the Delta Lake DB layer have been updated to bytes only — matching the protocol and the other database implementations.
…o_prefixed_digest()
Addresses second round of PR review comments on the UUID Arrow type migration.
Key changes:
- Add `ContentHash.to_prefixed_digest()` → `b"{method}:{digest}"` for
human-readable method-prefixed binary record IDs
- `compute_pipeline_entry_id()` now returns `bytes` via `to_prefixed_digest()`;
update all `dict[str, ...]` caches in FunctionNode to `dict[bytes, ...]`
- Migrate all database record-ID storage from `pa.binary(16)` → `pa.large_binary()`
so UUID-based and hash-based IDs share a single consistent Arrow type
- Drop `str | bytes` unions from `ArrowDatabaseProtocol`, all DB implementations,
and `_pending_record_ids`; record IDs are now uniformly `bytes`
- `_make_record_id_bytes` → `_make_record_id` returning `uuid.UUID` (caller calls `.bytes`)
- Namespace URL fixed: `orcapod.io` → `orcapod.org`, "Orcapod" capitalisation,
computed UUID value added as comment
- Revert C3: include all system tag columns (including record_id) in dedup hash
so rows with identical data values but different provenance are not collapsed
- Add test for `col_type` preservation through `_predict_system_tag_schema`
- Improve datagram.py comment explaining uuid_utils vs stdlib normalisation
- Update all DB and FunctionNode tests to use `bytes` record IDs
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Round 2 review — summary of changesAll review comments addressed in commit C1 —
|
…ID mapping - test-objective/unit/test_databases.py: update all string record IDs to bytes literals (b"...") to match the bytes-only record_id API introduced in the large_binary migration. String IDs were silently encoded by PyArrow but then not found during set-membership lookup (str != bytes). - postgresql_connector.py: map PostgreSQL uuid type to pa.large_string() instead of pa.binary(16), completing the TODO: PLT-1162 marker in the integration test. psycopg returns uuid.UUID objects; the value coercion now calls str(value) to produce the canonical "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" form. - tests/test_databases/test_postgresql_connector.py: update test_uuid assertion to expect pa.large_string(). - tests/test_databases/test_postgresql_connector_integration.py: remove the # TODO: PLT-1162 comment now that the assertion is fulfilled. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add a shared ``coerce_record_id(str | bytes) -> bytes`` helper in ``databases/_utils.py`` and apply it at the entry of every public method that accepts a ``record_id`` parameter: - ``add_record`` / ``get_record_by_id`` / ``get_records_by_ids`` in ``InMemoryArrowDatabase``, ``ConnectorArrowDatabase``, ``DeltaTableDatabase``, and ``NoOpArrowDatabase`` - ``_ensure_record_id_column`` and ``_create_record_id_filter`` in ``DeltaTableDatabase`` The protocol signatures are updated to ``str | bytes`` to reflect that both are accepted. Internally all paths remain ``bytes`` after coercion; the Arrow storage type stays ``pa.large_binary()``. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l only The str → bytes coercion is a convenience feature of the concrete database implementations, not part of the ArrowDatabaseProtocol contract. The protocol signatures stay bytes-only so that callers coding to the protocol get a clear type contract; the leniency lives in the implementing classes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No leading underscore; the file is not private to the package, just a shared utility module. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| import logging | ||
| import re | ||
| import sys | ||
| import uuid as _uuid_module |
There was a problem hiding this comment.
What's the need to name this _uuid_module?
There was a problem hiding this comment.
Fixed — removed the alias. Changed to plain import uuid and updated the usage to uuid.UUID(bytes=uuid7().bytes).
There was a problem hiding this comment.
Pull request overview
Migrates OrcaPod’s UUID handling from string-based Arrow storage toward binary representations, updating semantic type conversion, hashing/caching IDs, and database record ID APIs to work with bytes and fixed-size binary types.
Changes:
- Introduces a
UUIDStructConverterand registers it in the default semantic registry; updatesUUIDHandlerto hash UUIDs via raw bytes. - Switches many internal identifiers (system tags, pipeline entry IDs, observers, DB record IDs) from
strtobytesand updates tests accordingly. - Expands type conversion support for
fixed_size_binary→bytes, and updates multiple DB implementations/connectors to store record IDs in binary columns.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils/test_arrow_utils.py | Updates system tag record_id test input to use UUID bytes. |
| tests/test_semantic_types/test_uuid_struct_converter.py | Adds full unit coverage for UUIDStructConverter. |
| tests/test_semantic_types/test_semantic_registry.py | Verifies UUID converter registration and reverse lookup. |
| tests/test_hashing/test_uuid_handler.py | Adds tests asserting UUID handler returns 16-byte form. |
| tests/test_databases/test_postgresql_connector.py | Updates tests around PG type mapping/coercion (UUID mapping still asserted as string). |
| tests/test_databases/test_postgresql_connector_integration.py | Updates integration expectations (UUID mapping still asserted as string). |
| tests/test_databases/test_noop_database.py | Updates record_id arguments to bytes. |
| tests/test_databases/test_in_memory_database.py | Updates record_id arguments/expectations to bytes. |
| tests/test_databases/test_delta_table_database.py | Updates record_id arguments/expectations to bytes. |
| tests/test_databases/test_delta_table_database_s3.py | Updates record_id arguments/expectations to bytes. |
| tests/test_databases/test_connector_arrow_database.py | Updates record_id arguments/expectations to bytes. |
| tests/test_core/operators/test_operators.py | Adds assertions that system tag schemas preserve bytes vs str types. |
| tests/test_core/nodes/test_function_node_iteration.py | Updates cached entry-id key type to bytes. |
| tests/test_core/function_pod/test_function_pod_node.py | Updates data record-id assertions to 16-byte values. |
| tests/test_core/function_pod/test_function_node_caching.py | Updates pipeline entry-id type assertions to bytes. |
| tests/test_core/data_function/test_data_function.py | Adjusts source-info UUID segment expectations to hex form. |
| test-objective/unit/test_databases.py | Updates objective tests to use bytes record IDs and binary Arrow arrays. |
| test-objective/unit/test_arrow_data_utils.py | Updates system-tag tests to pass UUID bytes as record IDs. |
| superpowers/specs/2026-06-11-plt-1162-uuid-arrow-type-design.md | Adds design spec for UUID Arrow type migration. |
| superpowers/plans/2026-06-11-plt-1162-uuid-arrow-type.md | Adds implementation plan/checklist for migration steps. |
| src/orcapod/utils/arrow_utils.py | Changes system tag record_ids typing to bytes; updates record_id column storage type. |
| src/orcapod/types.py | Adds method-prefixed raw digest bytes API (to_prefixed_digest). |
| src/orcapod/semantic_types/universal_converter.py | Maps fixed_size_binary Arrow types to Python bytes. |
| src/orcapod/semantic_types/semantic_struct_converters.py | Adds UUIDStructConverter implementation. |
| src/orcapod/protocols/database_protocols.py | Updates Arrow database protocol record_id types to bytes. |
| src/orcapod/protocols/core_protocols/datagrams.py | Changes datagram_id protocol type to uuid.UUID. |
| src/orcapod/pipeline/status_observer.py | Stores _status_id as binary(16) UUID bytes. |
| src/orcapod/pipeline/logging_observer.py | Stores _log_id as binary(16) UUID bytes. |
| src/orcapod/hashing/versioned_hashers.py | Registers UUIDStructConverter in default semantic registry. |
| src/orcapod/hashing/semantic_hashing/builtin_handlers.py | Updates UUIDHandler to return UUID bytes. |
| src/orcapod/databases/utils.py | Adds helper to coerce record_id from str→bytes. |
| src/orcapod/databases/postgresql_connector.py | Adds value coercion helper; updates batch conversion pipeline; UUID mapping remains string. |
| src/orcapod/databases/noop_database.py | Broadens record_id parameter types to accept bytes. |
| src/orcapod/databases/in_memory_databases.py | Stores record IDs in binary Arrow columns; coerces input IDs to bytes. |
| src/orcapod/databases/delta_lake_databases.py | Stores record IDs in binary Arrow columns; coerces input IDs to bytes. |
| src/orcapod/databases/connector_arrow_database.py | Stores record IDs in binary Arrow columns; coerces input IDs to bytes. |
| src/orcapod/core/sources/stream_builder.py | Makes deterministic per-row UUID record IDs and stores bytes in system tags. |
| src/orcapod/core/sources/cached_source.py | Updates dedup hashing comment to reflect system-tag semantics. |
| src/orcapod/core/result_cache.py | Reads/writes DB record IDs as UUID bytes; converts back to uuid.UUID on lookup. |
| src/orcapod/core/operators/merge_join.py | Preserves original system-tag Python types (bytes vs str) in predicted schema. |
| src/orcapod/core/operators/join.py | Preserves original system-tag Python types; updates sort key defaults for bytes record IDs. |
| src/orcapod/core/nodes/function_node.py | Switches pipeline entry IDs and caches from str→bytes using to_prefixed_digest(). |
| src/orcapod/core/datagrams/tag_data.py | Changes datagram record_id parameter type to `uuid.UUID |
| src/orcapod/core/datagrams/datagram.py | Normalizes UUIDv7 IDs to stdlib uuid.UUID on generation. |
| src/orcapod/core/data_function.py | Stores record_id as uuid.UUID and source_info embeds UUID hex. |
| src/orcapod/init.py | Minor formatting-only change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| source_id_array = pa.array(source_ids, type=pa.large_string()) | ||
| record_id_array = pa.array(record_ids, type=pa.large_string()) | ||
| record_id_array = pa.array(record_ids, type=pa.large_binary()) | ||
|
|
There was a problem hiding this comment.
Fixed — changed add_system_tag_columns() to use pa.binary(16) for the record_id column. Record IDs in system tags are always UUID v5 bytes (exactly 16 bytes), so pa.binary(16) is the precise type. The docstring and the implementation now agree.
There was a problem hiding this comment.
Fixed in commit e0bbc37. add_system_tag_columns() now uses pa.binary(16) for the record_id column — both the field declaration and the pa.array() call — matching the docstring.
| table = table.append_column( | ||
| pa.field(record_id_col_name, pa.large_string(), nullable=False), record_id_array | ||
| pa.field(record_id_col_name, pa.large_binary(), nullable=False), record_id_array | ||
| ) |
There was a problem hiding this comment.
Fixed — same change as above. add_system_tag_columns() now appends the record_id column as pa.binary(16) (both the field definition and the pa.array() call). The docstring was already correct; the implementation is now consistent with it.
There was a problem hiding this comment.
Fixed in commit e0bbc37 (same change as above — both the field and the array call now use pa.binary(16)).
| if t == "uuid": | ||
| return _pa.large_string() | ||
|
|
There was a problem hiding this comment.
Fixed — reverted _pg_type_to_arrow("uuid") back to pa.binary(16), and updated _coerce_pg_value to extract .bytes from psycopg's uuid.UUID objects instead of calling str(). Unit test (test_uuid) and integration test (test_all_pg_types) updated accordingly.
There was a problem hiding this comment.
Fixed in commit e0bbc37. _pg_type_to_arrow("uuid") now returns pa.binary(16).
| # psycopg returns uuid columns as uuid.UUID objects; large_string() needs str | ||
| if arrow_type == _pa.large_string() and hasattr(value, "hex") and not isinstance(value, str): | ||
| return str(value) | ||
| return value |
There was a problem hiding this comment.
Fixed in commit e0bbc37. _coerce_pg_value() now checks arrow_type == pa.binary(16) and returns value.bytes (raw 16 bytes) instead of str(value).
| def test_uuid(self): | ||
| assert _pg_type_to_arrow("uuid", "uuid") == pa.large_string() | ||
| result = _pg_type_to_arrow("uuid", "uuid") | ||
| assert result == pa.large_string() | ||
|
|
There was a problem hiding this comment.
Fixed in commit e0bbc37. test_uuid now asserts pa.binary(16).
| assert infos["g"].arrow_type == pa.large_string() | ||
| assert infos["h"].arrow_type == pa.large_binary() | ||
| assert infos["i"].arrow_type == pa.large_string() # TODO: PLT-1162 | ||
| assert infos["i"].arrow_type == pa.large_string() | ||
| assert infos["j"].arrow_type == pa.large_string() |
There was a problem hiding this comment.
Fixed in commit e0bbc37. The integration test now asserts infos["i"].arrow_type == pa.binary(16) for the UUID column.
| def __init__(self) -> None: | ||
| super().__init__("uuid") | ||
| self._python_type = _uuid_module.UUID | ||
| self._arrow_struct_type = pa.struct([pa.field("uuid", pa.binary(16))]) | ||
|
|
There was a problem hiding this comment.
No code change needed — updated the PR description instead. Adding module-level constants to types.py would trigger PyArrow import at types import time, defeating the LazyModule pattern already used there. Each call site keeping its own pa.binary(16) literal is clear and avoids that footgun. The PR description table row for types.py now reflects this.
| @property | ||
| def datagram_id(self) -> str: | ||
| def datagram_id(self) -> uuid.UUID: | ||
| """Return (or lazily generate) the datagram's unique ID.""" | ||
| if self._datagram_id is None: | ||
| self._datagram_id = str(uuid7()) | ||
| # uuid_utils is used here because it provides UUIDv7 (time-ordered, | ||
| # monotonic) generation, which is not available in the stdlib before | ||
| # Python 3.12. However, uuid_utils.UUID and stdlib uuid.UUID are | ||
| # distinct types that do not compare equal even for identical bit | ||
| # patterns. We therefore normalise to stdlib uuid.UUID immediately | ||
| # so that the public API always returns a consistent type and | ||
| # equality / hashing work correctly regardless of how a UUID was | ||
| # originally produced. | ||
| self._datagram_id = uuid.UUID(bytes=uuid7().bytes) | ||
| return self._datagram_id |
There was a problem hiding this comment.
The code is correct — datagram_id intentionally returns uuid.UUID (not bytes), as agreed in a prior review round. The PR description's Breaking Change note was wrong. Updated it to correctly state: datagram_id → uuid.UUID; record_id/_log_id/_status_id → bytes (16 bytes).
…op uuid module alias - Remove `import uuid as _uuid_module` alias in data_function.py — use plain `import uuid` instead - Change system-tag record_id column from pa.large_binary() to pa.binary(16) in arrow_utils.py: record IDs are always UUID v5 bytes (exactly 16 bytes), so a fixed-size type is more precise - Revert PostgreSQL UUID → Arrow mapping from pa.large_string() back to pa.binary(16), and coerce uuid.UUID values via .bytes instead of str() - Update unit and integration test assertions to match pa.binary(16) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review round 3 — changes madeThree fixes in commit e0bbc37:
System-tag record_id column type changed to PostgreSQL UUID mapping reverted to All 3616 tests pass (excluding live-postgres integration tests, which require a running instance). |
Copilot review round — changes madeCommit e0bbc37 addressed comments 1–6 (all were the same class of issue:
No code change for comments 7–8 — PR description corrections only:
|
The property returns a uuid.UUID, so the name datagram_uuid is more descriptive. Renames the public property and protocol method, the private backing field (_datagram_uuid), and all call sites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Replace
pa.large_string()UUID storage withpa.binary(16)(fixed_size_binary[16]) across the entire OrcaPod codebase, introducing a semantic struct converter and updating all UUID generation and storage sites.Changes
types.pypa.binary(16)directly. Adding module-level lazy constants would trigger PyArrow import attypesimport time, defeating theLazyModulepattern already used there.semantic_struct_converters.pyUUIDStructConverterwith fullSemanticStructConverterProtocolcomplianceversioned_hashers.pyUUIDStructConverterregistered under"uuid"in the default semantic type registrybuiltin_handlers.pyUUIDHandler.handle()returnsobj.bytes(16 raw bytes) instead ofstr(obj)arrow_utils.pypa.binary(16); cascading fixes touniversal_converter.py,join.py,merge_join.py,stream_builder.py,cached_source.pydatagram.py,data_function.pydatagram_idnow returnsuuid.UUID(normalized fromuuid_utils.uuid7); source-info UUID segment uses.hexlogging_observer.py,status_observer.pyuuid7().bytes(raw 16 bytes) instead ofstr(uuid7())for_log_id/_status_idArrow columnspostgresql_connector.pyuuidPG type →pa.binary(16);_coerce_pg_value()converts driveruuid.UUIDobjects to.bytes; TODO comment removeddatabases/utils.pycoerce_record_id()helper: transparently encodesstrrecord IDs tobytesat DB method boundariessuperpowers/specs/,superpowers/plans/Test plan
UUIDStructConvertersuite (19 tests) including Arrow array round-trip, bytearray handling, protocol method coverageUUIDHandlerreturns bytes testpa.binary(16)foruuid)Notes
Breaking changes (pre-v0.1.0, intentional per spec):
datagram_idis nowuuid.UUID(returned byDatagram.datagram_id,Tag.datagram_id,Data.datagram_id). Any code assuming it wasstrwill break.record_id(DB pipeline entry IDs),_log_id,_status_idare nowbytes(16 bytes). Any code assuming these werestrwill break.Polars
write_json()limitation: pola-rs/polars#15410 — OrcaPod has zerowrite_json()calls, unaffected in practice.SQLite: No change required —
TEXTcolumns remainpa.large_string(). Arrow type round-trip fidelity for loosely-typed backends is tracked in PLT-1615.run_id: Intentionally unchanged — it's a transient Python string passed to observer callbacks, never stored in Arrow.Fixes PLT-1162
🤖 Generated with Claude Code