DDIR: adopt corgi's copy-audit API (frankmcsherry/WIP#18) - #857
Merged
Conversation
Bumps the corgi pin from #16 to #18 (`de0f2ac`), which also picks up #17. Four API changes, all of which delete code at the call site: * `corgi::hash` returns the ids (`Vec<u64>`) rather than a `Value` that every caller here unwrapped in the same expression. Four sites lose `.into_u64(..).unwrap()`. * `arrange::hash_rows` is gone — it was a second, disagreeing implementation of the same fold (it mixed leaf width into its seed, so it called a `u8 5` and a `u64 5` different values, which the canonical hash does not). The two round-trip test calls become `corgi::hash`. * `Bounds::Offsets` holds an `Arc`, so a `Value` clone no longer copies a list's partition. Construction goes through `Bounds::offsets`, and the three places that hand-rolled a `Vec<usize>` of end offsets by matching both variants now call `Bounds::to_vec`, which is public and does it. * `Value::Sum` carries one `Tags` — the lane assignment — instead of a tag column and an offset column, with a two-word `Const` form for the one-tag case. `signed_order_view` moves the assignment through untouched; `untranscode` reads `tag_at`/`offset_at` per row. And one adoption beyond compiling: `sort_consolidate` was building the two `i`/`i+1` index columns to ask for an adjacent comparison. `compare_adjacent` names the pattern instead, so the index columns are not built and corgi reads both sides densely. That is on the chunk-merge path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPtSwqXTuH4wefbQd2F8pA
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.
Bumps the corgi pin from #16 (
578ef3f) to frankmcsherry/WIP#18 (de0f2ac), which also picks up #17, and adopts the API changes that came with it.Net −12 lines: three of the four changes delete code at the call site, because each one removes a thing DDIR was doing to work around a corgi API that made the caller pay for something.
What changed upstream, and what it does here
corgi::hashreturns the ids. It returned aValuethat every caller outside corgi's op vocabulary unwrapped in the same expression — ours was literallycorgi::hash(col).into_u64("ids").unwrap(). Four sites lose the unwrap.arrange::hash_rowsis gone. It was a second implementation of the structural content hash, with different constants, and the two disagreed about whether au8 5and au64 5are the same value — which is exactly the question the boundary id exists to answer. Both were public and both documented as the boundary id. This repo had already worked that out:reduce.rsroutes real ids throughcorgi::hashand calls the other one "branch-local". The only surviving uses were two lines of a round-trip test comparing the function to itself, so nothing observable moves.Bounds::Offsetsholds anArc. AValueclone no longer copies a list's partition — which for aList<U64>was as many bytes as the payload column, at every shared graph edge. Construction goes throughBounds::offsets, and the three places here that hand-rolled aVec<usize>of end offsets by matching both variants now callBounds::to_vec, which is public and does exactly that.Value::Sumcarries oneTags. The lane assignment — tags plus within-lane offsets — is one type with a two-wordConstform for the case where every row carries one tag.signed_order_viewmoves the assignment through untouched instead of rebuilding it;untranscodereadstag_at/offset_atper row.One adoption beyond compiling
sort_consolidatewas building twoi/i+1index columns to ask corgi for an adjacent comparison — 16 bytes a row of index to produce a one-byte-a-row answer.compare_adjacentnames the pattern instead, so the index columns are not built and corgi reads both sides densely. That is on the chunk-merge path.Verification
interactivetests pass.master-nextbuild, so none are introduced.Cargo.lockis gitignored, so onlyCargo.tomlmoves.Note for review
Porting this is what caught a real gap in the upstream PR:
Tagswas exported but every reader waspub(crate), so the type could be named and not used — whichuntranscodeneeds. That went back as an extra commit on WIP#18 before it merged, so the pin here is the fixed version. Worth doing the downstream port before merging the upstream change rather than after.🤖 Generated with Claude Code
https://claude.ai/code/session_01PPtSwqXTuH4wefbQd2F8pA