Repository navigation
fix(xorq): an ordered copy keeps its source's types and reader options, and its digest file is written atomically (#197, #198, #211) - #215
Closed
paddymul wants to merge 4 commits into
Conversation
- #197: the ordered copy of a parquet source with date64, map, time32 and time64 columns has the source's schema plus __row_order, and a recipe sees the ibis types a direct read of the source gives. A decimal256 source builds. A polars panic while reading a CSV is a BuildError. - #198: a function passed as with_column_names reaches polars as a function, and a deleted copy of that read cannot be made again from the manifest and says so. - #211: an empty .digest sidecar is recomputed from the copy, not recorded as the copy's content digest. The polars PanicException is a BaseException, so the tests that can hit it catch it and fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the source's types (#197) polars wrote the copy and did not keep every Arrow type: a date64 came back as a timestamp, a map as a list of structs, time32 and time64 as time64[ns], and a decimal256 made it panic with a PanicException, which no `except Exception` catches. _write_parquet_copy now streams ParquetFile.iter_batches() in file order, regroups the rows into 122,880-row groups, appends __row_order from np.arange and writes with the snapshot's parquet settings. The copy's schema is the source's plus __row_order. SNAPSHOT_FORMAT_VERSION stays at 1. The row groups are where they were, and on ordinary columns the new copy has the same content digest and gives bit-identical float SUM and AVG on the single-partition connection; ADR-009's implementation notes have the measurements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lars panic is a BuildError (#198, #197) tallyman_read_csv built the copy from the JSON form of its schema and scan_csv options on the first ingest, the form the manifest records. JSON turns a function into its repr, so with_column_names=lambda ... reached polars as a string and polars panicked calling it. ensure_ordered_copy now takes the caller's (schema, scan_kwargs) as csv_args and the first write uses them; the JSON form still names the copy and is what recreate_ordered_copy replays, which already refuses a record marked lossless: False. polars still parses a CSV, and a function it calls can still raise, which makes it panic. _write_copy turns that PanicException, a BaseException that the build's `except Exception` and the MCP tool's `except BuildError` both miss, into a BuildError that names the source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… empty one is read back from the copy (#211) The sidecar that caches a copy's content digest was written with Path.write_text, which truncates before it writes, and was read back as whatever it held. A build that read it empty recorded content_digest "" in its manifest, and a later recreate_ordered_copy then reported the copy as unfaithful. _write_atomically now computes the digest from the temp file, writes the sidecar to a unique temp name and moves it into place with os.replace, then moves the copy into place, so a reader that finds the copy finds its whole digest beside it and never one left by an earlier copy. _content_digest_of treats an empty sidecar as missing: it reads the digest back from the copy and writes the sidecar the same atomic way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
paddymul
marked this pull request as ready for review
September 22, 2026 16:59
This was referenced Sep 22, 2026
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #189: the base is
feat/adr-007-009-cache-redesign, notmain. Fixes #197, Fixes #198, Fixes #211.An ordered copy is the parquet copy of a source that a recipe reads instead of the source: the source's rows in file order plus a last column,
__row_order,0..N-1(ADR-008 D2, every file tallyman reads carries__row_order). It lives atcompute_cache/ordered_sources/<key>.parquet, and a<key>.digestfile beside it caches its content digest. The manifest records the reader options it was made with, soensure_materializedcan make a deleted copy again (ADR-007 D13, which files are cache).What changed
1ac8fc7). pyarrow writes the copy of a parquet source (ordered_copy._write_parquet_copy). It streamsParquetFile.iter_batches()in file order, regroups the rows into 122,880-row groups whatever the source's row groups are, appends__row_orderfromnp.arange, and writes with the snapshot's parquet settings (materialize._PARQUET_OPTIONS: zstd level 3, format 2.6, statistics, a page index). The copy's Arrow schema is the source's plus__row_order, sodate64, map,time32,time64anddecimal256columns keep their types, and a recipe sees the ibis types a direct read of the source gives. A worthy aggregate over adecimal256(40, 2)column builds and sums it exactly.07eb3d0).tallyman_read_csvpasses the caller's ownschemaandscan_csvoptions to the first write. Their JSON form in the manifest still names the copy and is whatrecreate_ordered_copyreplays. That function already refuses a record markedlossless: False, so a deleted copy of a read with a function option says it cannot be made again. polars still parses a CSV, and aPanicExceptionfrom it now becomes aBuildErrorthat names the source. The panic is aBaseException, so before this it got past both the build'sexcept Exceptionand the MCP tool'sexcept BuildError. Awith_column_namesfunction that raises is one way to cause one.c267f7d). The sidecar is written to a unique temp name and moved into place withos.replace, before the copy itself is, so a reader that finds the copy finds its whole digest beside it. An empty sidecar is treated as missing: the digest is read back from the copy and the sidecar written again. The race as the issue describes it needs an unlocked writer, because a build and both copy writers hold the project lock. Two things could still leave the sidecar empty: an unlockedread_project_file(a notebook process) that rewrites a missing sidecar, or an in-place write cut off after its truncate. The fix covers both.Docs: ADR-008's and ADR-009's "Implementation notes", and the lines in
docs/caching.mdanddocs/system-contract.mdthat said polars writes every copy.The snapshot format version stays at 1
SNAPSHOT_FORMAT_VERSION(ADR-009 D3, the snapshot format) stands for the settings that decide the batch boundaries an entry sees, and the ordered copy's 122,880-row groups are one of them. The pyarrow copy's bytes differ from the polars copy's, but its row groups are where they were, and polars also wrote a page index. I measured the 1.5M-row test source (tests/big_parquet.py) on the single-partition materialization connection. The two copies have the same content digest. TheSUMandAVGof the float column come out bit for bit the same ungrouped, filtered on the float, and over four ranges of the sortedidcolumn, which the page index prunes. The two copies oforders.parquetalso have matching digests. A version bump would make any later unfaithful heal of an existing entry blame a format change that did not change what the entry read.What does change is the columns polars did not keep. An entry built before this over a
date64, map,time32ortime64column read a copy with other types. If its copy is made again,recreate_ordered_copyfinds that the content digest differs from the recorded one and recordsunfaithful_ordered_copy. Rebuilding such an entry is the fix.How it was built
2665f80: six failing tests. CI run: ruff passed and the fast suite failed on exactly those six (6 failed, 864 passed), each for the reason its issue gives.1ac8fc7,07eb3d0,c267f7d: one fix per issue, each making its own tests pass. CI run: ruff, the fast suite (870 passed) and the integration suite (7 passed) all pass.Locally, on
c267f7d: the fast suite gives 863 passed, 6 skipped and 1 failed. The failure istest_fouc.py::test_unknown_api_path_404s, which gets a 503 because this worktree has nopackages/app/dist, and it fails the same way without these commits. I also checked the new writer on an empty source, a source whose only column is__row_order, a pandas categorical spanning row groups with different dictionaries, and list and struct columns.Not in this PR
fixed_size_binary: xorq 0.3.26 cannot read it (KeyError: FixedSizeBinaryTypewhile it builds the table's schema), from the source or from the copy. polars turned it into binary, so a recipe over one used to build. Now it fails the way a direct read of the source does.use_dictionarywould change both files, so I left it.sys.modules._row_groupsrepeats the regrouping inmaterialize._stream_to_parquet. I left materialize.py alone because fix(xorq): a failed build keeps the snapshot already on disk for its hash #213 edits it.🤖 Generated with Claude Code