fix: extension-type metadata dropped for list-backed logical types (ITL-627) - #257
Conversation
…ixes Root causes identified: - ListLogicalType.get_polars_extension_type() omits metadata= arg, causing Polars to export b'' on to_arrow() which _deserialize rejects - SemanticHashingVisitor.visit_extension short-circuits on isinstance check for generic aliases like list[File] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add Defect 3: MergeJoin drops extension type when aggregating logical-type columns - Expand Defect 2 tests: set[File], list[list[T]], Dataclass with list[T] field - Add list[File] x list[File] -> list[list[File]] MergeJoin case (was wrongly out of scope) - Correct Fix 3: binary_output_schema needs no change (Schema stores Python types already correct) - Add implementation plan with complete code for all 4 tasks
…stLogicalType ListLogicalType.get_polars_extension_type() was not passing the JSON metadata bytes to make_polars_extension_type, so ext_metadata() returned None. Polars exported b'' on to_arrow(), which _deserialize rejected with ValueError during the Join/MergeJoin Polars round-trip. One-line fix covers both list[T] and set[T]. Fixes ITL-627 (Defect 1).
…-trip Exercises the full Join and MergeJoin Polars round-trip with list[Path] extension columns, confirming Fix 1 (ITL-627 Defect 1) at the operator integration level.
…ion boilerplate Keep registration boilerplate in both join tests and add a comment explaining why it is required: ArrowTableStream does not trigger LogicalType registration, so without explicit pa/pl registration Polars degrades the extension type to its storage type during the operator's round-trip. Add row-count and value spot-check assertions to verify data integrity after the join. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SemanticHashingVisitor.visit_extension previously short-circuited on `not isinstance(python_type, type)` for list[File] (a GenericAlias), returning the extension type unchanged — raw JSON path strings were hashed instead of file contents. Fix: detect list-backed extension types before the isinstance guard and delegate to _visit_list_elements with a virtual large_list(elem_ext_type) so each element is hashed identically to the scalar visit_extension path. Handles set[T], list[list[T]], and arbitrary nesting depth via recursion. Adds 9 regression tests covering: list[File], set[File], list[list[File]], struct with list[File] field, passthrough when no handler, passthrough for list[Path], content-change sensitivity, same-content determinism, and element-wise hash symmetry with scalar File hashing.
…docstring - Remove redundant from-body imports of File, pa, uuid, PythonTypeHandlerRegistry, SemanticAwarePythonHasher in test_extension_type_hashing.py — all were already available at module level. - Add LogicalPath, ListLogicalType, PythonTypeHandlerRegistry, SemanticAwarePythonHasher to module-level imports. - Add idempotency note to _make_list_file_ext_type / _make_scalar_file_ext_type helpers. - Add Args:/Returns: sections and passthrough-case documentation to SemanticHashingVisitor.visit_extension docstring (Google style). - Add defensive-guard comment on the `if args:` fallthrough.
…logical-type columns binary_static_process used pa.array(merged_vals) which inferred array type from raw storage values, producing plain large_list(storage_type) and losing the extension wrapper. Fix snapshots the element Arrow type before the Polars round-trip, then builds the merged array as pa.ExtensionArray.from_storage(ListLogicalType, ...) when the element is an extension type. Handles nested list[list[T]] naturally. Fixes ITL-627 (Defect 3).
…s; improve test messages - Rename list_lt to list_logical_type in binary_static_process for clarity. - Add late-import comment explaining the circular dependency guard. - Add failure messages to shape assertions in TestMergeJoinLogicalTypeColumns.
… Join/MergeJoin regression tests Fresh ListLogicalType instances create different underlying Arrow extension classes. When two tests both call pa.register_extension_type() for the same extension name, the second call is a no-op — so the second test holds a class object that is never globally registered. Join's table.cast() then tries to cast between two different class objects for the same extension name, raising ArrowTypeError. Fix: use ctx.type_converter.register_python_class(list[Path]) which goes through the type converter's shared cache, guaranteeing the same class object across tests.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…pped-for-list-backed-logical
There was a problem hiding this comment.
Pull request overview
Fixes ITL-627 by ensuring ListLogicalType-backed Arrow extension columns retain their extension metadata through Polars round-trips, participate correctly in semantic content hashing, and remain extension-typed when merged by MergeJoin.
Changes:
- Preserve Polars extension metadata for
list[T]/set[T]by passing JSONmetadata=when constructing the Polars extension type. - Teach
SemanticHashingVisitor.visit_extensionto semantically hash list/set-backed extension columns element-by-element (including nested list/set cases) instead of incorrectly passing them through. - Rebuild merged colliding columns in
MergeJoinusing the appropriate list extension type (instead of type inference from raw storage), with regression tests covering scalar and nested list logical types.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/orcapod/logical_types/list_logical_type_factory.py | Passes JSON metadata into the Polars extension type so Polars→Arrow round-trips don’t drop/empty extension metadata. |
| src/orcapod/hashing/visitors.py | Fixes semantic hashing for list/set-backed extension types by routing through list element visitation (supports nesting). |
| src/orcapod/core/operators/merge_join.py | Preserves extension wrappers when merging colliding logical-type columns by reconstructing via the list logical type’s Arrow extension type. |
| tests/test_logical_types/test_list_logical_type.py | Adds targeted regression tests validating Polars extension metadata is present and survives Polars→Arrow. |
| tests/test_hashing/test_extension_type_hashing.py | Adds comprehensive regression/contract tests for semantic hashing of list[File] / set[File] and nested forms. |
| tests/test_core/operators/test_operators.py | Adds Join integration regression test for list extension columns surviving the Polars round-trip. |
| tests/test_core/operators/test_merge_join.py | Adds MergeJoin integration/regression tests for non-colliding and colliding logical-type list columns (scalar + nested). |
| superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md | Captures problem statement, root cause analysis, and intended fixes/tests for ITL-627. |
| superpowers/plans/2026-08-22-itl-627-list-extension-metadata.md | Implementation plan detailing steps, file map, and test strategy for the three defects. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
brian-arnold
left a comment
brian-arnold
left a comment
There was a problem hiding this comment.
Reviewed at 8a9f4a33; verified each point empirically in a worktree against both this branch and main. Four inline comments: two crashes in the new list-backed hashing path (one a regression from main), a silent list[T]/set[T] hash collision, an order-dependent test failure that the full-suite run masks, and a context-plumbing nit in MergeJoin.
Two things worth adding to the PR description: the hashing change invalidates content hashes for every list-backed extension column, so existing FunctionNode/OperatorNode records keyed on those hashes become cache misses and get recomputed; and the bundled polling-source _combine change isn't mentioned (it also drops sort_by_tags, so the accumulated stream is no longer tag-sorted per batch). Per CLAUDE.md, the three defects fixed here don't appear in DESIGN_ISSUES.md.
| elem_python_type | ||
| ) | ||
| virtual_list_type = pa.large_list(elem_arrow_type) | ||
| return self._visit_list_elements(virtual_list_type, storage_value) |
There was a problem hiding this comment.
Two crash paths here share one root cause: _visit_list_elements can return a list type whose element type is still a live pa.ExtensionType, and _process_table_columns then calls pa.array(data, type=large_list(extension<...>)), which Arrow rejects. Whole-column passthrough is safe because normalize_extension_columns converts it to storage type + field metadata; an extension sitting in a list value field never gets that treatment.
(a) Nested alias whose innermost type has no handler. is_nested_list_or_set (L255) commits to recursing before knowing whether the inner level can actually hash. For list[list[Path]]: outer recurses, inner visit_extension sees element Path, finds no handler, passes through, and the outer level builds large_list(extension<list[orcapod.path]>). Verified: a one-row extension<list[list[orcapod.path]]> column raises RuntimeError: Failed to process column 'x': extension, while the same table hashes to 000001c6 on main. So this is a regression, not merely an unsupported case.
(b) Empty or all-null inner list. _visit_list_elements falls back to new_element_type = element_type (the element extension type) when no processed element is non-None, and _process_table_columns latches the first row's type for the whole column. Verified on extension<list[orcapod.file]>: [[], [file]], [[None], [file]] and [[]] all raise; [[file], []] is fine. Same for list[uuid.UUID]. This is reachable in normal use — ListLogicalType.python_to_storage(None) returns [], so any row with a None/empty list[File] that happens to sort first breaks hashing for the entire table.
Suggested shape for the fix: make the hash-vs-passthrough decision purely type-driven (so every row of a column takes the same branch) and derive the new list type from that decision rather than from the row data:
# unwrap list/set nesting down to the innermost plain type
inner = elem_python_type
while typing.get_origin(inner) in (list, set):
inner = typing.get_args(inner)[0]
hashable = isinstance(inner, type) and self._python_hasher.type_handler_registry.has_handler(inner)
if not hashable:
return extension_type, storage_value # whole-column passthrough, as on mainand then build the target type from the nesting depth (large_list(large_binary), large_list(large_list(large_binary)), ...) instead of letting _visit_list_elements infer it from the first non-None row. That covers (b) as well without changing the shared _visit_list_elements used by visit_list.
There was a problem hiding this comment.
Fixed. The list-backed detection block now makes a single type-driven hashability decision before touching any rows:
- Unwrap the element type through any list/set nesting to reach the innermost plain type.
- Check whether that innermost type has a semantic handler.
- If not hashable: whole-column passthrough (same as ).
- If hashable: call , but discard its returned type (using ). The output type is set to unconditionally, so empty and null inner lists no longer latch an extension type in .
Crash (a) — regression: because Path has no handler, the innermost-type check returns and the entire column passes through before any row is visited.
Crash (b) — empty/null inner list: still falls back to the element extension type when all rows are null/empty (its existing behaviour), but the returned type is discarded. The outer call always returns regardless of data content.
| # We intercept here and delegate to _visit_list_elements with a virtual | ||
| # large_list(elem_ext_type) so each element goes through visit_extension. | ||
| if ( | ||
| typing.get_origin(python_type) in (list, set) |
There was a problem hiding this comment.
The outer wrapper's extension identity is dropped on this branch. The scalar path below (L281-289) deliberately folds extension_type.extension_name into the hash token, but this branch returns a bare large_list(large_binary) with no record of which wrapper produced it.
Verified: single-row extension<list[orcapod.file]> and extension<set[orcapod.file]> tables holding the same file both hash to 0000019c292aa9a0 on this branch; on main they differ (000001ea... vs 000001df...). Two structurally distinct logical types are now indistinguishable to hash_table, so a memoized record keyed on one can be served for the other.
This is a silent-wrong-result bug rather than a crash, and the fix is small (include extension_type.extension_name in the result the way the scalar path does). Worth doing before merge, since fixing it later is another cache-invalidating hash change.
There was a problem hiding this comment.
Fixed. The result now includes the outer extension name the same way the scalar path does.
The list-backed path now returns (pa.large_binary(), combined_bytes) where combined_bytes = type_name.encode() + b"::" + b"\x00".join(element_hashes) and type_name = extension_type.extension_name.replace(".", ":"). For extension<list[orcapod.file]> this produces b"list[orcapod:file]::<h0>\x00<h1>". For extension<set[orcapod.file]> with the same files it produces b"set[orcapod:file]::<h0>\x00<h1>" — distinct because the outer wrapper name differs.
Added test_list_and_set_file_extension_produce_distinct_hashes to directly assert the two hashes differ.
| # Register the extension types with both Arrow and Polars registries so | ||
| # the round-trip can reconstruct the extension type (not fall back to storage). | ||
| try: | ||
| pa.register_extension_type(ext_type) |
There was a problem hiding this comment.
This test constructs a fresh ListLogicalType(LogicalPath()) (bypassing the converter registry) and registers it globally via pa.register_extension_type / pl.register_extension_type with no teardown. The global PyArrow/Polars registries then hold a different _ArrowExt_list_orcapod_path_ class than the one the orcapod registry hands out.
uv run pytest tests/test_logical_types tests/test_core/operators fails with:
ArrowTypeError: Casting from 'extension<list[orcapod.path]>' to different extension type 'extension<list[orcapod.path]>' not permitted
in TestJoinWithListExtensionColumn::test_join_preserves_list_extension_column — another test added by this PR. The full tests/ run passes (4711) only because test_core sorts before test_logical_types, so the green checklist is an artifact of collection order.
Fix: obtain the type from the registry the way the other new tests do — ctx.type_converter.get_logical_type_for_python_type(list[Path]) — or unregister in a fixture teardown.
There was a problem hiding this comment.
Fixed. The test now uses ctx.type_converter.register_python_class(list[Path]) instead of constructing a fresh ListLogicalType and calling pa.register_extension_type / pl.register_extension_type manually.
register_python_class calls register_logical_type internally, which registers with both the Arrow and Polars global registries using the same class object the orcapod registry holds. The fresh-instance approach registered a different class under the same extension name, causing the ArrowTypeError: Casting from extension<list[orcapod.path]> to different extension type failure when tests ran in test_logical_types → test_core/operators order.
Verified: uv run pytest tests/test_logical_types tests/test_core/operators now passes (698 tests, 0 failures).
| # Late import: orcapod.contexts pulls in the full type system; | ||
| # importing at module level creates a circular dependency. | ||
| from orcapod.contexts import get_default_context | ||
| tc = get_default_context().type_converter |
There was a problem hiding this comment.
get_default_context() here bypasses the operator's own context. Every other type-converter call in this layer goes through the input stream's context — stream.data_context.type_converter in batch.py, group_by.py, index.py, pick.py — so this should be left_stream.data_context.type_converter. The "circular dependency" rationale in the comment doesn't apply either: reaching the stream's context needs no import.
Concretely: a MergeJoin over streams built with a non-default DataContext reconstructs the merged column's extension type from the default registry. If the two registries hold distinct classes for the same extension name, you hit exactly the ArrowTypeError: ... different extension type ... not permitted failure described in the test-ordering comment on tests/test_logical_types/test_list_logical_type.py.
There was a problem hiding this comment.
Fixed. Replaced get_default_context().type_converter with left_stream.data_context.type_converter, hoisted above the colliding-keys loop, and removed the late import of get_default_context.
The "circular dependency" comment was wrong — reaching stream.data_context needs no module-level import. The fix matches the pattern used in index.py and pick.py.
|
Correction to my review summary above: the polling-source |
1. **Test contamination** (test_list_logical_type.py): Replace the fresh `ListLogicalType(LogicalPath())` + manual `pa.register_extension_type` / `pl.register_extension_type` with `ctx.type_converter.register_python_class(list[Path])`. The manual approach registered a different class object than the orcapod registry held, causing `ArrowTypeError: Casting from extension<list[orcapod.path]> to different extension type` when tests ran in `test_logical_types` before `test_core/operators` order. 2. **list[T] / set[T] hash collision** (visitors.py): The outer extension name was not encoded in the result, so `extension<list[orcapod.file]>` and `extension<set[orcapod.file]>` with identical contents produced identical hashes. Fixed by folding `extension_type.extension_name` into the combined result, mirroring the scalar path encoding. 3. **Two crash paths** (visitors.py): (a) `list[list[Path]]` — `is_nested_list_or_set` committed to recursing before checking whether the innermost type has a handler, causing `_visit_list_elements` to return `large_list(extension<...>)` which Arrow rejects in array creation. (b) Empty/null inner list — `_visit_list_elements` fell back to the element extension type when no non-null element was present, latching an extension type in `_process_table_columns`. Fixed by making the hash-vs-passthrough decision type-driven (unwrap nesting to innermost, check handler) before visiting any rows, and discarding the returned list type from `_visit_list_elements` (using `_`) since we return `(large_binary(), combined)` regardless. 4. **Wrong context in MergeJoin** (merge_join.py): `get_default_context().type_converter` bypassed the operator's actual context. Replaced with `left_stream.data_context.type_converter`, hoisted above the colliding-keys loop, and removed the late import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Tested these changes on my end and they appear to resolve the issues! |
Response to brian-arnold's review (round 2)All four issues addressed in commit
Tests updated in |
|
Response to brian-arnold review (round 2), commit fb2b062. Four issues fixed:
3a. Crash — nested unhashable alias (visitors.py L255): the detection block now unwraps list/set nesting to the innermost type and checks handler existence before visiting any rows. list[list[Path]] gets hashable=False (Path has no handler) and passes through immediately, restoring main-branch behaviour. 3b. Crash — empty/null inner list (visitors.py): _visit_list_elements return type is discarded. The outer call always returns (pa.large_binary(), combined_bytes) regardless of data content, so an empty inner list no longer latches an extension type in _process_table_columns.
Tests updated in test_extension_type_hashing.py: five assertions on large_list(large_binary) updated to large_binary, list-subscript element checks rewritten to split combined bytes by \x00, new collision-prevention test added. |
…204) ITL-627 Defect 2 reported that list-wrapping a File column lost content hashing: build_aggregated_table produced a list[orcapod.file] hashed by its storage values (JSON path strings), so editing a file silently failed to invalidate. Verified against 8968018, before this branch was rebased. It does not reproduce on the current base -- #257 fixed it in SemanticHashingVisitor.visit_extension, which previously short-circuited for generic aliases like list[File]. That fix added thorough unit coverage in test_extension_type_hashing.py::TestListExtensionHashing, but nothing exercises the job level, which is where the failure was observed and where it fails silently rather than raising. Adds a parametrized per-row/grouped test: cold run executes, identical re-run hits the cache, and a content edit at the same path re-executes. Mutation-checked by disabling the list/set unwrapping branch in visitors.py -- the grouped case fails while per_row still passes, so the test isolates the list-wrapping regression rather than scalar File hashing. NPIPE-204
Summary
Fixes three defects where
ListLogicalType-backed (list[T]/set[T]) Arrow extension columns were mishandled downstream:ListLogicalType.get_polars_extension_type()was not passingmetadata=tomake_polars_extension_type, soext_metadata()returnedNone. Polars exportedb''onto_arrow(), which_deserializerejected withValueErrorduring Join and MergeJoin round-trips.SemanticHashingVisitor.visit_extensionshort-circuited onnot isinstance(python_type, type)forlist[File](atypes.GenericAlias), so raw JSON path strings were hashed instead of file contents.MergeJoin.binary_static_processusedpa.array(merged_vals)which inferred type from raw storage, producinglarge_list(storage_type)instead ofextension<list[orcapod.file]>for merged logical-type columns.Changes
src/orcapod/logical_types/list_logical_type_factory.pymetadata=self._metadata_bytes.decode("utf-8")tomake_polars_extension_typesrc/orcapod/hashing/visitors.pyvisit_extensionbefore theisinstanceguard; delegate to_visit_list_elements; handlesset[T],list[list[T]], and arbitrary nestingsrc/orcapod/core/operators/merge_join.pypa.ExtensionArray.from_storage(ListLogicalType, ...)tests/test_logical_types/test_list_logical_type.pytests/test_core/operators/test_operators.pyTestJoinWithListExtensionColumntests/test_core/operators/test_merge_join.pyTestMergeJoinWithListExtensionColumn+TestMergeJoinLogicalTypeColumns(Fix 3 scalar + nested cases)tests/test_hashing/test_extension_type_hashing.pyTestListExtensionHashing(9 tests:list[File],set[File],list[list[File]], struct field, passthrough, content-change, determinism, symmetry)Test plan
uv run pytest tests/)uv run pytest tests/test_logical_types/— Fix 1 unit testsuv run pytest tests/test_hashing/test_extension_type_hashing.py::TestListExtensionHashing— Fix 2 regression testsuv run pytest tests/test_core/operators/test_merge_join.py::TestMergeJoinLogicalTypeColumns— Fix 3 regression testsuv run pytest tests/test_core/operators/— full operators suite including test-ordering safetyFixes ITL-627
🤖 Generated with Claude Code