From 8fd3252de91c0f392e860702b2b496bada4134de Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 00:39:49 +0000 Subject: [PATCH 01/12] docs(specs): add ITL-627 design spec for list-backed extension type fixes 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 --- ...6-08-22-itl-627-list-extension-metadata.md | 195 ++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md diff --git a/superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md b/superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md new file mode 100644 index 00000000..99e5c6b1 --- /dev/null +++ b/superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md @@ -0,0 +1,195 @@ +# ITL-627: Extension-type metadata dropped for list-backed logical types + +**Date:** 2026-08-22 +**Issue:** [ITL-627](https://linear.app/metamorphic/issue/ITL-627) + +--- + +## Overview + +`ListLogicalType` (ITL-173, PR #251) wraps `list[T]` / `set[T]` columns in an Arrow +extension type (`extension`, storage `large_list(large_string)`). +Two downstream paths mishandle that outer extension type: + +1. **Join raises** — `list[]` extension columns cannot survive the Polars round-trip + that `Join` (and `MergeJoin`) perform. After a successful `pl.DataFrame(table)` import, + `df.to_arrow()` calls `_deserialize` with empty metadata bytes and raises `ValueError`. + +2. **Aggregation silently loses content hashing** — `SemanticHashingVisitor.visit_extension` + falls through to passthrough for `list[File]` extension columns because `list[File]` is a + `types.GenericAlias`, not an instance of `type`, so the existing guard short-circuits. The + list elements are therefore hashed as JSON path strings rather than file contents. + +--- + +## Root Cause Analysis + +### Defect 1 — Polars round-trip drops metadata bytes + +`ListLogicalType.get_polars_extension_type()` calls `make_polars_extension_type` **without** +the `metadata=` argument: + +```python +# list_logical_type_factory.py (current — broken) +polars_ext_class = make_polars_extension_type( + self._logical_type_name, + self._storage_type, + # metadata= not passed → defaults to None +) +``` + +This means `pl.BaseExtension.__init__` stores `metadata=None`, so `ext_metadata()` returns +`None`. When Polars calls `to_arrow()` it exports the extension name correctly but passes +`b''` as the metadata bytes. PyArrow's `_import_from_c` calls `_deserialize` with those +empty bytes: + +``` +_deserialize("list[orcapod.path]", b'') +# b'' != b'{"category": "list", "element_ext_name": "orcapod.path", ...}' +# → ValueError +``` + +**Observed stack:** `polars/dataframe/frame.py to_arrow` → `pyarrow Array._import_from_c` +→ `orcapod/logical_types/registry.py _deserialize`. + +### Defect 2 — Content hashing bypassed for list-backed extension columns + +`SemanticHashingVisitor.visit_extension` checks `isinstance(python_type, type)` before +dispatching to the semantic handler: + +```python +# visitors.py (current — broken) +python_type = self._type_converter.arrow_type_to_python_type(extension_type) +if python_type is typing.Any or not isinstance(python_type, type): + return extension_type, storage_value # ← list[File] exits here +``` + +`list[File]` is a `types.GenericAlias`, so `isinstance(list[File], type)` is `False`. +The column passes through unchanged. `normalize_extension_columns` then exposes the raw +`large_list(large_string)` storage to Starfix, which hashes the JSON path strings — not +file contents. + +For contrast, `list>` (plain list with extension element type) +**already works**: `visit_list` → `_visit_list_elements` → `visit_extension` per scalar +element → `FileHandler`. The fix for the extension-wrapped form must produce identical +output. + +--- + +## Fixes + +### Fix 1 — `list_logical_type_factory.py` (one line) + +Pass the metadata bytes (decoded as a string) to `make_polars_extension_type`: + +```python +polars_ext_class = make_polars_extension_type( + self._logical_type_name, + self._storage_type, + metadata=self._metadata_bytes.decode("utf-8"), # ← add this +) +``` + +With `ext_metadata()` now returning the JSON string, Polars encodes it to UTF-8 bytes on +`to_arrow()`, PyArrow receives the correct bytes, and `_deserialize` validates successfully. +The extension type is preserved through the full Polars round-trip. + +Covers both `list[T]` and `set[T]` (same code path, same fix). + +### Fix 2 — `visitors.py` + +Extend `SemanticHashingVisitor.visit_extension` to detect list-backed extension types and +delegate to element-by-element visiting: + +``` +extension → large_list(extension) → large_list(large_binary) + ↑ virtual type ↑ per-element content hashes +``` + +**Algorithm:** + +1. After resolving `python_type = type_converter.arrow_type_to_python_type(extension_type)`, + check `typing.get_origin(python_type) in (list, set)` AND + `pa.types.is_large_list(extension_type.storage_type)`. + +2. Extract `elem_python_type = typing.get_args(python_type)[0]`. + +3. Check `isinstance(elem_python_type, type)` and `type_handler_registry.has_handler(elem_python_type)`. + If no handler → fall through to the existing passthrough (unchanged behaviour). + +4. Get `elem_arrow_type = type_converter.python_type_to_arrow_type(elem_python_type)`. + If it is not a `pa.ExtensionType` → fall through. + +5. Construct `virtual_list_type = pa.large_list(elem_arrow_type)`. + +6. Return `self._visit_list_elements(virtual_list_type, storage_value)`. + +`_visit_list_elements` visits each element via `self.visit(elem_arrow_type, item)` → +`visit_extension` scalar path → per-element content hash bytes → returns +`(pa.large_list(pa.large_binary()), [hash_bytes_0, hash_bytes_1, …])`. + +**Invariant (symmetry):** the `i`-th element of the returned list is byte-for-byte +identical to the result `visit_extension` would return for the same element in isolation +as a scalar `extension` column. This means `extension` +and `list>` produce identical per-element hash tokens for the +same file contents. + +--- + +## Tests + +### Defect 1 tests — `tests/test_core/operators/test_operators.py` + +**`test_join_preserves_list_extension_column`** +- Two streams: one with `list[Path]` data column, one with a scalar data column, shared tag. +- Join them via `Join.static_process`. +- Assert no error. +- Assert the output column type is still `extension` (not downgraded to plain `large_list`). + +**`test_merge_join_preserves_list_extension_column`** (in `test_merge_join.py`) +- Two streams: one has a non-colliding `list[Path]` data column and a shared tag; the + other has a different non-colliding data column and the same tag. +- MergeJoin them — no colliding data columns, so the `list[Path]` column passes through. +- Assert no error and assert the output column type is still `extension` + (same verification as the Join test, exercising MergeJoin's Polars round-trip). + +### Defect 2 tests — `tests/test_hashing/test_extension_type_hashing.py` + +**`test_list_file_extension_hashed_to_list_of_large_binary`** +- Create a `extension` column with two real files. +- Call `visitor.visit(ext_type, storage_value)`. +- Assert `new_type == pa.large_list(pa.large_binary())`. +- Assert `new_data` is a list of two `bytes` objects. + +**`test_list_file_extension_content_change_changes_hash`** +- Two runs: same file path, first with content `"v1"`, then `"v2"`. +- Assert the per-element hash bytes differ. + +**`test_list_file_extension_same_content_same_hash`** +- Two files at different paths with identical content. +- Assert their per-element hash bytes are equal. + +**`test_list_file_element_hash_matches_scalar_hash`** ← explicit contract test +- Scalar `orcapod.file` column with one file → scalar `visit_extension` → produces `hash_bytes_scalar`. +- `extension` column with the same single file as a list `[file]` → `visit_extension` → produces `[hash_bytes_list_elem]`. +- Assert `hash_bytes_list_elem == hash_bytes_scalar`. +- This pins the symmetry invariant: a file inside a list hashes the same way as a standalone file. + +**`test_list_file_extension_passthrough_when_no_handler`** +- Use a `SemanticAwarePythonHasher` with an empty registry (no `FileHandler`). +- Assert `visit_extension` returns the extension type and storage unchanged. + +**`test_list_path_extension_passthrough`** +- `extension` column (Path has no content handler). +- Assert passthrough: returned type is still the extension type, data unchanged. + +--- + +## Out of scope + +- `set[T]` semantic hashing — Defect 2 fix covers it mechanically (same code path), but no + additional tests are added beyond what is listed; `set[File]` is not a practical use case. +- Nested `list[list[T]]` — not supported by `ListLogicalType` and not addressed here. +- Defect 2 for `MergeJoin` — `MergeJoin` builds new list columns from Python values via + `pa.array(merged_vals)`, which produces a plain list type (no extension wrapper), so the + existing `_visit_list_elements` path already handles it correctly. From 08b50d98a18a311a1a14273202257e0146888140 Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:26:38 +0000 Subject: [PATCH 02/12] docs(specs): expand ITL-627 spec scope and add implementation plan - 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 --- ...6-08-22-itl-627-list-extension-metadata.md | 898 ++++++++++++++++++ ...6-08-22-itl-627-list-extension-metadata.md | 201 +++- 2 files changed, 1075 insertions(+), 24 deletions(-) create mode 100644 superpowers/plans/2026-08-22-itl-627-list-extension-metadata.md diff --git a/superpowers/plans/2026-08-22-itl-627-list-extension-metadata.md b/superpowers/plans/2026-08-22-itl-627-list-extension-metadata.md new file mode 100644 index 00000000..f5d037b4 --- /dev/null +++ b/superpowers/plans/2026-08-22-itl-627-list-extension-metadata.md @@ -0,0 +1,898 @@ +# ITL-627: List-Extension Metadata — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use sensei:subagent-driven-development (recommended) or sensei:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix three defects where `ListLogicalType`-backed extension columns are mishandled by Join, the semantic hashing visitor, and MergeJoin. + +**Architecture:** Three independent fixes in three files (`list_logical_type_factory.py`, `visitors.py`, `merge_join.py`) plus test coverage in a new `tests/test_logical_types/` directory and additions to existing test files. Each task is independently committable. + +**Tech Stack:** PyArrow extension types, Polars C data interface, Python `typing` module generics, `get_default_context()` for type converter access. + +**Spec:** `superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md` + +--- + +## File Map + +| File | Action | Purpose | +|---|---|---| +| `tests/test_logical_types/__init__.py` | Create | Make new package importable | +| `tests/test_logical_types/test_list_logical_type.py` | Create | Unit regression tests for Fix 1 | +| `src/orcapod/logical_types/list_logical_type_factory.py` | Modify line 151 | Fix 1: pass `metadata=` | +| `tests/test_core/operators/test_operators.py` | Modify | Integration test for Fix 1 via Join | +| `tests/test_core/operators/test_merge_join.py` | Modify | Integration test for Fix 1 via MergeJoin + Fix 3 tests | +| `src/orcapod/hashing/visitors.py` | Modify `visit_extension` | Fix 2: delegate list-backed extension to `_visit_list_elements` | +| `tests/test_hashing/test_extension_type_hashing.py` | Modify | Fix 2 regression + symmetry tests | +| `src/orcapod/core/operators/merge_join.py` | Modify `binary_static_process` | Fix 3: produce extension-typed merged arrays | + +--- + +## Task 1: Fix 1 — pass metadata to `make_polars_extension_type` + +**Files:** +- Create: `tests/test_logical_types/__init__.py` +- Create: `tests/test_logical_types/test_list_logical_type.py` +- Modify: `src/orcapod/logical_types/list_logical_type_factory.py:149-155` + +- [ ] **Step 1.1: Create the new test package** + +```bash +touch tests/test_logical_types/__init__.py +``` + +- [ ] **Step 1.2: Write the failing unit tests** + +Create `tests/test_logical_types/test_list_logical_type.py`: + +```python +"""Regression tests for ListLogicalType Polars extension type metadata. + +Defect 1 (ITL-627): get_polars_extension_type() was called without passing +metadata= to make_polars_extension_type, so ext_metadata() returned None. +Polars exported b'' on to_arrow(), which _deserialize rejected with ValueError. +""" + +from __future__ import annotations + +import json + +import pytest + +from orcapod.logical_types.builtin_logical_types import LogicalPath +from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + +class TestListLogicalTypePolarsMetadata: + def test_list_polars_ext_carries_metadata(self): + """get_polars_extension_type() for list[Path] must carry JSON metadata. + + With the buggy code (metadata= not passed), ext_metadata() returns None. + Polars then exports b'' on to_arrow(), causing _deserialize to raise + ValueError because b'' != the expected JSON bytes. + """ + lt = ListLogicalType(LogicalPath(), is_set=False) + polars_ext = lt.get_polars_extension_type() + + meta = polars_ext.ext_metadata() + assert meta is not None, ( + "ext_metadata() returned None — metadata= was not passed to " + "make_polars_extension_type. This causes the Polars→Arrow round-trip to fail." + ) + parsed = json.loads(meta) + assert parsed["category"] == "list" + assert parsed["element_ext_name"] == "orcapod.path" + + def test_set_polars_ext_carries_metadata(self): + """Same one-line fix covers set[T] (identical code path, is_set=True).""" + lt = ListLogicalType(LogicalPath(), is_set=True) + polars_ext = lt.get_polars_extension_type() + + meta = polars_ext.ext_metadata() + assert meta is not None + parsed = json.loads(meta) + assert parsed["category"] == "set" + assert parsed["element_ext_name"] == "orcapod.path" + + def test_polars_to_arrow_round_trip_preserves_extension_type(self): + """Full Polars round-trip must not raise and must preserve the extension type. + + pl.DataFrame(table).to_arrow() calls _deserialize; without the fix it + receives b'' and raises ValueError. + """ + import pyarrow as pa + import polars as pl + + lt = ListLogicalType(LogicalPath(), is_set=False) + ext_type = lt.get_arrow_extension_type() + + storage = pa.array([["/a.txt", "/b.txt"]], type=pa.large_list(pa.large_string())) + ext_array = pa.ExtensionArray.from_storage(ext_type, storage) + table = pa.table({"paths": ext_array}) + + # Must not raise ValueError from _deserialize + result = pl.DataFrame(table).to_arrow() + + assert isinstance(result.schema.field("paths").type, pa.ExtensionType) + assert result.schema.field("paths").type.extension_name == "list[orcapod.path]" +``` + +- [ ] **Step 1.3: Run to verify tests fail** + +```bash +uv run pytest tests/test_logical_types/test_list_logical_type.py -v +``` + +Expected: 3 FAILED — `ext_metadata()` returns `None`, round-trip raises `ValueError`. + +- [ ] **Step 1.4: Apply Fix 1** + +In `src/orcapod/logical_types/list_logical_type_factory.py`, modify `get_polars_extension_type` (lines 149–155): + +```python + def get_polars_extension_type(self) -> pl.BaseExtension: + """Return the cached Polars extension type for this list/set logical type. + + Returns: + A cached ``pl.BaseExtension`` instance registered under the logical + type name. + """ + if self._polars_ext is None: + polars_ext_class = make_polars_extension_type( + self._logical_type_name, + self._storage_type, + metadata=self._metadata_bytes.decode("utf-8"), + ) + self._polars_ext = polars_ext_class() + return self._polars_ext +``` + +The only change is adding `metadata=self._metadata_bytes.decode("utf-8")` on the line after `self._storage_type,`. + +- [ ] **Step 1.5: Run to verify tests pass** + +```bash +uv run pytest tests/test_logical_types/test_list_logical_type.py -v +``` + +Expected: 3 PASSED. + +- [ ] **Step 1.6: Commit** + +```bash +git add tests/test_logical_types/__init__.py \ + tests/test_logical_types/test_list_logical_type.py \ + src/orcapod/logical_types/list_logical_type_factory.py +git commit -m "fix(logical_types): pass metadata to make_polars_extension_type in ListLogicalType + +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)." +``` + +--- + +## Task 2: Defect 1 integration tests — Join and MergeJoin preserve list extension column + +**Files:** +- Modify: `tests/test_core/operators/test_operators.py` +- Modify: `tests/test_core/operators/test_merge_join.py` + +- [ ] **Step 2.1: Add the Join integration test** + +In `tests/test_core/operators/test_operators.py`, add a new test class after `TestJoinBehavior`: + +```python +class TestJoinWithListExtensionColumn: + """Regression tests for ITL-627 Defect 1: Join Polars round-trip with list extension columns.""" + + def test_join_preserves_list_extension_column(self): + """Join must not raise and must preserve extension. + + Before Fix 1, df.to_arrow() inside static_process called _deserialize + with b'' (no metadata), raising ValueError. + """ + import pyarrow as pa + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=False) + ext_type = lt.get_arrow_extension_type() + + storage = pa.array( + [["/a.txt", "/b.txt"], ["/c.txt"]], + type=pa.large_list(pa.large_string()), + ) + ext_array = pa.ExtensionArray.from_storage(ext_type, storage) + left_table = pa.table({ + "animal": pa.array(["cat", "dog"], type=pa.large_string()), + "paths": ext_array, + }) + left_stream = ArrowTableStream(left_table, tag_columns=["animal"]) + + right_table = pa.table({ + "animal": pa.array(["cat", "dog"], type=pa.large_string()), + "speed": pa.array([30.0, 45.0], type=pa.float64()), + }) + right_stream = ArrowTableStream(right_table, tag_columns=["animal"]) + + op = Join() + result = op.static_process(left_stream, right_stream) # must not raise + out_table = result.as_table() + + paths_type = out_table.schema.field("paths").type + assert isinstance(paths_type, pa.ExtensionType), ( + f"'paths' column must remain an extension type, got {paths_type}" + ) + assert paths_type.extension_name == "list[orcapod.path]" +``` + +- [ ] **Step 2.2: Add the MergeJoin round-trip test (non-colliding list column)** + +In `tests/test_core/operators/test_merge_join.py`, add a new test class: + +```python +class TestMergeJoinWithListExtensionColumn: + """Regression for ITL-627 Defect 1: MergeJoin Polars round-trip with list extension columns.""" + + def test_merge_join_preserves_non_colliding_list_extension_column(self): + """MergeJoin must not raise and must preserve extension + for a non-colliding list[Path] data column. + + MergeJoin also does a Polars round-trip; without Fix 1 it raises ValueError. + """ + import pyarrow as pa + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=False) + ext_type = lt.get_arrow_extension_type() + + storage = pa.array( + [["/a.txt", "/b.txt"], ["/c.txt"]], + type=pa.large_list(pa.large_string()), + ) + ext_array = pa.ExtensionArray.from_storage(ext_type, storage) + + # Left stream: list[Path] data column (non-colliding) + shared tag + left_table = pa.table({ + "id": pa.array([1, 2], type=pa.int64()), + "paths": ext_array, + }) + left_stream = ArrowTableStream(left_table, tag_columns=["id"]) + + # Right stream: different non-colliding data column + same tag + right_table = pa.table({ + "id": pa.array([1, 2], type=pa.int64()), + "score": pa.array([10.0, 20.0], type=pa.float64()), + }) + right_stream = ArrowTableStream(right_table, tag_columns=["id"]) + + result = MergeJoin().static_process(left_stream, right_stream) # must not raise + out_table = result.as_table() + + paths_type = out_table.schema.field("paths").type + assert isinstance(paths_type, pa.ExtensionType), ( + f"'paths' column must remain an extension type, got {paths_type}" + ) + assert paths_type.extension_name == "list[orcapod.path]" +``` + +- [ ] **Step 2.3: Run both new tests to verify they pass (Fix 1 already applied)** + +```bash +uv run pytest tests/test_core/operators/test_operators.py::TestJoinWithListExtensionColumn \ + tests/test_core/operators/test_merge_join.py::TestMergeJoinWithListExtensionColumn \ + -v +``` + +Expected: 2 PASSED (Fix 1 from Task 1 already resolves these). + +- [ ] **Step 2.4: Commit** + +```bash +git add tests/test_core/operators/test_operators.py \ + tests/test_core/operators/test_merge_join.py +git commit -m "test(operators): add regression tests for list extension column round-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." +``` + +--- + +## Task 3: Fix 2 — semantic hashing for list-backed extension columns + +**Files:** +- Modify: `src/orcapod/hashing/visitors.py` — `SemanticHashingVisitor.visit_extension` +- Modify: `tests/test_hashing/test_extension_type_hashing.py` + +- [ ] **Step 3.1: Write the failing hashing tests** + +Add a new test class `TestListExtensionHashing` to `tests/test_hashing/test_extension_type_hashing.py`: + +```python +class TestListExtensionHashing: + """Regression and contract tests for ITL-627 Defect 2. + + Before Fix 2, visit_extension 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. + """ + + def _make_list_file_ext_type(self, ctx): + """Return the extension Arrow type via the type converter.""" + from orcapod.logical_types.file_type import File + ctx.type_converter.register_python_class(list[File]) + return ctx.type_converter.python_type_to_arrow_type(list[File]) + + def _make_scalar_file_ext_type(self, ctx): + """Return the extension Arrow type.""" + from orcapod.logical_types.file_type import File + return ctx.type_converter.register_python_class(File) + + def _file_storage(self, ctx, path): + """Return the large_string storage value for a File.""" + from orcapod.logical_types.file_type import File + return ctx.type_converter.python_to_storage(File(path), File) + + def test_list_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): + """visit_extension for extension must return + (large_list(large_binary), [bytes, bytes]) — not the extension type unchanged. + + With the buggy code the isinstance guard exits immediately, returning + (extension_type, storage_value). This assertion on new_type would fail. + """ + f0 = tmp_path / "f0.txt"; f0.write_text("alpha") + f1 = tmp_path / "f1.txt"; f1.write_text("beta") + + list_ext_type = self._make_list_file_ext_type(ctx) + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + storage_value = [s0, s1] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(list_ext_type, storage_value) + + import pyarrow as pa + assert new_type == pa.large_list(pa.large_binary()), ( + f"Expected large_list(large_binary), got {new_type}. " + "Buggy code returns the extension type unchanged." + ) + assert isinstance(new_data, list) + assert len(new_data) == 2 + assert all(isinstance(b, bytes) for b in new_data) + + def test_list_file_extension_is_hash_of_file_content_hashes(self, ctx, tmp_path): + """Each element of the list result equals the scalar visit result for the same file. + + Contract: visit(list_ext, [s0, s1])[1][i] == visit(scalar_ext, si)[1] + This is the symmetry invariant — list[File] and scalar File hash identically + per element. Starfix then sees an ordered list of content-hash tokens. + """ + f0 = tmp_path / "contract0.txt"; f0.write_text("content zero") + f1 = tmp_path / "contract1.txt"; f1.write_text("content one") + + scalar_ext_type = self._make_scalar_file_ext_type(ctx) + list_ext_type = self._make_list_file_ext_type(ctx) + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + + # Scalar hashes + _, h0_bytes = visitor.visit(scalar_ext_type, s0) + _, h1_bytes = visitor.visit(scalar_ext_type, s1) + + # List hash + _, list_result = visitor.visit(list_ext_type, [s0, s1]) + + assert list_result[0] == h0_bytes, ( + "Element 0 of list result must equal the scalar hash of file 0" + ) + assert list_result[1] == h1_bytes, ( + "Element 1 of list result must equal the scalar hash of file 1" + ) + + def test_list_file_extension_content_change_changes_hash(self, ctx, tmp_path): + """Changing file content changes the per-element hash.""" + f = tmp_path / "mutable.txt" + f.write_text("v1") + + list_ext_type = self._make_list_file_ext_type(ctx) + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + + s_v1 = self._file_storage(ctx, f) + _, result_v1 = visitor.visit(list_ext_type, [s_v1]) + r0_v1 = result_v1[0] + + f.write_text("v2") + from orcapod.logical_types.file_type import File + s_v2 = ctx.type_converter.python_to_storage(File(f), File) + _, result_v2 = visitor.visit(list_ext_type, [s_v2]) + r0_v2 = result_v2[0] + + assert r0_v1 != r0_v2, "Content change must change the per-element hash" + + def test_list_file_extension_same_content_same_hash(self, ctx, tmp_path): + """Two files with identical content produce identical per-element hashes.""" + fa = tmp_path / "a.txt"; fa.write_text("identical") + fb = tmp_path / "b.txt"; fb.write_text("identical") + + list_ext_type = self._make_list_file_ext_type(ctx) + sa = self._file_storage(ctx, fa) + sb = self._file_storage(ctx, fb) + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + _, result_a = visitor.visit(list_ext_type, [sa]) + _, result_b = visitor.visit(list_ext_type, [sb]) + + assert result_a[0] == result_b[0], ( + "Same content at different paths must produce the same hash" + ) + + def test_list_file_extension_passthrough_when_no_handler(self, ctx, tmp_path): + """When the registry has no FileHandler, visit_extension must passthrough.""" + from orcapod.hashing.semantic_hashing.type_handler_registry import PythonTypeHandlerRegistry + from orcapod.hashing.semantic_hashing.semantic_hasher import SemanticAwarePythonHasher + + empty_registry = PythonTypeHandlerRegistry() + stripped_hasher = SemanticAwarePythonHasher( + hasher_id="test_v0", + type_handler_registry=empty_registry, + ) + + f = tmp_path / "f.txt"; f.write_text("test") + list_ext_type = self._make_list_file_ext_type(ctx) + storage_value = [self._file_storage(ctx, f)] + + visitor = SemanticHashingVisitor(ctx.type_converter, stripped_hasher) + new_type, new_data = visitor.visit(list_ext_type, storage_value) + + assert new_type == list_ext_type, "Must passthrough extension type when no handler" + assert new_data == storage_value, "Must passthrough storage value when no handler" + + def test_list_path_extension_passthrough(self, ctx, tmp_path): + """extension must passthrough — Path has no content handler.""" + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + import pyarrow as pa + from pathlib import Path + + lt = ListLogicalType(LogicalPath(), is_set=False) + list_ext_type = lt.get_arrow_extension_type() + storage_value = ["/a.txt", "/b.txt"] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(list_ext_type, storage_value) + + assert new_type == list_ext_type, "Path has no handler — must passthrough" + assert new_data == storage_value + + def test_set_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): + """set[File] (extension) also hashes per element. + + get_origin(set[File]) is `set`, covered by `in (list, set)` in Fix 2. + """ + from orcapod.logical_types.file_type import File + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + from orcapod.logical_types.file_type import LogicalFile + import pyarrow as pa + + f0 = tmp_path / "s0.txt"; f0.write_text("set alpha") + f1 = tmp_path / "s1.txt"; f1.write_text("set beta") + + lt = ListLogicalType(LogicalFile(), is_set=True) + set_ext_type = lt.get_arrow_extension_type() + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + storage_value = [s0, s1] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(set_ext_type, storage_value) + + assert new_type == pa.large_list(pa.large_binary()) + assert isinstance(new_data, list) + assert len(new_data) == 2 + assert all(isinstance(b, bytes) for b in new_data) + + def test_list_list_file_extension_hashed_recursively(self, ctx, tmp_path): + """extension recurses: each inner list becomes large_list(large_binary). + + Fix 2 recurses naturally: outer visit_extension delegates to _visit_list_elements + with virtual_type=large_list(extension), which calls + visit(extension, inner_list) for each element, which + recurses back into visit_extension. + """ + from orcapod.logical_types.file_type import File, LogicalFile + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + import pyarrow as pa + + f0 = tmp_path / "n0.txt"; f0.write_text("nested zero") + f1 = tmp_path / "n1.txt"; f1.write_text("nested one") + f2 = tmp_path / "n2.txt"; f2.write_text("nested two") + + inner_lt = ListLogicalType(LogicalFile(), is_set=False) + outer_lt = ListLogicalType(inner_lt, is_set=False) + outer_ext_type = outer_lt.get_arrow_extension_type() + + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + s2 = self._file_storage(ctx, f2) + # One row: [[s0, s1], [s2]] + storage_value = [[s0, s1], [s2]] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(outer_ext_type, storage_value) + + assert new_type == pa.large_list(pa.large_list(pa.large_binary())), ( + f"Expected large_list(large_list(large_binary)), got {new_type}" + ) + assert len(new_data) == 2 + assert len(new_data[0]) == 2 # two files in first inner list + assert len(new_data[1]) == 1 # one file in second inner list + assert all(isinstance(b, bytes) for b in new_data[0]) + assert all(isinstance(b, bytes) for b in new_data[1]) + + def test_dataclass_with_list_file_field_hashed(self, ctx, tmp_path): + """A Dataclass stored as a struct whose field is list[File] must hash per element. + + visit_struct recurses into fields via visit(field_type, field_data). + The list[File] field has type extension, so visit + dispatches to visit_extension, which Fix 2 handles correctly. + """ + from dataclasses import dataclass + from orcapod.logical_types.file_type import File + import pyarrow as pa + + @dataclass + class FileBundle: + name: str + files: list[File] + + # Register the dataclass with the type converter + ctx.type_converter.register_python_class(FileBundle) + arrow_type = ctx.type_converter.python_type_to_arrow_type(FileBundle) + + f0 = tmp_path / "dc0.txt"; f0.write_text("dc alpha") + f1 = tmp_path / "dc1.txt"; f1.write_text("dc beta") + + s0 = ctx.type_converter.python_to_storage(File(f0), File) + s1 = ctx.type_converter.python_to_storage(File(f1), File) + storage_value = {"name": "bundle", "files": [s0, s1]} + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(arrow_type, storage_value) + + # The `files` field should be hashed to large_list(large_binary) + files_field_type = new_type.field("files").type + assert files_field_type == pa.large_list(pa.large_binary()), ( + f"files field must be large_list(large_binary), got {files_field_type}" + ) + files_hashes = new_data["files"] + assert len(files_hashes) == 2 + assert all(isinstance(b, bytes) for b in files_hashes) +``` + +- [ ] **Step 3.2: Run to verify all new tests fail** + +```bash +uv run pytest tests/test_hashing/test_extension_type_hashing.py::TestListExtensionHashing -v +``` + +Expected: All FAILED — the `isinstance(python_type, type)` guard short-circuits for `list[File]`. + +- [ ] **Step 3.3: Apply Fix 2** + +In `src/orcapod/hashing/visitors.py`, replace the full `visit_extension` method of `SemanticHashingVisitor` (lines 196–231): + +```python + def visit_extension( + self, + extension_type: "pa.ExtensionType", + storage_value: Any, + ) -> tuple["pa.DataType", Any]: + """Hash an extension type value to pa.large_binary(), or passthrough. + + For list-backed extension types (e.g. ``extension``), + delegates to ``_visit_list_elements`` with a virtual + ``large_list(elem_ext_type)`` so that each element is hashed identically + to the scalar ``visit_extension`` path. This covers ``list[T]``, + ``set[T]``, and arbitrary nesting depth via recursion. + """ + if storage_value is None: + return extension_type, None + + # Resolve extension type → Python type. + python_type = self._type_converter.arrow_type_to_python_type(extension_type) + + # Detect list-backed extension types: extension, + # extension, etc. list[File] is a types.GenericAlias + # (not isinstance(..., type)), so the guard below would incorrectly skip it. + # 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) + and pa.types.is_large_list(extension_type.storage_type) + ): + args = typing.get_args(python_type) + if args: + elem_python_type = args[0] + if ( + isinstance(elem_python_type, type) + and self._python_hasher.type_handler_registry.has_handler( + elem_python_type + ) + ): + elem_arrow_type = self._type_converter.python_type_to_arrow_type( + elem_python_type + ) + virtual_list_type = pa.large_list(elem_arrow_type) + return self._visit_list_elements(virtual_list_type, storage_value) + + # If the converter couldn't resolve to a concrete class, passthrough. + if python_type is typing.Any or not isinstance(python_type, type): + return extension_type, storage_value + + # Only hash if a semantic hasher is registered for this Python type. + if not self._python_hasher.type_handler_registry.has_handler(python_type): + return extension_type, storage_value + + # Convert storage value → Python object and hash it. + python_obj = self._type_converter.storage_to_python(storage_value, python_type) + content_hash = self._python_hasher.hash_object(python_obj) + + # Encode as binary: ":::" + # Dots in the extension name → colons (e.g. "orcapod.path" → "orcapod:path"). + # The "::" separator is unambiguous because to_prefixed_digest() uses only ":". + type_name = extension_type.extension_name.replace(".", ":") + hash_bytes = ( + type_name.encode("utf-8") + + b"::" + + content_hash.to_prefixed_digest() + ) + return pa.large_binary(), hash_bytes +``` + +- [ ] **Step 3.4: Run to verify new tests pass** + +```bash +uv run pytest tests/test_hashing/test_extension_type_hashing.py -v +``` + +Expected: All PASSED (new tests + pre-existing tests). + +- [ ] **Step 3.5: Commit** + +```bash +git add src/orcapod/hashing/visitors.py \ + tests/test_hashing/test_extension_type_hashing.py +git commit -m "fix(hashing): hash list-backed extension columns element-by-element + +SemanticHashingVisitor.visit_extension 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 detects list/set-backed extension types and delegates to +_visit_list_elements with a virtual large_list(elem_ext_type), producing +per-element content hashes identical to the scalar visit path. Recurses +naturally for nested list[list[T]]. + +Fixes ITL-627 (Defect 2)." +``` + +--- + +## Task 4: Fix 3 — MergeJoin produces extension-typed merged arrays + +**Files:** +- Modify: `src/orcapod/core/operators/merge_join.py` — `binary_static_process` +- Modify: `tests/test_core/operators/test_merge_join.py` + +- [ ] **Step 4.1: Write the failing MergeJoin tests** + +Add `TestMergeJoinLogicalTypeColumns` to `tests/test_core/operators/test_merge_join.py`: + +```python +class TestMergeJoinLogicalTypeColumns: + """Regression tests for ITL-627 Defect 3. + + Before Fix 3, pa.array(merged_vals) inferred the array type from raw storage + values, producing plain large_list(storage_type) — the extension wrapper was lost. + """ + + def test_merge_join_scalar_logical_type_column_yields_list_extension(self, tmp_path): + """Merging a File column must produce extension, not large_list. + + Before Fix 3: pa.array([[json1, json2]]) inferred large_list(large_string). + After Fix 3: pa.ExtensionArray.from_storage(list_file_ext, ...) gives the correct type. + """ + import pyarrow as pa + from orcapod.logical_types.file_type import File, LogicalFile + from orcapod.contexts import get_default_context + + f1 = tmp_path / "mj1.txt"; f1.write_text("merge left") + f2 = tmp_path / "mj2.txt"; f2.write_text("merge right") + + ctx = get_default_context() + scalar_lt = LogicalFile() + ext_type = scalar_lt.get_arrow_extension_type() + + s1 = ctx.type_converter.python_to_storage(File(f1), File) + s2 = ctx.type_converter.python_to_storage(File(f2), File) + + left_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "file": pa.ExtensionArray.from_storage( + ext_type, pa.array([s1], type=pa.large_string()) + ), + }) + right_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "file": pa.ExtensionArray.from_storage( + ext_type, pa.array([s2], type=pa.large_string()) + ), + }) + left_stream = ArrowTableStream(left_table, tag_columns=["id"]) + right_stream = ArrowTableStream(right_table, tag_columns=["id"]) + + result = MergeJoin().static_process(left_stream, right_stream) + out_table = result.as_table() + + file_type = out_table.schema.field("file").type + assert isinstance(file_type, pa.ExtensionType), ( + f"'file' column must be extension, got {file_type}. " + "Buggy code produces plain large_list(large_string)." + ) + assert file_type.extension_name == "list[orcapod.file]" + + # Values must be a list of two storage values + file_values = out_table.column("file").to_pylist() + assert len(file_values) == 1 # one row + assert len(file_values[0]) == 2 # two merged elements + + def test_merge_join_list_backed_column_yields_nested_list_extension(self, tmp_path): + """Merging a list[File] column must produce extension. + + Fix 3 handles this naturally: elem_python_type = list[File], + get_logical_type_for_python_type(list[list[File]]) = ListLogicalType(ListLogicalType(LogicalFile())). + """ + import pyarrow as pa + from orcapod.logical_types.file_type import File, LogicalFile + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + from orcapod.contexts import get_default_context + + f1 = tmp_path / "nl1.txt"; f1.write_text("nested left") + f2 = tmp_path / "nl2.txt"; f2.write_text("nested right") + + ctx = get_default_context() + inner_lt = ListLogicalType(LogicalFile(), is_set=False) + inner_ext_type = inner_lt.get_arrow_extension_type() + + s1 = ctx.type_converter.python_to_storage(File(f1), File) + s2 = ctx.type_converter.python_to_storage(File(f2), File) + + # Each row's "files" value is a list of one file-storage-value + left_storage = pa.array([[s1]], type=pa.large_list(pa.large_string())) + right_storage = pa.array([[s2]], type=pa.large_list(pa.large_string())) + + left_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "files": pa.ExtensionArray.from_storage(inner_ext_type, left_storage), + }) + right_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "files": pa.ExtensionArray.from_storage(inner_ext_type, right_storage), + }) + left_stream = ArrowTableStream(left_table, tag_columns=["id"]) + right_stream = ArrowTableStream(right_table, tag_columns=["id"]) + + result = MergeJoin().static_process(left_stream, right_stream) + out_table = result.as_table() + + files_type = out_table.schema.field("files").type + assert isinstance(files_type, pa.ExtensionType), ( + f"'files' column must be extension, got {files_type}" + ) + assert files_type.extension_name == "list[list[orcapod.file]]" + + # One row with two inner lists + files_values = out_table.column("files").to_pylist() + assert len(files_values) == 1 + assert len(files_values[0]) == 2 +``` + +- [ ] **Step 4.2: Run to verify tests fail** + +```bash +uv run pytest tests/test_core/operators/test_merge_join.py::TestMergeJoinLogicalTypeColumns -v +``` + +Expected: 2 FAILED — column type is plain `large_list(large_string)`, not the extension type. + +- [ ] **Step 4.3: Apply Fix 3** + +In `src/orcapod/core/operators/merge_join.py`, make two changes to `binary_static_process`: + +**Change A** — add the colliding column type snapshot just before the Polars join (after the `output_nullable` block, around line 206). Insert before the `COMMON_JOIN_KEY` block: + +```python + # Snapshot Arrow types of colliding columns BEFORE the Polars round-trip. + # The round-trip may strip or alter extension metadata; we need the original + # element type to reconstruct the correct list extension type after merging. + colliding_col_types: dict[str, "pa.DataType"] = { + col: left_table.schema.field(col).type + for col in colliding_keys + if col in left_table.schema.names + } +``` + +**Change B** — replace the `merged_array = pa.array(merged_vals)` line (around line 276) with: + +```python + elem_arrow_type = colliding_col_types.get(col) + if elem_arrow_type is not None and isinstance(elem_arrow_type, pa.ExtensionType): + from orcapod.contexts import get_default_context + tc = get_default_context().type_converter + elem_python_type = tc.arrow_type_to_python_type(elem_arrow_type) + list_lt = tc.get_logical_type_for_python_type(list[elem_python_type]) + if list_lt is not None: + list_ext_type = list_lt.get_arrow_extension_type() + storage_array = pa.array(merged_vals, type=list_ext_type.storage_type) + merged_array = pa.ExtensionArray.from_storage(list_ext_type, storage_array) + else: + merged_array = pa.array(merged_vals) + else: + merged_array = pa.array(merged_vals) +``` + +- [ ] **Step 4.4: Run to verify new tests pass** + +```bash +uv run pytest tests/test_core/operators/test_merge_join.py -v +``` + +Expected: All PASSED. + +- [ ] **Step 4.5: Run the full test suite to check for regressions** + +```bash +uv run pytest tests/ -v --tb=short +``` + +Expected: All PASSED. + +- [ ] **Step 4.6: Commit** + +```bash +git add src/orcapod/core/operators/merge_join.py \ + tests/test_core/operators/test_merge_join.py +git commit -m "fix(operators): MergeJoin produces extension-typed list when merging 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)." +``` + +--- + +## Self-Review + +**Spec coverage:** +- Defect 1 fix ✓ (Task 1) + unit test ✓ + integration tests ✓ (Task 2) +- Defect 1 `test_list_logical_type_polars_ext_carries_metadata` ✓ +- Defect 2 fix ✓ (Task 3) + all 8 hashing tests ✓ +- Defect 3 fix ✓ (Task 4) + scalar and nested MergeJoin tests ✓ +- `set[File]` test ✓ +- `list[list[File]]` hashing test ✓ +- Dataclass with `list[File]` field test ✓ +- `list[File] × list[File]` MergeJoin test ✓ + +**No placeholders:** All test code and implementation code is complete. + +**Type consistency:** `list_lt`, `list_ext_type`, `elem_arrow_type` are consistent across Fix 3 Part A and Part B. diff --git a/superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md b/superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md index 99e5c6b1..e0432ca3 100644 --- a/superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md +++ b/superpowers/specs/2026-08-22-itl-627-list-extension-metadata.md @@ -20,6 +20,14 @@ Two downstream paths mishandle that outer extension type: `types.GenericAlias`, not an instance of `type`, so the existing guard short-circuits. The list elements are therefore hashed as JSON path strings rather than file contents. +3. **MergeJoin drops extension type when aggregating logical-type columns** — + `MergeJoin.binary_static_process` builds merged arrays via `pa.array(merged_vals)`, which + infers the element type from raw storage values and produces a plain `large_list(storage_type)`. + If the colliding column had type `extension`, the merged output should be + `extension`, but instead becomes `large_list(large_binary)`. The schema + prediction in `binary_output_schema` has the same problem: it predicts `list[T]` as a plain + Python generic alias rather than the proper extension-wrapped form. + --- ## Root Cause Analysis @@ -74,6 +82,34 @@ For contrast, `list>` (plain list with extension element element → `FileHandler`. The fix for the extension-wrapped form must produce identical output. +### Defect 3 — MergeJoin drops extension type when aggregating logical-type columns + +`MergeJoin.binary_static_process` (line 276 of `merge_join.py`) builds merged arrays via: + +```python +# merge_join.py (current — broken) +merged_array = pa.array(merged_vals) +``` + +`merged_vals` is a list of lists of raw storage values (e.g. `[[b"..json..", b"..json.."]]` +for a `File` column). PyArrow infers the array type from the values, producing +`large_list(large_binary)` — the `extension` wrapper is never applied. + +`binary_output_schema` has the same problem at line 104: + +```python +# merge_join.py (current — broken) +merged_data_schema[key] = list[colliding_schema[key]] +``` + +This produces a plain `list[T]` Python generic alias regardless of whether `T` is a logical +type. The predicted output schema therefore diverges from the actual merged column type for +logical-type columns. + +MergeJoin currently has no access to the type converter (`UniversalTypeConverter`). The fix +requires using `get_default_context().type_converter` to resolve the element extension type +and look up the corresponding `ListLogicalType`. + --- ## Fixes @@ -118,7 +154,6 @@ extension → large_list(extension) → la If no handler → fall through to the existing passthrough (unchanged behaviour). 4. Get `elem_arrow_type = type_converter.python_type_to_arrow_type(elem_python_type)`. - If it is not a `pa.ExtensionType` → fall through. 5. Construct `virtual_list_type = pa.large_list(elem_arrow_type)`. @@ -134,19 +169,87 @@ as a scalar `extension` column. This means `extension>` produce identical per-element hash tokens for the same file contents. +**Recursive correctness:** Fix 2 handles `extension` without +additional code. `visit_extension` detects the outer list, constructs +`virtual_list_type = large_list(extension)`, and calls +`_visit_list_elements`. Each inner element is `extension`, so +`visit(extension, inner_list)` dispatches back to `visit_extension` → +recurses. The result is `large_list(large_list(large_binary))` with per-element hashes +at every nesting level. + +### Fix 3 — `merge_join.py` + +`binary_output_schema` is already correct: it stores Python types (`list[File]` for a merged +`File` column), and `Schema` is `Mapping[str, Python type]`. No change needed there. + +The only site that needs fixing is **`binary_static_process`** — specifically the array +construction after merging. The fix has two parts: + +**Part A** — before the Polars round-trip, snapshot the Arrow type of each colliding column: + +```python +# Capture Arrow types of colliding columns BEFORE the Polars round-trip. +# The round-trip may strip extension metadata; we need the original type +# to reconstruct the correct list extension type after merging. +colliding_col_types: dict[str, pa.DataType] = { + col: left_table.schema.field(col).type + for col in colliding_keys + if col in left_table.schema.names +} +``` + +**Part B** — after computing `merged_vals`, build the merged array with the correct type: + +```python +# Replace the left column with merged list, drop right column. +elem_arrow_type = colliding_col_types.get(col) +if elem_arrow_type is not None and isinstance(elem_arrow_type, pa.ExtensionType): + from orcapod.contexts import get_default_context + tc = get_default_context().type_converter + elem_python_type = tc.arrow_type_to_python_type(elem_arrow_type) + list_lt = tc.get_logical_type_for_python_type(list[elem_python_type]) + if list_lt is not None: + list_ext_type = list_lt.get_arrow_extension_type() + storage_array = pa.array(merged_vals, type=list_ext_type.storage_type) + merged_array = pa.ExtensionArray.from_storage(list_ext_type, storage_array) + else: + merged_array = pa.array(merged_vals) +else: + merged_array = pa.array(merged_vals) +``` + +This handles the nested case naturally: when `elem_arrow_type` is +`extension`, `elem_python_type` becomes `list[File]`, +`get_logical_type_for_python_type(list[list[File]])` returns +`ListLogicalType(ListLogicalType(LogicalFile()))`, and the merged array is +`extension` — correct for the `list[File] × list[File]` case. + --- ## Tests -### Defect 1 tests — `tests/test_core/operators/test_operators.py` +### Defect 1 tests + +**`test_list_logical_type_polars_ext_carries_metadata`** — `tests/test_logical_types/test_list_logical_type.py` -**`test_join_preserves_list_extension_column`** +This is the direct regression test for the root cause. It targets the exact line that was +missing (`metadata=` not passed to `make_polars_extension_type`) without needing the full +join pipeline. + +- Instantiate `ListLogicalType(LogicalPath(), is_set=False)`. +- Call `get_polars_extension_type().ext_metadata()`. +- Assert the result is not `None`. +- Parse it as JSON; assert `"category" == "list"` and `"element_ext_name" == "orcapod.path"`. +- With the buggy code, `ext_metadata()` returns `None` immediately — this test fails before + a single Arrow operation is performed. + +**`test_join_preserves_list_extension_column`** — `tests/test_core/operators/test_operators.py` - Two streams: one with `list[Path]` data column, one with a scalar data column, shared tag. - Join them via `Join.static_process`. - Assert no error. - Assert the output column type is still `extension` (not downgraded to plain `large_list`). -**`test_merge_join_preserves_list_extension_column`** (in `test_merge_join.py`) +**`test_merge_join_preserves_list_extension_column`** — `tests/test_core/operators/test_merge_join.py` - Two streams: one has a non-colliding `list[Path]` data column and a shared tag; the other has a different non-colliding data column and the same tag. - MergeJoin them — no colliding data columns, so the `list[Path]` column passes through. @@ -156,40 +259,90 @@ same file contents. ### Defect 2 tests — `tests/test_hashing/test_extension_type_hashing.py` **`test_list_file_extension_hashed_to_list_of_large_binary`** -- Create a `extension` column with two real files. -- Call `visitor.visit(ext_type, storage_value)`. +- Create an `extension` column with two real files. +- Call `visitor.visit(ext_type, storage_value)` for one row. - Assert `new_type == pa.large_list(pa.large_binary())`. -- Assert `new_data` is a list of two `bytes` objects. +- Assert `new_data` is a Python list of exactly two `bytes` objects. + +**`test_list_file_extension_is_hash_of_file_content_hashes`** ← explicit contract test +- Create two files with distinct content. Build a list storage value `[s0, s1]`. +- Call scalar `visit_extension(orcapod.file_ext_type, s0)` → `h0_bytes`. +- Call scalar `visit_extension(orcapod.file_ext_type, s1)` → `h1_bytes`. +- Call `visit_extension(list_file_ext_type, [s0, s1])` → `list_type, [r0, r1]`. +- Assert `r0 == h0_bytes` and `r1 == h1_bytes`. +- Meaning: the list result at position `i` is byte-for-byte the content hash of file `i`, + identical to what scalar hashing of the same file produces. The Starfix table hasher + therefore sees an ordered list of per-file content hashes — a single table hash derived + from hashing a list of hash values. **`test_list_file_extension_content_change_changes_hash`** -- Two runs: same file path, first with content `"v1"`, then `"v2"`. -- Assert the per-element hash bytes differ. +- Write one file with content `"v1"`, build storage value for the single-element list. +- Compute the list visit result; capture `r0_v1`. +- Overwrite the file with `"v2"`, rebuild `File` (re-validates existence), recompute storage. +- Compute the list visit result; capture `r0_v2`. +- Assert `r0_v1 != r0_v2` — a content change propagates into the per-element hash. **`test_list_file_extension_same_content_same_hash`** - Two files at different paths with identical content. -- Assert their per-element hash bytes are equal. - -**`test_list_file_element_hash_matches_scalar_hash`** ← explicit contract test -- Scalar `orcapod.file` column with one file → scalar `visit_extension` → produces `hash_bytes_scalar`. -- `extension` column with the same single file as a list `[file]` → `visit_extension` → produces `[hash_bytes_list_elem]`. -- Assert `hash_bytes_list_elem == hash_bytes_scalar`. -- This pins the symmetry invariant: a file inside a list hashes the same way as a standalone file. +- Assert the per-element hash bytes for each are equal. **`test_list_file_extension_passthrough_when_no_handler`** - Use a `SemanticAwarePythonHasher` with an empty registry (no `FileHandler`). -- Assert `visit_extension` returns the extension type and storage unchanged. +- Assert `visit_extension` for `extension` returns the extension type + and storage value unchanged — the no-handler branch falls through cleanly. **`test_list_path_extension_passthrough`** -- `extension` column (Path has no content handler). +- `extension` column (Path has no content handler in the default context). - Assert passthrough: returned type is still the extension type, data unchanged. +**`test_set_file_extension_hashed_to_list_of_large_binary`** +- Create an `extension` column with two real files. +- Call `visitor.visit(ext_type, storage_value)` for one row. +- Assert `new_type == pa.large_list(pa.large_binary())`. +- Assert `new_data` is a list of exactly two `bytes` objects — same invariant as for `list[File]`. + (`get_origin(set[File]) is set`, covered by Fix 2 condition `in (list, set)`.) + +**`test_list_list_file_extension_hashed_recursively`** +- Create an `extension` column: a list of two inner file-lists. +- Call `visitor.visit(outer_ext_type, [[s0, s1], [s2]])` for one row. +- Assert outer `new_type == pa.large_list(pa.large_list(pa.large_binary()))`. +- Assert `new_data[0]` equals `[h0_bytes, h1_bytes]` and `new_data[1]` equals `[h2_bytes]` + where each `hi_bytes` is the scalar content hash for file `i`. Proves Fix 2 recurses. + +**`test_dataclass_with_list_file_field_hashed`** +- Define a Dataclass (or use an existing registered one) with a `files: list[File]` field. +- The Dataclass is stored as a struct Arrow type; the `files` field has type + `extension`. +- Call `visitor.visit(struct_type, {"files": [s0, s1], ...})`. +- Assert the `files` field in the result has type `pa.large_list(pa.large_binary())` and + the values are content hash bytes — confirms `visit_struct` → `visit` → `visit_extension` + correctly delegates for nested extension-typed struct fields. + +### Defect 3 tests — `tests/test_core/operators/test_merge_join.py` + +**`test_merge_join_scalar_logical_type_column_yields_list_extension`** +- Two streams: both have a `File` data column and a shared tag; different files in each stream. +- MergeJoin them (colliding `File` column). +- Assert no error. +- Assert the output `File` column type is `extension` (not plain `large_list`). + +**`test_merge_join_schema_prediction_for_logical_type_column`** +- Call `output_schema()` on a MergeJoin whose inputs both have a `File` data column. +- Assert the predicted output type for that column is `extension`. +- Proves `binary_output_schema` is consistent with `binary_static_process`. + +**`test_merge_join_list_backed_column_yields_nested_list_extension`** +- Two streams: both have a `list[File]` data column (type `extension`) + and a shared tag; different file lists in each stream. +- MergeJoin them (colliding `list[File]` column). +- Assert no error. +- Assert the output column type is `extension`. +- Fix 3's algorithm handles this naturally: `elem_python_type = list[File]` → + `list_lt = ListLogicalType(ListLogicalType(LogicalFile()))` → same code path as scalar case. + --- ## Out of scope -- `set[T]` semantic hashing — Defect 2 fix covers it mechanically (same code path), but no - additional tests are added beyond what is listed; `set[File]` is not a practical use case. -- Nested `list[list[T]]` — not supported by `ListLogicalType` and not addressed here. -- Defect 2 for `MergeJoin` — `MergeJoin` builds new list columns from Python values via - `pa.array(merged_vals)`, which produces a plain list type (no extension wrapper), so the - existing `_visit_list_elements` path already handles it correctly. +- Nested `list[list[T]]` MergeJoin where nesting depth exceeds two (e.g. `list[list[list[T]]]` + × `list[list[list[T]]]`). Not a practical use case. From 3b9071dff9ff6f0b5627ed381499d4de6da674db Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:35:14 +0000 Subject: [PATCH 03/12] fix(logical_types): pass metadata to make_polars_extension_type in ListLogicalType 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). --- .../list_logical_type_factory.py | 1 + .../test_list_logical_type.py | 85 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/src/orcapod/logical_types/list_logical_type_factory.py b/src/orcapod/logical_types/list_logical_type_factory.py index f0798171..78cc11ae 100644 --- a/src/orcapod/logical_types/list_logical_type_factory.py +++ b/src/orcapod/logical_types/list_logical_type_factory.py @@ -150,6 +150,7 @@ def get_polars_extension_type(self) -> pl.BaseExtension: polars_ext_class = make_polars_extension_type( self._logical_type_name, self._storage_type, + metadata=self._metadata_bytes.decode("utf-8"), ) self._polars_ext = polars_ext_class() return self._polars_ext diff --git a/tests/test_logical_types/test_list_logical_type.py b/tests/test_logical_types/test_list_logical_type.py index a66b8243..df06fb5f 100644 --- a/tests/test_logical_types/test_list_logical_type.py +++ b/tests/test_logical_types/test_list_logical_type.py @@ -286,3 +286,88 @@ def test_list_logical_type_factory_reconstruct_raises_on_missing_element_ext_nam factory.reconstruct_from_arrow( "list[orcapod.uuid]", pa.large_list(pa.large_binary()), metadata, _StubConverter() ) + + +# ── Regression tests for ITL-627 (Defect 1) ────────────────────────────────── + + +class TestListLogicalTypePolarsMetadata: + """Regression tests for ListLogicalType Polars extension type metadata. + + Defect 1 (ITL-627): get_polars_extension_type() was called without passing + metadata= to make_polars_extension_type, so ext_metadata() returned None. + Polars exported b'' on to_arrow(), which _deserialize rejected with ValueError. + """ + + def test_list_polars_ext_carries_metadata(self): + """get_polars_extension_type() for list[Path] must carry JSON metadata. + + With the buggy code (metadata= not passed), ext_metadata() returns None. + Polars then exports b'' on to_arrow(), causing _deserialize to raise + ValueError because b'' != the expected JSON bytes. + """ + import json + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=False) + polars_ext = lt.get_polars_extension_type() + + meta = polars_ext.ext_metadata() + assert meta is not None, ( + "ext_metadata() returned None — metadata= was not passed to " + "make_polars_extension_type. This causes the Polars→Arrow round-trip to fail." + ) + parsed = json.loads(meta) + assert parsed["category"] == "list" + assert parsed["element_ext_name"] == "orcapod.path" + + def test_set_polars_ext_carries_metadata(self): + """Same one-line fix covers set[T] (identical code path, is_set=True).""" + import json + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=True) + polars_ext = lt.get_polars_extension_type() + + meta = polars_ext.ext_metadata() + assert meta is not None + parsed = json.loads(meta) + assert parsed["category"] == "set" + assert parsed["element_ext_name"] == "orcapod.path" + + def test_polars_to_arrow_round_trip_preserves_extension_type(self): + """Full Polars round-trip must not raise and must preserve the extension type. + + pl.DataFrame(table).to_arrow() calls _deserialize; without the fix it + receives b'' and raises ValueError. + """ + import polars as pl + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=False) + ext_type = lt.get_arrow_extension_type() + + # 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) + except pa.lib.ArrowKeyError: + pass # already registered + polars_ext = lt.get_polars_extension_type() + try: + pl.register_extension_type(ext_type.extension_name, type(polars_ext)) + except (ValueError, pl.exceptions.ComputeError): + pass # already registered + + storage = pa.array([["/a.txt", "/b.txt"]], type=pa.large_list(pa.large_string())) + ext_array = pa.ExtensionArray.from_storage(ext_type, storage) + table = pa.table({"paths": ext_array}) + + # Must not raise ValueError from _deserialize + result = pl.DataFrame(table).to_arrow() + + assert isinstance(result.schema.field("paths").type, pa.ExtensionType) + assert result.schema.field("paths").type.extension_name == "list[orcapod.path]" From 21676b8cce8497495d46c9b368f3ff302ad53a7d Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:41:17 +0000 Subject: [PATCH 04/12] test(operators): add regression tests for list extension column round-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. --- tests/test_core/operators/test_merge_join.py | 59 ++++++++++++++++++++ tests/test_core/operators/test_operators.py | 57 +++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/tests/test_core/operators/test_merge_join.py b/tests/test_core/operators/test_merge_join.py index a317697c..d8c674f8 100644 --- a/tests/test_core/operators/test_merge_join.py +++ b/tests/test_core/operators/test_merge_join.py @@ -853,3 +853,62 @@ def test_system_tag_values_sorted_for_same_pipeline_hash(self): for row_ab, row_ba in zip(rows_ab, rows_ba): for col in sys_cols: assert row_ab[col] == row_ba[col] + + +class TestMergeJoinWithListExtensionColumn: + """Regression for ITL-627 Defect 1: MergeJoin Polars round-trip with list extension columns.""" + + def test_merge_join_preserves_non_colliding_list_extension_column(self): + """MergeJoin must not raise and must preserve extension + for a non-colliding list[Path] data column. + + MergeJoin also does a Polars round-trip; without Fix 1 it raises ValueError. + """ + import polars as pl + import pyarrow as pa + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=False) + ext_type = lt.get_arrow_extension_type() + + # 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) + except pa.lib.ArrowKeyError: + pass # already registered + polars_ext = lt.get_polars_extension_type() + try: + pl.register_extension_type(ext_type.extension_name, type(polars_ext)) + except (ValueError, pl.exceptions.ComputeError): + pass # already registered + + storage = pa.array( + [["/a.txt", "/b.txt"], ["/c.txt"]], + type=pa.large_list(pa.large_string()), + ) + ext_array = pa.ExtensionArray.from_storage(ext_type, storage) + + # Left stream: list[Path] data column (non-colliding) + shared tag + left_table = pa.table({ + "id": pa.array([1, 2], type=pa.int64()), + "paths": ext_array, + }) + left_stream = ArrowTableStream(left_table, tag_columns=["id"]) + + # Right stream: different non-colliding data column + same tag + right_table = pa.table({ + "id": pa.array([1, 2], type=pa.int64()), + "score": pa.array([10.0, 20.0], type=pa.float64()), + }) + right_stream = ArrowTableStream(right_table, tag_columns=["id"]) + + result = MergeJoin().static_process(left_stream, right_stream) # must not raise + out_table = result.as_table() + + paths_type = out_table.schema.field("paths").type + assert isinstance(paths_type, pa.ExtensionType), ( + f"'paths' column must remain an extension type, got {paths_type}" + ) + assert paths_type.extension_name == "list[orcapod.path]" diff --git a/tests/test_core/operators/test_operators.py b/tests/test_core/operators/test_operators.py index 232276f9..c5066dc5 100644 --- a/tests/test_core/operators/test_operators.py +++ b/tests/test_core/operators/test_operators.py @@ -465,6 +465,63 @@ def test_join_is_commutative(self, simple_stream, disjoint_stream): assert isinstance(sym, frozenset) +class TestJoinWithListExtensionColumn: + """Regression tests for ITL-627 Defect 1: Join Polars round-trip with list extension columns.""" + + def test_join_preserves_list_extension_column(self): + """Join must not raise and must preserve extension. + + Before Fix 1, df.to_arrow() inside static_process called _deserialize + with b'' (no metadata), raising ValueError. + """ + import polars as pl + import pyarrow as pa + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=False) + ext_type = lt.get_arrow_extension_type() + + # 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) + except pa.lib.ArrowKeyError: + pass # already registered + polars_ext = lt.get_polars_extension_type() + try: + pl.register_extension_type(ext_type.extension_name, type(polars_ext)) + except (ValueError, pl.exceptions.ComputeError): + pass # already registered + + storage = pa.array( + [["/a.txt", "/b.txt"], ["/c.txt"]], + type=pa.large_list(pa.large_string()), + ) + ext_array = pa.ExtensionArray.from_storage(ext_type, storage) + left_table = pa.table({ + "animal": pa.array(["cat", "dog"], type=pa.large_string()), + "paths": ext_array, + }) + left_stream = ArrowTableStream(left_table, tag_columns=["animal"]) + + right_table = pa.table({ + "animal": pa.array(["cat", "dog"], type=pa.large_string()), + "speed": pa.array([30.0, 45.0], type=pa.float64()), + }) + right_stream = ArrowTableStream(right_table, tag_columns=["animal"]) + + op = Join() + result = op.static_process(left_stream, right_stream) # must not raise + out_table = result.as_table() + + paths_type = out_table.schema.field("paths").type + assert isinstance(paths_type, pa.ExtensionType), ( + f"'paths' column must remain an extension type, got {paths_type}" + ) + assert paths_type.extension_name == "list[orcapod.path]" + + class TestJoinMetaColumnCollision: """Verify that a 3-way join with identical meta columns on all inputs does not raise a DuplicateError. Instead, colliding meta columns should be renamed From 8791c061c69b77c7b884a25d1c599ae5ebb54077 Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:53:27 +0000 Subject: [PATCH 05/12] test(operators): add data integrity assertions and clean up registration 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 --- tests/test_core/operators/test_merge_join.py | 13 +++++++++++-- tests/test_core/operators/test_operators.py | 13 +++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/test_core/operators/test_merge_join.py b/tests/test_core/operators/test_merge_join.py index d8c674f8..1322921e 100644 --- a/tests/test_core/operators/test_merge_join.py +++ b/tests/test_core/operators/test_merge_join.py @@ -872,8 +872,10 @@ def test_merge_join_preserves_non_colliding_list_extension_column(self): lt = ListLogicalType(LogicalPath(), is_set=False) ext_type = lt.get_arrow_extension_type() - # Register the extension types with both Arrow and Polars registries so - # the round-trip can reconstruct the extension type (not fall back to storage). + # Registration required: ArrowTableStream does not trigger LogicalType + # registration, so without this Polars degrades the extension type to its + # storage type (large_list) during the operator's Polars + # round-trip, and the post-join Arrow table loses the extension wrapper. try: pa.register_extension_type(ext_type) except pa.lib.ArrowKeyError: @@ -912,3 +914,10 @@ def test_merge_join_preserves_non_colliding_list_extension_column(self): f"'paths' column must remain an extension type, got {paths_type}" ) assert paths_type.extension_name == "list[orcapod.path]" + + # Data integrity: 2 rows (inner join on id=1 and id=2) + assert len(out_table) == 2 + # Values are preserved + paths_values = out_table.column("paths").to_pylist() + assert len(paths_values) == 2 + assert any(len(row) >= 1 for row in paths_values) # each row has at least one path diff --git a/tests/test_core/operators/test_operators.py b/tests/test_core/operators/test_operators.py index c5066dc5..3dba4235 100644 --- a/tests/test_core/operators/test_operators.py +++ b/tests/test_core/operators/test_operators.py @@ -482,8 +482,10 @@ def test_join_preserves_list_extension_column(self): lt = ListLogicalType(LogicalPath(), is_set=False) ext_type = lt.get_arrow_extension_type() - # Register the extension types with both Arrow and Polars registries so - # the round-trip can reconstruct the extension type (not fall back to storage). + # Registration required: ArrowTableStream does not trigger LogicalType + # registration, so without this Polars degrades the extension type to its + # storage type (large_list) during the operator's Polars + # round-trip, and the post-join Arrow table loses the extension wrapper. try: pa.register_extension_type(ext_type) except pa.lib.ArrowKeyError: @@ -521,6 +523,13 @@ def test_join_preserves_list_extension_column(self): ) assert paths_type.extension_name == "list[orcapod.path]" + # Data integrity: 2 rows (inner join on "cat" and "dog") + assert len(out_table) == 2 + # Values are preserved + paths_values = out_table.column("paths").to_pylist() + assert len(paths_values) == 2 + assert any(len(row) >= 1 for row in paths_values) # each row has at least one path + class TestJoinMetaColumnCollision: """Verify that a 3-way join with identical meta columns on all inputs does not From ef49aeacf6a085c099c7aa741b2a35dc6f7a771f Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 01:56:11 +0000 Subject: [PATCH 06/12] test(operators): fix any->all in path value assertions --- tests/test_core/operators/test_merge_join.py | 2 +- tests/test_core/operators/test_operators.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_core/operators/test_merge_join.py b/tests/test_core/operators/test_merge_join.py index 1322921e..2bb89a02 100644 --- a/tests/test_core/operators/test_merge_join.py +++ b/tests/test_core/operators/test_merge_join.py @@ -920,4 +920,4 @@ def test_merge_join_preserves_non_colliding_list_extension_column(self): # Values are preserved paths_values = out_table.column("paths").to_pylist() assert len(paths_values) == 2 - assert any(len(row) >= 1 for row in paths_values) # each row has at least one path + assert all(len(row) >= 1 for row in paths_values) # each row has at least one path diff --git a/tests/test_core/operators/test_operators.py b/tests/test_core/operators/test_operators.py index 3dba4235..370885ce 100644 --- a/tests/test_core/operators/test_operators.py +++ b/tests/test_core/operators/test_operators.py @@ -528,7 +528,7 @@ def test_join_preserves_list_extension_column(self): # Values are preserved paths_values = out_table.column("paths").to_pylist() assert len(paths_values) == 2 - assert any(len(row) >= 1 for row in paths_values) # each row has at least one path + assert all(len(row) >= 1 for row in paths_values) # each row has at least one path class TestJoinMetaColumnCollision: From 450c65ce9db583b362422ade3fa198efd58e76e7 Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 02:20:30 +0000 Subject: [PATCH 07/12] fix(hashing): hash list-backed extension columns element-by-element MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/orcapod/hashing/visitors.py | 44 ++- .../test_extension_type_hashing.py | 287 ++++++++++++++++++ 2 files changed, 327 insertions(+), 4 deletions(-) diff --git a/src/orcapod/hashing/visitors.py b/src/orcapod/hashing/visitors.py index ec0382ac..456c1e6f 100644 --- a/src/orcapod/hashing/visitors.py +++ b/src/orcapod/hashing/visitors.py @@ -198,21 +198,57 @@ def visit_extension( extension_type: "pa.ExtensionType", storage_value: Any, ) -> tuple["pa.DataType", Any]: - """Hash an extension type value to pa.large_binary(), or passthrough.""" + """Hash an extension type value to pa.large_binary(), or passthrough. + + For list-backed extension types (e.g. ``extension``), + delegates to ``_visit_list_elements`` with a virtual + ``large_list(elem_ext_type)`` so that each element is hashed identically + to the scalar ``visit_extension`` path. This covers ``list[T]``, + ``set[T]``, and arbitrary nesting depth via recursion. + """ if storage_value is None: return extension_type, None # Resolve extension type → Python type. python_type = self._type_converter.arrow_type_to_python_type(extension_type) + # Detect list-backed extension types: extension, + # extension, etc. list[File] is a types.GenericAlias + # (not isinstance(..., type)), so the guard below would incorrectly skip it. + # 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) + and pa.types.is_large_list(extension_type.storage_type) + ): + args = typing.get_args(python_type) + if args: + elem_python_type = args[0] + # Check handler for plain element types (e.g. File). + has_handler = ( + isinstance(elem_python_type, type) + and self._python_hasher.type_handler_registry.has_handler(elem_python_type) + ) + # For generic alias element types (e.g. list[File] inside list[list[File]]), + # the element is itself a list/set — we should recurse so the inner + # visit_extension call can decide whether to hash or passthrough. + # Only recurse for nested list/set generic aliases, NOT for scalar + # extension types (e.g. File) that happen to map to a pa.ExtensionType — + # those must respect the has_handler check above. + is_nested_list_or_set = typing.get_origin(elem_python_type) in (list, set) + if has_handler or is_nested_list_or_set: + elem_arrow_type = self._type_converter.python_type_to_arrow_type( + elem_python_type + ) + virtual_list_type = pa.large_list(elem_arrow_type) + return self._visit_list_elements(virtual_list_type, storage_value) + # If the converter couldn't resolve to a concrete class, passthrough. if python_type is typing.Any or not isinstance(python_type, type): return extension_type, storage_value # Only hash if a semantic hasher is registered for this Python type. - if not self._python_hasher.type_handler_registry.has_handler( - python_type - ): + if not self._python_hasher.type_handler_registry.has_handler(python_type): return extension_type, storage_value # Convert storage value → Python object and hash it. diff --git a/tests/test_hashing/test_extension_type_hashing.py b/tests/test_hashing/test_extension_type_hashing.py index 0b02a8ac..890a4ce1 100644 --- a/tests/test_hashing/test_extension_type_hashing.py +++ b/tests/test_hashing/test_extension_type_hashing.py @@ -2,6 +2,8 @@ from __future__ import annotations +from dataclasses import dataclass + import pyarrow as pa import pytest from pathlib import Path @@ -11,6 +13,16 @@ from orcapod.contexts import get_default_context +# Module-level dataclass required for registration (local classes are rejected +# because they have no stable fully-qualified class name). +@dataclass +class FileBundle: + """Test dataclass with a list[File] field for TestListExtensionHashing.""" + + name: str + files: list[File] + + @pytest.fixture def ctx(): return get_default_context() @@ -224,3 +236,278 @@ def test_same_content_two_files_cross_path(self, ctx, tmp_path): prefixed_from_python = python_content_hash.to_prefixed_digest() assert prefixed_from_arrow == prefixed_from_python + + +class TestListExtensionHashing: + """Regression and contract tests for ITL-627 Defect 2. + + Before Fix 2, visit_extension 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. + """ + + def _make_list_file_ext_type(self, ctx): + """Return the extension Arrow type via the type converter.""" + from orcapod.logical_types.file_type import File + ctx.type_converter.register_python_class(list[File]) + return ctx.type_converter.python_type_to_arrow_type(list[File]) + + def _make_scalar_file_ext_type(self, ctx): + """Return the extension Arrow type.""" + from orcapod.logical_types.file_type import File + return ctx.type_converter.register_python_class(File) + + def _file_storage(self, ctx, path): + """Return the large_string storage value for a File.""" + from orcapod.logical_types.file_type import File + return ctx.type_converter.python_to_storage(File(path), File) + + def test_list_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): + """visit_extension for extension must return + (large_list(large_binary), [bytes, bytes]) — not the extension type unchanged. + + With the buggy code the isinstance guard exits immediately, returning + (extension_type, storage_value). This assertion on new_type would fail. + """ + f0 = tmp_path / "f0.txt"; f0.write_text("alpha") + f1 = tmp_path / "f1.txt"; f1.write_text("beta") + + list_ext_type = self._make_list_file_ext_type(ctx) + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + storage_value = [s0, s1] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(list_ext_type, storage_value) + + import pyarrow as pa + assert new_type == pa.large_list(pa.large_binary()), ( + f"Expected large_list(large_binary), got {new_type}. " + "Buggy code returns the extension type unchanged." + ) + assert isinstance(new_data, list) + assert len(new_data) == 2 + assert all(isinstance(b, bytes) for b in new_data) + + def test_list_file_extension_is_hash_of_file_content_hashes(self, ctx, tmp_path): + """Each element of the list result equals the scalar visit result for the same file. + + Contract: visit(list_ext, [s0, s1])[1][i] == visit(scalar_ext, si)[1] + This is the symmetry invariant — list[File] and scalar File hash identically + per element. Starfix then sees an ordered list of content-hash tokens. + """ + f0 = tmp_path / "contract0.txt"; f0.write_text("content zero") + f1 = tmp_path / "contract1.txt"; f1.write_text("content one") + + scalar_ext_type = self._make_scalar_file_ext_type(ctx) + list_ext_type = self._make_list_file_ext_type(ctx) + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + + # Scalar hashes + _, h0_bytes = visitor.visit(scalar_ext_type, s0) + _, h1_bytes = visitor.visit(scalar_ext_type, s1) + + # List hash + _, list_result = visitor.visit(list_ext_type, [s0, s1]) + + assert list_result[0] == h0_bytes, ( + "Element 0 of list result must equal the scalar hash of file 0" + ) + assert list_result[1] == h1_bytes, ( + "Element 1 of list result must equal the scalar hash of file 1" + ) + + def test_list_file_extension_content_change_changes_hash(self, ctx, tmp_path): + """Changing file content changes the per-element hash.""" + f = tmp_path / "mutable.txt" + f.write_text("v1") + + list_ext_type = self._make_list_file_ext_type(ctx) + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + + s_v1 = self._file_storage(ctx, f) + _, result_v1 = visitor.visit(list_ext_type, [s_v1]) + r0_v1 = result_v1[0] + + f.write_text("v2") + from orcapod.logical_types.file_type import File + s_v2 = ctx.type_converter.python_to_storage(File(f), File) + _, result_v2 = visitor.visit(list_ext_type, [s_v2]) + r0_v2 = result_v2[0] + + assert r0_v1 != r0_v2, "Content change must change the per-element hash" + + def test_list_file_extension_same_content_same_hash(self, ctx, tmp_path): + """Two files with identical content produce identical per-element hashes.""" + fa = tmp_path / "a.txt"; fa.write_text("identical") + fb = tmp_path / "b.txt"; fb.write_text("identical") + + list_ext_type = self._make_list_file_ext_type(ctx) + sa = self._file_storage(ctx, fa) + sb = self._file_storage(ctx, fb) + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + _, result_a = visitor.visit(list_ext_type, [sa]) + _, result_b = visitor.visit(list_ext_type, [sb]) + + assert result_a[0] == result_b[0], ( + "Same content at different paths must produce the same hash" + ) + + def test_list_file_extension_passthrough_when_no_handler(self, ctx, tmp_path): + """When the registry has no FileHandler, visit_extension must passthrough.""" + from orcapod.hashing.semantic_hashing.type_handler_registry import PythonTypeHandlerRegistry + from orcapod.hashing.semantic_hashing.semantic_hasher import SemanticAwarePythonHasher + + empty_registry = PythonTypeHandlerRegistry() + stripped_hasher = SemanticAwarePythonHasher( + hasher_id="test_v0", + type_handler_registry=empty_registry, + ) + + f = tmp_path / "f.txt"; f.write_text("test") + list_ext_type = self._make_list_file_ext_type(ctx) + storage_value = [self._file_storage(ctx, f)] + + visitor = SemanticHashingVisitor(ctx.type_converter, stripped_hasher) + new_type, new_data = visitor.visit(list_ext_type, storage_value) + + assert new_type == list_ext_type, "Must passthrough extension type when no handler" + assert new_data == storage_value, "Must passthrough storage value when no handler" + + def test_list_path_extension_passthrough(self, ctx, tmp_path): + """extension must passthrough — Path has no content handler.""" + from orcapod.logical_types.builtin_logical_types import LogicalPath + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + + lt = ListLogicalType(LogicalPath(), is_set=False) + list_ext_type = lt.get_arrow_extension_type() + storage_value = ["/a.txt", "/b.txt"] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(list_ext_type, storage_value) + + assert new_type == list_ext_type, "Path has no handler — must passthrough" + assert new_data == storage_value + + def test_set_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): + """set[File] (extension) also hashes per element. + + get_origin(set[File]) is `set`, covered by `in (list, set)` in Fix 2. + The type must be registered with the converter before visiting so that + ``arrow_type_to_python_type`` can resolve it. + """ + from orcapod.logical_types.file_type import File + import pyarrow as pa + + f0 = tmp_path / "s0.txt"; f0.write_text("set alpha") + f1 = tmp_path / "s1.txt"; f1.write_text("set beta") + + ctx.type_converter.register_python_class(set[File]) + set_ext_type = ctx.type_converter.python_type_to_arrow_type(set[File]) + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + storage_value = [s0, s1] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(set_ext_type, storage_value) + + assert new_type == pa.large_list(pa.large_binary()) + assert isinstance(new_data, list) + assert len(new_data) == 2 + assert all(isinstance(b, bytes) for b in new_data) + + def test_list_list_file_extension_hashed_recursively(self, ctx, tmp_path): + """extension recurses: each inner list becomes large_list(large_binary). + + Fix 2 recurses naturally: outer visit_extension delegates to _visit_list_elements + with virtual_type=large_list(extension), which calls + visit(extension, inner_list) for each element, which + recurses back into visit_extension. + + Both the inner and outer list types must be registered with the converter so + that ``arrow_type_to_python_type`` can resolve them. + """ + from orcapod.logical_types.file_type import File + import pyarrow as pa + + f0 = tmp_path / "n0.txt"; f0.write_text("nested zero") + f1 = tmp_path / "n1.txt"; f1.write_text("nested one") + f2 = tmp_path / "n2.txt"; f2.write_text("nested two") + + # Register both inner and outer list types so the converter knows them. + ctx.type_converter.register_python_class(list[File]) + ctx.type_converter.register_python_class(list[list[File]]) + outer_ext_type = ctx.type_converter.python_type_to_arrow_type(list[list[File]]) + + s0 = self._file_storage(ctx, f0) + s1 = self._file_storage(ctx, f1) + s2 = self._file_storage(ctx, f2) + # One row: [[s0, s1], [s2]] + storage_value = [[s0, s1], [s2]] + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(outer_ext_type, storage_value) + + assert new_type == pa.large_list(pa.large_list(pa.large_binary())), ( + f"Expected large_list(large_list(large_binary)), got {new_type}" + ) + assert len(new_data) == 2 + assert len(new_data[0]) == 2 # two files in first inner list + assert len(new_data[1]) == 1 # one file in second inner list + assert all(isinstance(b, bytes) for b in new_data[0]) + assert all(isinstance(b, bytes) for b in new_data[1]) + + def test_dataclass_with_list_file_field_hashed(self, ctx, tmp_path): + """A struct with a list[File] extension field must hash per element. + + When visiting a struct type whose ``files`` field is + ``extension``, ``visit_struct`` recurses into each + field via ``visit(field_type, field_data)``. Fix 2 handles the resulting + ``visit_extension`` dispatch correctly. + + Note: Dataclasses registered via ``register_python_class`` are backed by + Arrow extension types with struct storage. Because ``FileBundle`` has no + semantic handler, ``visit_extension`` passes through the extension type + unchanged and does NOT recurse into the struct's fields. To exercise + ``visit_struct`` → ``visit_extension(list[File])`` recursion we must + visit the *storage struct type* directly. + + Note: Uses module-level ``FileBundle`` — local dataclasses are rejected + because they have no stable fully-qualified class name. + """ + import pyarrow as pa + + # Register the dataclass to get its Arrow extension type. + ctx.type_converter.register_python_class(FileBundle) + ext_type = ctx.type_converter.python_type_to_arrow_type(FileBundle) + # The storage is a struct>. + # We need to reconstruct the struct type with the list[File] extension type + # for the `files` field so that visit_struct -> visit_extension(list[File]) fires. + list_file_ext_type = self._make_list_file_ext_type(ctx) + struct_type = pa.struct([ + pa.field("name", pa.large_utf8()), + pa.field("files", list_file_ext_type), + ]) + + f0 = tmp_path / "dc0.txt"; f0.write_text("dc alpha") + f1 = tmp_path / "dc1.txt"; f1.write_text("dc beta") + + s0 = ctx.type_converter.python_to_storage(File(f0), File) + s1 = ctx.type_converter.python_to_storage(File(f1), File) + storage_value = {"name": "bundle", "files": [s0, s1]} + + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + new_type, new_data = visitor.visit(struct_type, storage_value) + + # The `files` field should be hashed to large_list(large_binary) + files_field_type = new_type.field("files").type + assert files_field_type == pa.large_list(pa.large_binary()), ( + f"files field must be large_list(large_binary), got {files_field_type}" + ) + files_hashes = new_data["files"] + assert len(files_hashes) == 2 + assert all(isinstance(b, bytes) for b in files_hashes) From 1628db29eabda2236f3854fd7bfeb5f8b091dcf6 Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 02:25:47 +0000 Subject: [PATCH 08/12] refactor(hashing): move method-body imports to module level; improve docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- src/orcapod/hashing/visitors.py | 19 +++++++++- .../test_extension_type_hashing.py | 38 +++++++------------ 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/orcapod/hashing/visitors.py b/src/orcapod/hashing/visitors.py index 456c1e6f..dfae2264 100644 --- a/src/orcapod/hashing/visitors.py +++ b/src/orcapod/hashing/visitors.py @@ -198,13 +198,28 @@ def visit_extension( extension_type: "pa.ExtensionType", storage_value: Any, ) -> tuple["pa.DataType", Any]: - """Hash an extension type value to pa.large_binary(), or passthrough. + """Hash an extension type value to ``pa.large_binary()``, or passthrough. For list-backed extension types (e.g. ``extension``), delegates to ``_visit_list_elements`` with a virtual ``large_list(elem_ext_type)`` so that each element is hashed identically to the scalar ``visit_extension`` path. This covers ``list[T]``, ``set[T]``, and arbitrary nesting depth via recursion. + + Three passthrough cases (extension type and storage value returned unchanged): + - ``storage_value`` is ``None``. + - The Python type could not be resolved (``typing.Any`` or not a plain ``type``). + - The element type has no registered semantic handler and is not a nested list/set. + + Args: + extension_type: The Arrow extension type to process. + storage_value: The storage-level value (result of ``to_pylist()`` on the column). + + Returns: + Tuple of ``(new_arrow_type, new_data)``. For hashable scalar types returns + ``(pa.large_binary(), hash_bytes)``. For list/set-backed types returns + ``(pa.large_list(...), [hash_bytes, ...])``. Passthroughs return the + original ``(extension_type, storage_value)``. """ if storage_value is None: return extension_type, None @@ -222,6 +237,8 @@ def visit_extension( and pa.types.is_large_list(extension_type.storage_type) ): args = typing.get_args(python_type) + # Defensive guard: a well-formed list[T]/set[T] always has args, but if + # not, fall through to the isinstance(python_type, type) passthrough below. if args: elem_python_type = args[0] # Check handler for plain element types (e.g. File). diff --git a/tests/test_hashing/test_extension_type_hashing.py b/tests/test_hashing/test_extension_type_hashing.py index 890a4ce1..0d6a02bb 100644 --- a/tests/test_hashing/test_extension_type_hashing.py +++ b/tests/test_hashing/test_extension_type_hashing.py @@ -2,6 +2,7 @@ from __future__ import annotations +import uuid from dataclasses import dataclass import pyarrow as pa @@ -9,7 +10,11 @@ from pathlib import Path from orcapod.logical_types.file_type import File +from orcapod.logical_types.builtin_logical_types import LogicalPath +from orcapod.logical_types.list_logical_type_factory import ListLogicalType from orcapod.hashing.visitors import SemanticHashingVisitor +from orcapod.hashing.semantic_hashing.type_handler_registry import PythonTypeHandlerRegistry +from orcapod.hashing.semantic_hashing.semantic_hasher import SemanticAwarePythonHasher from orcapod.contexts import get_default_context @@ -137,10 +142,6 @@ def test_null_value_passthrough(self, ctx): def test_unregistered_python_type_passes_through(self, ctx): """Extension types with no registered semantic hasher pass through unchanged.""" - import uuid - from orcapod.hashing.semantic_hashing.type_handler_registry import PythonTypeHandlerRegistry - from orcapod.hashing.semantic_hashing.semantic_hasher import SemanticAwarePythonHasher - # Build a hasher with a registry that has NO entry for UUID empty_registry = PythonTypeHandlerRegistry() stripped_hasher = SemanticAwarePythonHasher( @@ -247,19 +248,22 @@ class TestListExtensionHashing: """ def _make_list_file_ext_type(self, ctx): - """Return the extension Arrow type via the type converter.""" - from orcapod.logical_types.file_type import File + """Return the ``extension`` Arrow type via the type converter. + + Idempotent: ``register_python_class`` is a no-op if the type is already registered. + """ ctx.type_converter.register_python_class(list[File]) return ctx.type_converter.python_type_to_arrow_type(list[File]) def _make_scalar_file_ext_type(self, ctx): - """Return the extension Arrow type.""" - from orcapod.logical_types.file_type import File + """Return the ``extension`` Arrow type. + + Idempotent: ``register_python_class`` is a no-op if the type is already registered. + """ return ctx.type_converter.register_python_class(File) def _file_storage(self, ctx, path): """Return the large_string storage value for a File.""" - from orcapod.logical_types.file_type import File return ctx.type_converter.python_to_storage(File(path), File) def test_list_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): @@ -280,7 +284,6 @@ def test_list_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path) visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) new_type, new_data = visitor.visit(list_ext_type, storage_value) - import pyarrow as pa assert new_type == pa.large_list(pa.large_binary()), ( f"Expected large_list(large_binary), got {new_type}. " "Buggy code returns the extension type unchanged." @@ -333,7 +336,6 @@ def test_list_file_extension_content_change_changes_hash(self, ctx, tmp_path): r0_v1 = result_v1[0] f.write_text("v2") - from orcapod.logical_types.file_type import File s_v2 = ctx.type_converter.python_to_storage(File(f), File) _, result_v2 = visitor.visit(list_ext_type, [s_v2]) r0_v2 = result_v2[0] @@ -359,9 +361,6 @@ def test_list_file_extension_same_content_same_hash(self, ctx, tmp_path): def test_list_file_extension_passthrough_when_no_handler(self, ctx, tmp_path): """When the registry has no FileHandler, visit_extension must passthrough.""" - from orcapod.hashing.semantic_hashing.type_handler_registry import PythonTypeHandlerRegistry - from orcapod.hashing.semantic_hashing.semantic_hasher import SemanticAwarePythonHasher - empty_registry = PythonTypeHandlerRegistry() stripped_hasher = SemanticAwarePythonHasher( hasher_id="test_v0", @@ -380,9 +379,6 @@ def test_list_file_extension_passthrough_when_no_handler(self, ctx, tmp_path): def test_list_path_extension_passthrough(self, ctx, tmp_path): """extension must passthrough — Path has no content handler.""" - from orcapod.logical_types.builtin_logical_types import LogicalPath - from orcapod.logical_types.list_logical_type_factory import ListLogicalType - lt = ListLogicalType(LogicalPath(), is_set=False) list_ext_type = lt.get_arrow_extension_type() storage_value = ["/a.txt", "/b.txt"] @@ -400,9 +396,6 @@ def test_set_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): The type must be registered with the converter before visiting so that ``arrow_type_to_python_type`` can resolve it. """ - from orcapod.logical_types.file_type import File - import pyarrow as pa - f0 = tmp_path / "s0.txt"; f0.write_text("set alpha") f1 = tmp_path / "s1.txt"; f1.write_text("set beta") @@ -431,9 +424,6 @@ def test_list_list_file_extension_hashed_recursively(self, ctx, tmp_path): Both the inner and outer list types must be registered with the converter so that ``arrow_type_to_python_type`` can resolve them. """ - from orcapod.logical_types.file_type import File - import pyarrow as pa - f0 = tmp_path / "n0.txt"; f0.write_text("nested zero") f1 = tmp_path / "n1.txt"; f1.write_text("nested one") f2 = tmp_path / "n2.txt"; f2.write_text("nested two") @@ -479,8 +469,6 @@ def test_dataclass_with_list_file_field_hashed(self, ctx, tmp_path): Note: Uses module-level ``FileBundle`` — local dataclasses are rejected because they have no stable fully-qualified class name. """ - import pyarrow as pa - # Register the dataclass to get its Arrow extension type. ctx.type_converter.register_python_class(FileBundle) ext_type = ctx.type_converter.python_type_to_arrow_type(FileBundle) From 601ace58928c49706231dc0a66c32feb7ef95ef5 Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 02:46:04 +0000 Subject: [PATCH 09/12] fix(operators): MergeJoin produces extension-typed list when merging 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). --- src/orcapod/core/operators/merge_join.py | 24 +++- tests/test_core/operators/test_merge_join.py | 119 +++++++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) diff --git a/src/orcapod/core/operators/merge_join.py b/src/orcapod/core/operators/merge_join.py index 43324f7d..2a93340e 100644 --- a/src/orcapod/core/operators/merge_join.py +++ b/src/orcapod/core/operators/merge_join.py @@ -182,6 +182,15 @@ def binary_static_process( # Find colliding data columns colliding_keys = set(left_data_keys) & set(right_data_keys) + # Snapshot Arrow types of colliding columns BEFORE the Polars round-trip. + # The round-trip may strip or alter extension metadata; we need the original + # element type to reconstruct the correct list extension type after merging. + colliding_col_types: dict[str, "pa.DataType"] = { + col: left_table.schema.field(col).type + for col in colliding_keys + if col in left_table.schema.names + } + # Capture nullable flags from input schemas BEFORE Polars conversion. # Polars' join discards nullable info (defaults all to True); we derive # the output schema from the inputs instead of from data null counts. @@ -273,7 +282,20 @@ def binary_static_process( joined = joined.drop(left_col_name) joined = joined.drop(right_col_name) - merged_array = pa.array(merged_vals) + elem_arrow_type = colliding_col_types.get(col) + if elem_arrow_type is not None and isinstance(elem_arrow_type, pa.ExtensionType): + from orcapod.contexts import get_default_context + tc = get_default_context().type_converter + elem_python_type = tc.arrow_type_to_python_type(elem_arrow_type) + list_lt = tc.get_logical_type_for_python_type(list[elem_python_type]) + if list_lt is not None: + list_ext_type = list_lt.get_arrow_extension_type() + storage_array = pa.array(merged_vals, type=list_ext_type.storage_type) + merged_array = pa.ExtensionArray.from_storage(list_ext_type, storage_array) + else: + merged_array = pa.array(merged_vals) + else: + merged_array = pa.array(merged_vals) joined = joined.add_column(col_idx, left_col_name, merged_array) if has_source: diff --git a/tests/test_core/operators/test_merge_join.py b/tests/test_core/operators/test_merge_join.py index 2bb89a02..7673c2c3 100644 --- a/tests/test_core/operators/test_merge_join.py +++ b/tests/test_core/operators/test_merge_join.py @@ -921,3 +921,122 @@ def test_merge_join_preserves_non_colliding_list_extension_column(self): paths_values = out_table.column("paths").to_pylist() assert len(paths_values) == 2 assert all(len(row) >= 1 for row in paths_values) # each row has at least one path + + +class TestMergeJoinLogicalTypeColumns: + """Regression tests for ITL-627 Defect 3. + + Before Fix 3, pa.array(merged_vals) inferred the array type from raw storage + values, producing plain large_list(storage_type) — the extension wrapper was lost. + """ + + def test_merge_join_scalar_logical_type_column_yields_list_extension(self, tmp_path): + """Merging a File column must produce extension, not large_list. + + Before Fix 3: pa.array([[json1, json2]]) inferred large_list(large_string). + After Fix 3: pa.ExtensionArray.from_storage(list_file_ext, ...) gives the correct type. + """ + import pyarrow as pa + from orcapod.logical_types.file_type import File, LogicalFile + from orcapod.contexts import get_default_context + + ctx = get_default_context() + # Registration required so that list[File] resolves to the extension type + # and Polars round-trips preserve the extension wrapper. + ctx.type_converter.register_python_class(File) + ctx.type_converter.register_python_class(list[File]) + + f1 = tmp_path / "mj1.txt"; f1.write_text("merge left") + f2 = tmp_path / "mj2.txt"; f2.write_text("merge right") + + scalar_lt = LogicalFile() + ext_type = scalar_lt.get_arrow_extension_type() + + s1 = ctx.type_converter.python_to_storage(File(f1), File) + s2 = ctx.type_converter.python_to_storage(File(f2), File) + + left_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "file": pa.ExtensionArray.from_storage( + ext_type, pa.array([s1], type=pa.large_string()) + ), + }) + right_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "file": pa.ExtensionArray.from_storage( + ext_type, pa.array([s2], type=pa.large_string()) + ), + }) + left_stream = ArrowTableStream(left_table, tag_columns=["id"]) + right_stream = ArrowTableStream(right_table, tag_columns=["id"]) + + result = MergeJoin().static_process(left_stream, right_stream) + out_table = result.as_table() + + file_type = out_table.schema.field("file").type + assert isinstance(file_type, pa.ExtensionType), ( + f"'file' column must be extension, got {file_type}. " + "Buggy code produces plain large_list(large_string)." + ) + assert file_type.extension_name == "list[orcapod.file]" + + # Values must be a list of two storage values + file_values = out_table.column("file").to_pylist() + assert len(file_values) == 1 # one row + assert len(file_values[0]) == 2 # two merged elements + + def test_merge_join_list_backed_column_yields_nested_list_extension(self, tmp_path): + """Merging a list[File] column must produce extension. + + Fix 3 handles this naturally: elem_python_type = list[File], + get_logical_type_for_python_type(list[list[File]]) = ListLogicalType(ListLogicalType(LogicalFile())). + """ + import pyarrow as pa + from orcapod.logical_types.file_type import File, LogicalFile + from orcapod.logical_types.list_logical_type_factory import ListLogicalType + from orcapod.contexts import get_default_context + + ctx = get_default_context() + # Registration required so that list[File] and list[list[File]] resolve to + # extension types and Polars round-trips preserve the extension wrapper. + ctx.type_converter.register_python_class(File) + ctx.type_converter.register_python_class(list[File]) + ctx.type_converter.register_python_class(list[list[File]]) + + f1 = tmp_path / "nl1.txt"; f1.write_text("nested left") + f2 = tmp_path / "nl2.txt"; f2.write_text("nested right") + + inner_lt = ListLogicalType(LogicalFile(), is_set=False) + inner_ext_type = inner_lt.get_arrow_extension_type() + + s1 = ctx.type_converter.python_to_storage(File(f1), File) + s2 = ctx.type_converter.python_to_storage(File(f2), File) + + # Each row's "files" value is a list of one file-storage-value + left_storage = pa.array([[s1]], type=pa.large_list(pa.large_string())) + right_storage = pa.array([[s2]], type=pa.large_list(pa.large_string())) + + left_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "files": pa.ExtensionArray.from_storage(inner_ext_type, left_storage), + }) + right_table = pa.table({ + "id": pa.array([1], type=pa.int64()), + "files": pa.ExtensionArray.from_storage(inner_ext_type, right_storage), + }) + left_stream = ArrowTableStream(left_table, tag_columns=["id"]) + right_stream = ArrowTableStream(right_table, tag_columns=["id"]) + + result = MergeJoin().static_process(left_stream, right_stream) + out_table = result.as_table() + + files_type = out_table.schema.field("files").type + assert isinstance(files_type, pa.ExtensionType), ( + f"'files' column must be extension, got {files_type}" + ) + assert files_type.extension_name == "list[list[orcapod.file]]" + + # One row with two inner lists + files_values = out_table.column("files").to_pylist() + assert len(files_values) == 1 + assert len(files_values[0]) == 2 From 6d545f59be1722af8da3ead400dda5581b004198 Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 02:48:53 +0000 Subject: [PATCH 10/12] refactor(operators): rename list_lt -> list_logical_type; add comments; 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. --- src/orcapod/core/operators/merge_join.py | 8 +++++--- tests/test_core/operators/test_merge_join.py | 8 ++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/orcapod/core/operators/merge_join.py b/src/orcapod/core/operators/merge_join.py index 2a93340e..6fe72829 100644 --- a/src/orcapod/core/operators/merge_join.py +++ b/src/orcapod/core/operators/merge_join.py @@ -284,12 +284,14 @@ def binary_static_process( elem_arrow_type = colliding_col_types.get(col) if elem_arrow_type is not None and isinstance(elem_arrow_type, pa.ExtensionType): + # 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 elem_python_type = tc.arrow_type_to_python_type(elem_arrow_type) - list_lt = tc.get_logical_type_for_python_type(list[elem_python_type]) - if list_lt is not None: - list_ext_type = list_lt.get_arrow_extension_type() + list_logical_type = tc.get_logical_type_for_python_type(list[elem_python_type]) + if list_logical_type is not None: + list_ext_type = list_logical_type.get_arrow_extension_type() storage_array = pa.array(merged_vals, type=list_ext_type.storage_type) merged_array = pa.ExtensionArray.from_storage(list_ext_type, storage_array) else: diff --git a/tests/test_core/operators/test_merge_join.py b/tests/test_core/operators/test_merge_join.py index 7673c2c3..7ffd5c92 100644 --- a/tests/test_core/operators/test_merge_join.py +++ b/tests/test_core/operators/test_merge_join.py @@ -982,8 +982,8 @@ def test_merge_join_scalar_logical_type_column_yields_list_extension(self, tmp_p # Values must be a list of two storage values file_values = out_table.column("file").to_pylist() - assert len(file_values) == 1 # one row - assert len(file_values[0]) == 2 # two merged elements + assert len(file_values) == 1, f"Expected 1 row, got {len(file_values)}" + assert len(file_values[0]) == 2, f"Expected 2 merged elements, got {len(file_values[0])}" def test_merge_join_list_backed_column_yields_nested_list_extension(self, tmp_path): """Merging a list[File] column must produce extension. @@ -1038,5 +1038,5 @@ def test_merge_join_list_backed_column_yields_nested_list_extension(self, tmp_pa # One row with two inner lists files_values = out_table.column("files").to_pylist() - assert len(files_values) == 1 - assert len(files_values[0]) == 2 + assert len(files_values) == 1, f"Expected 1 row, got {len(files_values)}" + assert len(files_values[0]) == 2, f"Expected 2 merged inner lists, got {len(files_values[0])}" From d0a9044179bdf0715ee21daf5db35639d5a0f4fd Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 03:16:39 +0000 Subject: [PATCH 11/12] fix(tests): use type-converter cache for list[Path] extension type in Join/MergeJoin regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/test_core/operators/test_merge_join.py | 26 +++++------------- tests/test_core/operators/test_operators.py | 28 ++++++-------------- 2 files changed, 15 insertions(+), 39 deletions(-) diff --git a/tests/test_core/operators/test_merge_join.py b/tests/test_core/operators/test_merge_join.py index 7ffd5c92..4935c5c0 100644 --- a/tests/test_core/operators/test_merge_join.py +++ b/tests/test_core/operators/test_merge_join.py @@ -864,27 +864,15 @@ def test_merge_join_preserves_non_colliding_list_extension_column(self): MergeJoin also does a Polars round-trip; without Fix 1 it raises ValueError. """ - import polars as pl import pyarrow as pa - from orcapod.logical_types.builtin_logical_types import LogicalPath - from orcapod.logical_types.list_logical_type_factory import ListLogicalType + from pathlib import Path + from orcapod.contexts import get_default_context - lt = ListLogicalType(LogicalPath(), is_set=False) - ext_type = lt.get_arrow_extension_type() - - # Registration required: ArrowTableStream does not trigger LogicalType - # registration, so without this Polars degrades the extension type to its - # storage type (large_list) during the operator's Polars - # round-trip, and the post-join Arrow table loses the extension wrapper. - try: - pa.register_extension_type(ext_type) - except pa.lib.ArrowKeyError: - pass # already registered - polars_ext = lt.get_polars_extension_type() - try: - pl.register_extension_type(ext_type.extension_name, type(polars_ext)) - except (ValueError, pl.exceptions.ComputeError): - pass # already registered + # Use the shared type-converter cache so both Arrow and Polars always see + # the same extension class object, avoiding ArrowTypeError on table.cast(). + ctx = get_default_context() + ctx.type_converter.register_python_class(list[Path]) + ext_type = ctx.type_converter.python_type_to_arrow_type(list[Path]) storage = pa.array( [["/a.txt", "/b.txt"], ["/c.txt"]], diff --git a/tests/test_core/operators/test_operators.py b/tests/test_core/operators/test_operators.py index 370885ce..d519d1c0 100644 --- a/tests/test_core/operators/test_operators.py +++ b/tests/test_core/operators/test_operators.py @@ -474,27 +474,15 @@ def test_join_preserves_list_extension_column(self): Before Fix 1, df.to_arrow() inside static_process called _deserialize with b'' (no metadata), raising ValueError. """ - import polars as pl import pyarrow as pa - from orcapod.logical_types.builtin_logical_types import LogicalPath - from orcapod.logical_types.list_logical_type_factory import ListLogicalType - - lt = ListLogicalType(LogicalPath(), is_set=False) - ext_type = lt.get_arrow_extension_type() - - # Registration required: ArrowTableStream does not trigger LogicalType - # registration, so without this Polars degrades the extension type to its - # storage type (large_list) during the operator's Polars - # round-trip, and the post-join Arrow table loses the extension wrapper. - try: - pa.register_extension_type(ext_type) - except pa.lib.ArrowKeyError: - pass # already registered - polars_ext = lt.get_polars_extension_type() - try: - pl.register_extension_type(ext_type.extension_name, type(polars_ext)) - except (ValueError, pl.exceptions.ComputeError): - pass # already registered + from pathlib import Path + from orcapod.contexts import get_default_context + + # Use the shared type-converter cache so both Arrow and Polars always see + # the same extension class object, avoiding ArrowTypeError on table.cast(). + ctx = get_default_context() + ctx.type_converter.register_python_class(list[Path]) + ext_type = ctx.type_converter.python_type_to_arrow_type(list[Path]) storage = pa.array( [["/a.txt", "/b.txt"], ["/c.txt"]], From fb2b0623fc906511e9b8213626e4e2c55863a0f6 Mon Sep 17 00:00:00 2001 From: "agent-kurodo[bot]" <268466204+agent-kurodo[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 00:43:32 +0000 Subject: [PATCH 12/12] fix(hashing): address four review issues from brian-arnold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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` and `extension` 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 --- src/orcapod/core/operators/merge_join.py | 9 +- src/orcapod/hashing/visitors.py | 63 +++++--- .../test_extension_type_hashing.py | 139 +++++++++++------- .../test_list_logical_type.py | 28 ++-- 4 files changed, 148 insertions(+), 91 deletions(-) diff --git a/src/orcapod/core/operators/merge_join.py b/src/orcapod/core/operators/merge_join.py index 6fe72829..34782533 100644 --- a/src/orcapod/core/operators/merge_join.py +++ b/src/orcapod/core/operators/merge_join.py @@ -241,6 +241,11 @@ def binary_static_process( ) joined = joined.drop(COMMON_JOIN_KEY) + # Use the left stream's type converter — not the default context — so that a + # MergeJoin over streams built with a non-default DataContext reconstructs the + # merged column's extension type from the correct registry. + tc = left_stream.data_context.type_converter + # Process colliding data columns: merge into sorted lists for col in colliding_keys: left_col_name = col @@ -284,10 +289,6 @@ def binary_static_process( elem_arrow_type = colliding_col_types.get(col) if elem_arrow_type is not None and isinstance(elem_arrow_type, pa.ExtensionType): - # 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 elem_python_type = tc.arrow_type_to_python_type(elem_arrow_type) list_logical_type = tc.get_logical_type_for_python_type(list[elem_python_type]) if list_logical_type is not None: diff --git a/src/orcapod/hashing/visitors.py b/src/orcapod/hashing/visitors.py index dfae2264..8abd3698 100644 --- a/src/orcapod/hashing/visitors.py +++ b/src/orcapod/hashing/visitors.py @@ -230,8 +230,8 @@ def visit_extension( # Detect list-backed extension types: extension, # extension, etc. list[File] is a types.GenericAlias # (not isinstance(..., type)), so the guard below would incorrectly skip it. - # We intercept here and delegate to _visit_list_elements with a virtual - # large_list(elem_ext_type) so each element goes through visit_extension. + # We intercept here and hash each element, folding the outer extension name + # into the result (mirrors the scalar path) to prevent list[T]/set[T] collisions. if ( typing.get_origin(python_type) in (list, set) and pa.types.is_large_list(extension_type.storage_type) @@ -241,24 +241,51 @@ def visit_extension( # not, fall through to the isinstance(python_type, type) passthrough below. if args: elem_python_type = args[0] - # Check handler for plain element types (e.g. File). - has_handler = ( - isinstance(elem_python_type, type) - and self._python_hasher.type_handler_registry.has_handler(elem_python_type) + + # Type-driven hashability: unwrap list/set nesting to the innermost + # non-container type and check whether it has a semantic handler. + # Decision is made once per column (not per row) so empty and null + # inner lists are handled correctly without crashing. + inner = elem_python_type + while typing.get_origin(inner) in (list, set): + inner_args = typing.get_args(inner) + if not inner_args: + break + inner = inner_args[0] + hashable = ( + isinstance(inner, type) + and self._python_hasher.type_handler_registry.has_handler(inner) + ) + if not hashable: + # Innermost element type has no semantic handler — whole-column + # passthrough, identical to main-branch behaviour. + return extension_type, storage_value + + # Hashable: delegate element-level hashing to _visit_list_elements. + # Using the converter's element arrow type (which may itself be an + # extension) ensures each element recurses back into + # visit_extension, producing large_binary() per element. + # We discard the returned list type (it may hold an extension type + # when data is empty) and derive the output type from the outer name. + elem_arrow_type = self._type_converter.python_type_to_arrow_type( + elem_python_type ) - # For generic alias element types (e.g. list[File] inside list[list[File]]), - # the element is itself a list/set — we should recurse so the inner - # visit_extension call can decide whether to hash or passthrough. - # Only recurse for nested list/set generic aliases, NOT for scalar - # extension types (e.g. File) that happen to map to a pa.ExtensionType — - # those must respect the has_handler check above. - is_nested_list_or_set = typing.get_origin(elem_python_type) in (list, set) - if has_handler or is_nested_list_or_set: - elem_arrow_type = self._type_converter.python_type_to_arrow_type( - elem_python_type + virtual_list_type = pa.large_list(elem_arrow_type) + _, list_data = self._visit_list_elements(virtual_list_type, storage_value) + + # Fold the outer extension name into the result, the same way the + # scalar path does. This ensures list[T] and set[T] with identical + # contents produce distinct hashes. + type_name = extension_type.extension_name.replace(".", ":") + combined = ( + type_name.encode("utf-8") + + b"::" + + b"\x00".join( + elem if isinstance(elem, bytes) else b"" + for elem in (list_data or []) ) - virtual_list_type = pa.large_list(elem_arrow_type) - return self._visit_list_elements(virtual_list_type, storage_value) + ) + return pa.large_binary(), combined # If the converter couldn't resolve to a concrete class, passthrough. if python_type is typing.Any or not isinstance(python_type, type): diff --git a/tests/test_hashing/test_extension_type_hashing.py b/tests/test_hashing/test_extension_type_hashing.py index 0d6a02bb..12b8ab53 100644 --- a/tests/test_hashing/test_extension_type_hashing.py +++ b/tests/test_hashing/test_extension_type_hashing.py @@ -266,12 +266,17 @@ def _file_storage(self, ctx, path): """Return the large_string storage value for a File.""" return ctx.type_converter.python_to_storage(File(path), File) - def test_list_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): + def test_list_file_extension_hashed_to_large_binary(self, ctx, tmp_path): """visit_extension for extension must return - (large_list(large_binary), [bytes, bytes]) — not the extension type unchanged. + (large_binary, bytes) — not the extension type unchanged. - With the buggy code the isinstance guard exits immediately, returning - (extension_type, storage_value). This assertion on new_type would fail. + The result is a single bytes value combining the outer extension name and + the per-element content hashes, mirroring the scalar encoding: + ``b"::\\x00"``. + + With the original buggy code the isinstance guard exits immediately, + returning (extension_type, storage_value). This assertion on new_type + would fail. """ f0 = tmp_path / "f0.txt"; f0.write_text("alpha") f1 = tmp_path / "f1.txt"; f1.write_text("beta") @@ -284,20 +289,22 @@ def test_list_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path) visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) new_type, new_data = visitor.visit(list_ext_type, storage_value) - assert new_type == pa.large_list(pa.large_binary()), ( - f"Expected large_list(large_binary), got {new_type}. " - "Buggy code returns the extension type unchanged." + assert new_type == pa.large_binary(), ( + f"Expected large_binary(), got {new_type}. " + "The list result is a single combined hash bytes value." ) - assert isinstance(new_data, list) - assert len(new_data) == 2 - assert all(isinstance(b, bytes) for b in new_data) + assert isinstance(new_data, bytes) + # Combined format: b"::" + assert b"::" in new_data + type_prefix, _ = new_data.split(b"::", 1) + assert type_prefix == b"list[orcapod:file]" - def test_list_file_extension_is_hash_of_file_content_hashes(self, ctx, tmp_path): - """Each element of the list result equals the scalar visit result for the same file. + def test_list_file_extension_embeds_per_element_scalar_hashes(self, ctx, tmp_path): + """The combined list hash embeds each element's scalar hash. - Contract: visit(list_ext, [s0, s1])[1][i] == visit(scalar_ext, si)[1] - This is the symmetry invariant — list[File] and scalar File hash identically - per element. Starfix then sees an ordered list of content-hash tokens. + Combined format: b"::\\x00

" + where h0 and h1 are the scalar visit results for the same files. + Splitting the suffix by \\x00 recovers individual element hashes. """ f0 = tmp_path / "contract0.txt"; f0.write_text("content zero") f1 = tmp_path / "contract1.txt"; f1.write_text("content one") @@ -309,22 +316,25 @@ def test_list_file_extension_is_hash_of_file_content_hashes(self, ctx, tmp_path) visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) - # Scalar hashes + # Scalar hashes — b"orcapod:file:::" each _, h0_bytes = visitor.visit(scalar_ext_type, s0) _, h1_bytes = visitor.visit(scalar_ext_type, s1) - # List hash - _, list_result = visitor.visit(list_ext_type, [s0, s1]) + # List hash — b"list[orcapod:file]::\x00

" + _, combined = visitor.visit(list_ext_type, [s0, s1]) - assert list_result[0] == h0_bytes, ( - "Element 0 of list result must equal the scalar hash of file 0" + # Strip outer type prefix and split to recover per-element hashes + suffix = combined.split(b"::", 1)[1] # b"\x00

" + elem_hashes = suffix.split(b"\x00") + assert elem_hashes[0] == h0_bytes, ( + "Embedded element 0 must equal the scalar hash of file 0" ) - assert list_result[1] == h1_bytes, ( - "Element 1 of list result must equal the scalar hash of file 1" + assert elem_hashes[1] == h1_bytes, ( + "Embedded element 1 must equal the scalar hash of file 1" ) def test_list_file_extension_content_change_changes_hash(self, ctx, tmp_path): - """Changing file content changes the per-element hash.""" + """Changing file content changes the combined hash.""" f = tmp_path / "mutable.txt" f.write_text("v1") @@ -333,17 +343,15 @@ def test_list_file_extension_content_change_changes_hash(self, ctx, tmp_path): s_v1 = self._file_storage(ctx, f) _, result_v1 = visitor.visit(list_ext_type, [s_v1]) - r0_v1 = result_v1[0] f.write_text("v2") s_v2 = ctx.type_converter.python_to_storage(File(f), File) _, result_v2 = visitor.visit(list_ext_type, [s_v2]) - r0_v2 = result_v2[0] - assert r0_v1 != r0_v2, "Content change must change the per-element hash" + assert result_v1 != result_v2, "Content change must change the combined hash" def test_list_file_extension_same_content_same_hash(self, ctx, tmp_path): - """Two files with identical content produce identical per-element hashes.""" + """Two files with identical content produce identical combined hashes.""" fa = tmp_path / "a.txt"; fa.write_text("identical") fb = tmp_path / "b.txt"; fb.write_text("identical") @@ -355,8 +363,8 @@ def test_list_file_extension_same_content_same_hash(self, ctx, tmp_path): _, result_a = visitor.visit(list_ext_type, [sa]) _, result_b = visitor.visit(list_ext_type, [sb]) - assert result_a[0] == result_b[0], ( - "Same content at different paths must produce the same hash" + assert result_a == result_b, ( + "Same content at different paths must produce the same combined hash" ) def test_list_file_extension_passthrough_when_no_handler(self, ctx, tmp_path): @@ -389,10 +397,10 @@ def test_list_path_extension_passthrough(self, ctx, tmp_path): assert new_type == list_ext_type, "Path has no handler — must passthrough" assert new_data == storage_value - def test_set_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): - """set[File] (extension) also hashes per element. + def test_set_file_extension_hashed_to_large_binary(self, ctx, tmp_path): + """set[File] (extension) also hashes to a combined large_binary. - get_origin(set[File]) is `set`, covered by `in (list, set)` in Fix 2. + get_origin(set[File]) is `set`, covered by `in (list, set)` in the fix. The type must be registered with the converter before visiting so that ``arrow_type_to_python_type`` can resolve it. """ @@ -408,18 +416,44 @@ def test_set_file_extension_hashed_to_list_of_large_binary(self, ctx, tmp_path): visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) new_type, new_data = visitor.visit(set_ext_type, storage_value) - assert new_type == pa.large_list(pa.large_binary()) - assert isinstance(new_data, list) - assert len(new_data) == 2 - assert all(isinstance(b, bytes) for b in new_data) + assert new_type == pa.large_binary() + assert isinstance(new_data, bytes) + # Outer type name is "set[orcapod:file]" (dots replaced with colons) + assert new_data.startswith(b"set[orcapod:file]::") + + def test_list_and_set_file_extension_produce_distinct_hashes(self, ctx, tmp_path): + """list[File] and set[File] with identical contents must hash differently. + + Before the fix, the outer extension name was not included in the result, + so ``extension`` and ``extension`` + tables holding the same file produced identical hashes — a silent hash + collision that allows memoised records keyed on one to be served for the other. + """ + f = tmp_path / "same.txt"; f.write_text("collision test") + + ctx.type_converter.register_python_class(list[File]) + ctx.type_converter.register_python_class(set[File]) + list_ext_type = ctx.type_converter.python_type_to_arrow_type(list[File]) + set_ext_type = ctx.type_converter.python_type_to_arrow_type(set[File]) + + s = self._file_storage(ctx, f) + visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) + + _, list_hash = visitor.visit(list_ext_type, [s]) + _, set_hash = visitor.visit(set_ext_type, [s]) + + assert list_hash != set_hash, ( + "list[File] and set[File] with identical content must produce distinct hashes" + ) def test_list_list_file_extension_hashed_recursively(self, ctx, tmp_path): - """extension recurses: each inner list becomes large_list(large_binary). + """extension recurses to a single combined large_binary. - Fix 2 recurses naturally: outer visit_extension delegates to _visit_list_elements - with virtual_type=large_list(extension), which calls + Recursion: outer visit_extension delegates to _visit_list_elements with + virtual_type=large_list(extension), which calls visit(extension, inner_list) for each element, which - recurses back into visit_extension. + recurses back into visit_extension, each returning (large_binary, inner_bytes). + The outer call then combines: b"::\\x00". Both the inner and outer list types must be registered with the converter so that ``arrow_type_to_python_type`` can resolve them. @@ -442,14 +476,13 @@ def test_list_list_file_extension_hashed_recursively(self, ctx, tmp_path): visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) new_type, new_data = visitor.visit(outer_ext_type, storage_value) - assert new_type == pa.large_list(pa.large_list(pa.large_binary())), ( - f"Expected large_list(large_list(large_binary)), got {new_type}" + assert new_type == pa.large_binary(), ( + f"Expected large_binary(), got {new_type}" ) - assert len(new_data) == 2 - assert len(new_data[0]) == 2 # two files in first inner list - assert len(new_data[1]) == 1 # one file in second inner list - assert all(isinstance(b, bytes) for b in new_data[0]) - assert all(isinstance(b, bytes) for b in new_data[1]) + assert isinstance(new_data, bytes) + # Outer type prefix encodes the nesting depth + assert b"list[" in new_data[:30] + assert b"::" in new_data def test_dataclass_with_list_file_field_hashed(self, ctx, tmp_path): """A struct with a list[File] extension field must hash per element. @@ -491,11 +524,11 @@ def test_dataclass_with_list_file_field_hashed(self, ctx, tmp_path): visitor = SemanticHashingVisitor(ctx.type_converter, ctx.semantic_hasher) new_type, new_data = visitor.visit(struct_type, storage_value) - # The `files` field should be hashed to large_list(large_binary) + # The `files` field should be hashed to large_binary (single combined value) files_field_type = new_type.field("files").type - assert files_field_type == pa.large_list(pa.large_binary()), ( - f"files field must be large_list(large_binary), got {files_field_type}" + assert files_field_type == pa.large_binary(), ( + f"files field must be large_binary(), got {files_field_type}" ) - files_hashes = new_data["files"] - assert len(files_hashes) == 2 - assert all(isinstance(b, bytes) for b in files_hashes) + files_combined = new_data["files"] + assert isinstance(files_combined, bytes) + assert files_combined.startswith(b"list[orcapod:file]::") diff --git a/tests/test_logical_types/test_list_logical_type.py b/tests/test_logical_types/test_list_logical_type.py index df06fb5f..a6a7da29 100644 --- a/tests/test_logical_types/test_list_logical_type.py +++ b/tests/test_logical_types/test_list_logical_type.py @@ -342,25 +342,21 @@ def test_polars_to_arrow_round_trip_preserves_extension_type(self): pl.DataFrame(table).to_arrow() calls _deserialize; without the fix it receives b'' and raises ValueError. + + Uses the type converter registry (not a fresh ListLogicalType instance) so + that the same class object is in both the orcapod and Arrow/Polars global + registries. A fresh instance registered manually would leave a different + class in the global registry, causing ArrowTypeError in cross-directory + test runs (e.g. ``uv run pytest tests/test_logical_types tests/test_core``). """ import polars as pl - from orcapod.logical_types.builtin_logical_types import LogicalPath - from orcapod.logical_types.list_logical_type_factory import ListLogicalType + from pathlib import Path + from orcapod.contexts import get_default_context - lt = ListLogicalType(LogicalPath(), is_set=False) - ext_type = lt.get_arrow_extension_type() - - # 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) - except pa.lib.ArrowKeyError: - pass # already registered - polars_ext = lt.get_polars_extension_type() - try: - pl.register_extension_type(ext_type.extension_name, type(polars_ext)) - except (ValueError, pl.exceptions.ComputeError): - pass # already registered + ctx = get_default_context() + # register_python_class registers the type with both Arrow and Polars global + # registries using the same class object the orcapod registry holds. + ext_type = ctx.type_converter.register_python_class(list[Path]) storage = pa.array([["/a.txt", "/b.txt"]], type=pa.large_list(pa.large_string())) ext_array = pa.ExtensionArray.from_storage(ext_type, storage)