Skip to content

Commit 0fd3f51

Browse files
paddymulclaude
andcommitted
fix(csv-import): thread the reserved column through every header reader
9182e0f #3 excluded original_row_order from the totality check by pre-stripping the header at the call site (`spec_header`). Three other surfaces still read the raw or stripped header inconsistently, breaking exactly in the path #3 enables. Thread the reserved column through _normalize_schema (and _suggest_schema_dsl) as a `reserved` param instead, so the coverage check, the diagnostics, and positional binding all read one consistent header: - Suggested-schema hint (#1): _suggest_schema_dsl drops reserved columns, so the #143 recovery suggestion is paste-ready. It no longer emits a cell for original_row_order, which the totality check excludes — pasting the old suggestion back raised 'positional schema has 3 columns but the header has 2'. - Diagnostics (#2): error messages report the speccable header plus a note about the reserved column, instead of the reserved-stripped subset. A 3-column file is no longer described as 'has 2 [a, b]', and a by-name schema that names the reserved column gets a targeted message, not a misleading 'not in the header'. - Positional binding (#3): a canonical original_row_order that is not the trailing column now raises (steering to a by-name schema) rather than silently shifting later positional cells onto the wrong data column. Also rejects a spec that renames a DATA column ONTO original_row_order up front, instead of leaking a raw polars DuplicateError at sink. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent f81b1d5 commit 0fd3f51

1 file changed

Lines changed: 77 additions & 24 deletions

File tree

src/tallyman_xorq/io.py

Lines changed: 77 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -207,19 +207,39 @@ def _spec_pairs(spec):
207207
return list(spec.items())
208208

209209

210-
def _normalize_schema(spec, header: list[str]) -> dict:
210+
def _normalize_schema(spec, header: list[str], *, reserved: tuple[str, ...] = ()) -> dict:
211211
"""Resolve a schema spec against the CSV *header* into a polars parse plan.
212212
213+
*header* is the CSV's full header. *reserved* names tallyman-managed columns
214+
(currently ``original_row_order``) that the caller must not spec: they are
215+
excluded from the columns the spec has to cover, but kept in the diagnostics
216+
so an error message matches the file the user is looking at rather than a
217+
silently reserved-stripped subset. Threading ``reserved`` in — rather than
218+
pre-stripping the header at the call site — keeps the totality check, the
219+
diagnostics, and positional binding all reading one consistent header.
220+
213221
Returns ``{overrides, rename, out_names, all_pinned}``:
214222
- ``overrides`` — ``{header_name: polars_dtype}`` for pinned columns, keyed by the
215223
name polars reads (positional renames are applied *after* the read).
216224
- ``rename`` — ``{header_name: output_name}`` where they differ (positional only).
217225
- ``out_names`` — output column names in column order (the final ``select``).
218226
- ``all_pinned`` — True if no column is inferred (then ``infer_schema_length=0``).
219227
220-
Raises the actionable ValueError on a by-name miss, a non-total spec, or an
221-
over-long positional spec — these are the #143 error-contract surfaces.
228+
Raises the actionable ValueError on a by-name miss, a non-total spec, an
229+
over-long positional spec, a by-name spec that names a reserved column, or a
230+
positional spec whose reserved column is not the trailing column — these are
231+
the #143 error-contract surfaces.
222232
"""
233+
reserved = tuple(r for r in reserved if r in header)
234+
speccable = [h for h in header if h not in reserved]
235+
reserved_hint = ""
236+
if reserved:
237+
names = ", ".join(repr(r) for r in reserved)
238+
reserved_hint = (
239+
f" (the file also has tallyman's reserved column {names}, which tallyman "
240+
"manages automatically — leave it out of the schema.)"
241+
)
242+
223243
overrides: dict = {}
224244
rename: dict = {}
225245
inferred = False
@@ -237,20 +257,31 @@ def _normalize_schema(spec, header: list[str]) -> dict:
237257
cells = cells[:-1]
238258
if any(n in (_REST, _BEGIN) for n, _ in cells):
239259
raise ValueError("tallyman_read_csv: '&rest' must be the last cell and '&begin' the first.")
240-
if len(cells) > len(header):
260+
# Positional cells bind by physical column position. Excluding a reserved column
261+
# that is not the trailing column would silently shift every later cell onto the
262+
# wrong data column, so require the reserved column(s) to be a trailing suffix
263+
# (header[:len(speccable)] == speccable); otherwise steer to a by-name schema.
264+
if reserved and header[: len(speccable)] != speccable:
265+
raise ValueError(
266+
f"tallyman_read_csv: a positional schema cannot bind {list(header)} — its reserved "
267+
f"column {list(reserved)} is not the last column, and positional cells bind by physical "
268+
"column position, so excluding it would silently rebind later cells onto the wrong data "
269+
"column. Use a by-name schema (a dict or ibis schema keyed on the header names) instead."
270+
)
271+
if len(cells) > len(speccable):
241272
raise ValueError(
242273
f"tallyman_read_csv: positional schema has {len(cells)} columns but the CSV header has "
243-
f"{len(header)} {list(header)}."
274+
f"{len(speccable)} {list(speccable)}.{reserved_hint}"
244275
)
245-
if rest_dtype is None and len(cells) != len(header):
246-
uncovered = list(header[len(cells):])
276+
if rest_dtype is None and len(cells) != len(speccable):
277+
uncovered = list(speccable[len(cells):])
247278
raise ValueError(
248279
f"tallyman_read_csv: schema is not total — it leaves column(s) {uncovered} unspecified. "
249280
'Name every column, or close the spec with ("&rest", "infer").'
250281
)
251282
out_names = []
252283
for i, (out_name, dt) in enumerate(cells):
253-
orig = header[i]
284+
orig = speccable[i]
254285
out_names.append(out_name)
255286
if out_name != orig:
256287
rename[orig] = out_name
@@ -259,7 +290,7 @@ def _normalize_schema(spec, header: list[str]) -> dict:
259290
inferred = True
260291
else:
261292
overrides[orig] = pl_dt
262-
for orig in header[len(cells):]: # the &rest tail keeps its header name
293+
for orig in speccable[len(cells):]: # the &rest tail keeps its header name
263294
out_names.append(orig)
264295
pl_dt = _resolve_spec_dtype(rest_dtype)
265296
if pl_dt is None:
@@ -278,14 +309,20 @@ def _normalize_schema(spec, header: list[str]) -> dict:
278309
rest_dtype = dt
279310
continue
280311
named[str(name)] = dt
281-
missing = [n for n in named if n not in header]
312+
named_reserved = [n for n in named if n in reserved]
313+
if named_reserved:
314+
raise ValueError(
315+
f"tallyman_read_csv: schema names tallyman's reserved column {sorted(named_reserved)}, which "
316+
"tallyman manages automatically — remove it from the schema (it is added and ordered for you)."
317+
)
318+
missing = [n for n in named if n not in speccable]
282319
if missing:
283320
raise ValueError(
284-
f"tallyman_read_csv: schema column(s) {sorted(missing)} are not in the CSV header {list(header)}. "
321+
f"tallyman_read_csv: schema column(s) {sorted(missing)} are not in the CSV header {list(speccable)}. "
285322
"Bind by name only when the names match the header; to rename by position pass a tuple-of-tuples "
286-
'schema, e.g. schema=(("Date", "date"), ("&rest", "infer")).'
323+
'schema, e.g. schema=(("Date", "date"), ("&rest", "infer")).' + reserved_hint
287324
)
288-
uncovered = [h for h in header if h not in named]
325+
uncovered = [h for h in speccable if h not in named]
289326
if rest_dtype is None and uncovered:
290327
raise ValueError(
291328
f"tallyman_read_csv: schema is not total — it leaves column(s) {uncovered} unspecified. "
@@ -303,7 +340,7 @@ def _normalize_schema(spec, header: list[str]) -> dict:
303340
inferred = True
304341
else:
305342
overrides[name] = pl_dt
306-
return {"overrides": overrides, "rename": {}, "out_names": list(header), "all_pinned": not inferred}
343+
return {"overrides": overrides, "rename": {}, "out_names": list(speccable), "all_pinned": not inferred}
307344

308345

309346
_ESCALATED_INFER = 10_000 # the middle rung of the #143 infer ladder; whole-file (None) is the terminal
@@ -335,12 +372,18 @@ def _polars_to_ibis(dt) -> str:
335372
return "string" # safe fallback — an unmapped column still reads as string
336373

337374

338-
def _suggest_schema_dsl(src: Path, scan_kwargs: dict) -> str:
339-
"""Whole-file-infer *src* and render it as the paste-ready tuple-of-tuples DSL."""
375+
def _suggest_schema_dsl(src: Path, scan_kwargs: dict, reserved: tuple[str, ...] = ()) -> str:
376+
"""Whole-file-infer *src* and render it as the paste-ready tuple-of-tuples DSL.
377+
378+
*reserved* columns (tallyman-managed, e.g. ``original_row_order``) are dropped:
379+
they must not appear in the suggestion because the caller cannot spec them —
380+
a suggestion carrying one is un-pasteable (the totality check excludes the
381+
reserved column, so pasting it back over-counts the columns and raises).
382+
"""
340383
import polars as pl
341384

342385
inferred = pl.scan_csv(str(src), infer_schema_length=None, **scan_kwargs).collect_schema()
343-
pairs = tuple((name, _polars_to_ibis(dt)) for name, dt in inferred.items())
386+
pairs = tuple((name, _polars_to_ibis(dt)) for name, dt in inferred.items() if name not in reserved)
344387
return repr(pairs)
345388

346389

@@ -398,18 +441,28 @@ def _materialize_ordered(src: Path, schema, scan_kwargs: dict, tmp_path: Path) -
398441
# check; infer_schema_length=0 reads just the header line, no type sampling.
399442
header = list(pl.scan_csv(str(src), infer_schema_length=0, **scan_kwargs).collect_schema().names())
400443
has_row_order = "original_row_order" in header
444+
# original_row_order is tallyman's reserved index — _ordered re-appends it, and
445+
# _validate_existing_row_order (below) vets a pre-existing one. It is not the caller's
446+
# to spec, so it is threaded as `reserved` through every header reader (the schema
447+
# plan, the diagnostics, and the suggested-schema hint) rather than stripped ad hoc.
448+
reserved = ("original_row_order",) if has_row_order else ()
401449
if has_row_order:
402450
_validate_existing_row_order(src, scan_kwargs) # reuse it, or raise — never with_row_index over it
403451

404452
plan = None
405453
explicit_only = False
406454
if schema is not None:
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)
455+
plan = _normalize_schema(schema, header, reserved=reserved)
456+
# A spec may not PRODUCE the reserved name either: renaming a data column onto
457+
# 'original_row_order' collides with the index _ordered re-appends (a raw polars
458+
# DuplicateError at sink). Reject the reserved output name up front with the same
459+
# guidance as the pre-existing-column clash.
460+
if "original_row_order" in plan["out_names"]:
461+
raise ValueError(
462+
"tallyman_read_csv: 'original_row_order' is reserved for tallyman's canonical "
463+
"row index and cannot be a schema output column name. Rename that column to "
464+
"something other than 'original_row_order' in the schema."
465+
)
413466
explicit_only = plan["all_pinned"]
414467

415468
def _ordered(infer_len):
@@ -440,7 +493,7 @@ def _ordered(infer_len):
440493
last_exc = exc
441494
raise ValueError(
442495
f"tallyman_read_csv: the schema does not parse {src.name}: {last_exc}. "
443-
f"Suggested schema (whole-file inference): schema={_suggest_schema_dsl(src, scan_kwargs)}"
496+
f"Suggested schema (whole-file inference): schema={_suggest_schema_dsl(src, scan_kwargs, reserved)}"
444497
)
445498

446499

0 commit comments

Comments
 (0)