Skip to content

Commit 9182e0f

Browse files
paddymulclaude
andcommitted
fix(csv-import): kwarg collision, reserved-col totality, recon-after-delete
Green for the three #148-review findings: - #1 tallyman_read_csv now rejects infer_schema_length / schema_overrides up front (_RESERVED_SCAN_KWARGS) with a ValueError steering to schema=, instead of forwarding them into the internal scan_csv calls that already set them (raw TypeError) or silently bypassing the schema system. - #2 project_path grows must_exist=False; read_project_file resolves the reconstruction path with it and runs the cas block BEFORE the existence guard, so a moved/deleted source no longer defeats the frozen-.cas-clone read. - #3 _materialize_ordered excludes original_row_order from the header the schema spec must cover, so a complete data-column schema over a CSV that carries the reserved column is no longer rejected as 'not total'. Also adds a regression guard that an ordinary reader option (separator) still forwards to scan_csv. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent c0f5689 commit 9182e0f

2 files changed

Lines changed: 56 additions & 7 deletions

File tree

src/tallyman_xorq/io.py

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,13 @@ class ProjectDataNotFound(FileNotFoundError):
1919
pass
2020

2121

22-
def project_path(rel_path: str, project: str | None = None) -> Path:
23-
"""Resolve `rel_path` against the project's data dir. Raises if absent."""
22+
def project_path(rel_path: str, project: str | None = None, must_exist: bool = True) -> Path:
23+
"""Resolve `rel_path` against the project's data dir. Raises if absent.
24+
25+
Pass ``must_exist=False`` for digest-pinned reconstruction: it serves the frozen
26+
``.cas`` clone (``recon_cas_path``) and must not require the live source to still
27+
be present. The traversal guard still applies regardless.
28+
"""
2429
proj = resolve_project(project)
2530
base = data_dir(proj)
2631
candidate = (base / rel_path).resolve()
@@ -29,7 +34,7 @@ def project_path(rel_path: str, project: str | None = None) -> Path:
2934
candidate.relative_to(base.resolve())
3035
except ValueError as exc:
3136
raise ProjectDataNotFound(f"{rel_path!r} resolves outside {base} (got {candidate})") from exc
32-
if not candidate.is_file():
37+
if must_exist and not candidate.is_file():
3338
raise ProjectDataNotFound(f"{candidate} not found")
3439
return candidate
3540

@@ -53,7 +58,6 @@ def read_project_file(rel_path: str, project: str | None = None):
5358
from tallyman_xorq import source_identity as si
5459

5560
proj = resolve_project(project)
56-
path = project_path(rel_path, proj)
5761
if si.mode() == "cas":
5862
recorded = _reconstructing_source_digest(proj, rel_path)
5963
if recorded is not None:
@@ -65,8 +69,14 @@ def read_project_file(rel_path: str, project: str | None = None):
6569
# source collector is independent of tracked_expr_from_alias's _RECONSTRUCTING-gated
6670
# parent capture, so both fire) so a child build reconstructing this entry
6771
# records the frozen digest and gc_cas keeps the clone alive.
72+
#
73+
# must_exist=False: recon_cas_path serves the frozen clone without the live
74+
# source, so a moved/deleted source must not defeat the pinned read. This
75+
# block MUST run before the existence-requiring project_path below.
76+
recon_path = project_path(rel_path, proj, must_exist=False)
6877
si.note_source(rel_path, recorded)
69-
return deferred_read_parquet(str(si.recon_cas_path(proj, path, recorded)))
78+
return deferred_read_parquet(str(si.recon_cas_path(proj, recon_path, recorded)))
79+
path = project_path(rel_path, proj)
7080
if si.mode() != "off":
7181
digest = si.digest_for(proj, path)
7282
si.note_source(rel_path, digest)
@@ -156,6 +166,13 @@ def _polars_overrides(schema):
156166
_REST = "&rest" # wildcard closure: the remaining tail columns (ADR D3)
157167
_BEGIN = "&begin" # wildcard closure: the leading head columns (specced, not yet implemented)
158168

169+
# scan_csv options tallyman_read_csv owns and must not accept as pass-through
170+
# **kwargs: every internal scan_csv call hardcodes infer_schema_length (the
171+
# escalation ladder) and schema_overrides (derived from schema=), so forwarding
172+
# either collides ("multiple values for keyword argument") or silently bypasses
173+
# the schema system. Rejected up front with a message steering to schema=.
174+
_RESERVED_SCAN_KWARGS = ("infer_schema_length", "schema_overrides")
175+
159176

160177
def _spec_sig(spec) -> str:
161178
"""A stable string for the cache key, covering every schema-spec shape.
@@ -387,7 +404,12 @@ def _materialize_ordered(src: Path, schema, scan_kwargs: dict, tmp_path: Path) -
387404
plan = None
388405
explicit_only = False
389406
if schema is not None:
390-
plan = _normalize_schema(schema, header)
407+
# original_row_order is tallyman's reserved index — _ordered re-appends it, and
408+
# _validate_existing_row_order already vetted a pre-existing one. It is not the
409+
# caller's to spec, so exclude it from the header the spec must totally cover;
410+
# otherwise a complete data-column schema is spuriously rejected as "not total".
411+
spec_header = [h for h in header if h != "original_row_order"]
412+
plan = _normalize_schema(schema, spec_header)
391413
explicit_only = plan["all_pinned"]
392414

393415
def _ordered(infer_len):
@@ -534,10 +556,21 @@ def tallyman_read_csv(path: str, schema=None, **kwargs):
534556
**kwargs: Forwarded to ``polars.scan_csv`` — reader options such as
535557
``separator``, ``skip_rows``, ``null_values``, ``quote_char``,
536558
``has_header``, ``encoding``. They participate in the intermediate's
537-
cache key, so changing one re-ingests.
559+
cache key, so changing one re-ingests. ``infer_schema_length`` and
560+
``schema_overrides`` are managed internally (see ``_RESERVED_SCAN_KWARGS``)
561+
and rejected — they would collide with the values every internal
562+
``scan_csv`` call already sets.
538563
"""
539564
import xorq.api as xo
540565

566+
reserved = [k for k in _RESERVED_SCAN_KWARGS if k in kwargs]
567+
if reserved:
568+
raise ValueError(
569+
f"tallyman_read_csv: {reserved} is managed internally, not a pass-through "
570+
"polars.scan_csv reader option. Type inference escalates automatically "
571+
"(100 -> 10k -> whole-file); to pin column types pass schema= (an ibis schema, "
572+
"plain dict, or tuple-of-tuples), never schema_overrides."
573+
)
541574
parquet_path = _ordered_csv_parquet(path, schema, kwargs)
542575
return xo.deferred_read_parquet(str(parquet_path)).order_by("original_row_order")
543576

tests/test_tallyman_read_csv_contract.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,3 +233,19 @@ def test_reserved_scan_kwarg_raises_clear_error(project, monkeypatch, bad_kwarg)
233233
kwargs = {bad_kwarg: 1000 if bad_kwarg == "infer_schema_length" else {"a": "int64"}}
234234
with pytest.raises(ValueError, match="managed internally"):
235235
tallyman_read_csv(str(p), **kwargs)
236+
237+
238+
def test_ordinary_reader_kwarg_still_forwarded(project, monkeypatch):
239+
"""The guard must reject only the two reserved keys — a genuine reader option
240+
such as ``separator`` still reaches polars.scan_csv and parses correctly."""
241+
monkeypatch.setenv("TALLYMAN_PROJECT", project)
242+
p = data_dir(project) / "semi.csv"
243+
p.write_text("a;b\n1;x\n2;y\n")
244+
code = f"""
245+
from tallyman_xorq.io import tallyman_read_csv
246+
expr = tallyman_read_csv({str(p)!r}, separator=";")
247+
"""
248+
res = catalog_create("semicsv", code)
249+
assert "error" not in res, res
250+
types = _types_of(res)
251+
assert "a" in types and "b" in types # split into two columns, not one "a;b"

0 commit comments

Comments
 (0)