Repository navigation
Materialize a dataset's query surface to Parquet [REPORT-113] - #16
Conversation
Requirements and implementation plan for cc-data dataset materialize, which writes each of a dataset's file-backed views to a ZSTD Parquet at a stable path inside the dataset folder. Queries read a view's Parquet while it is fresh and fall back to the raw artifacts otherwise, so materializing never changes an answer, and the file is readable by any DuckDB, pandas or Polars without going through cc-data at all. What the plan builds, in seven steps: - A named derived-subdirectory predicate, and the purge gap it closes. Purge leaves such a folder and everything in it intact, verified by planting files and running it. Reindex and the orphan check already ignore one, but only by accident, so the step is mostly tests turning that accident into a contract. REPORT-89 registers exports/ against the same predicate. - The manifest's Materialized entry, recording each view's Parquet and the inputs it was built from, each with a size-and-mtime fingerprint. Freshness cannot key on a version, because report CSVs have none: report_<run>.csv is a fixed name overwritten on re-fetch. A declared input that will not stat records a reserved sentinel, and Fresh compares the whole input set, so a run downloaded since the copy makes a union view stale even though every recorded input still matches. - One more level in the engine's registration loop: the materialized statement, then primary, then fallback. A Parquet that is missing, corrupt or truncated fails at CREATE VIEW rather than at query time, which is what lets a single warning cover all three and send the view back to its raw artifacts rather than to empty. - The writer. It opens the dataset with materialization suppressed, so a stale Parquet can never be copied forward into a fresh-looking one, and it copies outside the mutation locks so a concurrent get cannot fail as busy. Copies land under a temp name; the manifest is then re-read and re-checked, the survivors renamed and the rest deleted, and the manifest write is the commit point as it is in merge and purge. - materialized_bytes on dataset show and dataset list, reported beside size_bytes rather than redefining a documented contract, plus two warnings. The unreferenced-Parquet one needs only the folder listing, so it lives in driftWarnings and reaches both surfaces for free; the staleness one needs a view's current input list and so comes from duck, through one decorator both callers of the show contract use. - The CLI command and the MCP tool together, because the drift guard checks the tool inventory in both directions. - The guidance, the researcher guide's layout contract, and an opt-in timing test gated behind an environment variable, since its fixture is roughly 455MB of generated JSONL and CI runs plain go test ./... with no -short convention. Decisions a reviewer needs. It materializes views, not stores: the dataset REPORT-120 consumes holds no stores at all, so store-scoped materialization would produce nothing and 120 could not land, and a view's Parquet carries REPORT-112's derived columns rather than the raw CSV's. The ticket's persistent DuckDB is gone, having no counterpart in the pipeline it was modeled on, and because a view stored in a .duckdb file freezes the absolute Parquet path so dataset rename breaks it. Paths are fixed rather than version-stamped and the folder is materialized/ rather than hidden, because external scripts hardcode them. There is no view-selection flag: the folder is a deterministic mirror of the materializable set, which is what lets an outside reader treat a path as a contract instead of a function of which flags someone last ran. Reindex drops the manifest entries but never deletes the files, since cc-data losing confidence in a Parquet is not grounds for removing something an external study script may be reading. Two refusals, both per view, so clean views are still materialized and the run exits non-zero. A declared input missing from disk is the case the sibling repo's guard exists for, where a glob matching fewer files once replaced a complete history corpus with a fraction of it; --allow-partial overrides it and reports N of M. A view that registered from its typed-empty fallback is refused with no override, because COPY of a degraded view succeeds and writes a 404-byte zero-row Parquet. Store-backed views additionally assert the copy's row count against the manifest, the check both hand-written build scripts end with. The incomplete-download guard the ticket asked for is demoted to a warning. Exactly one place writes Complete: false, the store fetch, and a paged fetch merges once at the end, so an incomplete store download leaves the Parquet byte-identical to one built if the run had never been requested. Under the obvious file-based mapping the guard could never have fired at all, since store downloads carry no Files.
IsDerivedSubdir names the subfolders holding data cc-data generated from a dataset's own artifacts, and deleteArtifacts removes them. Purge previously left such a folder and everything in it behind, because its directory arm matched segments and attachments by literal name. The invisibility half already held, but only because Reindex and the orphan scan skip every directory. The tests pin it: three files that would each be adopted or flagged at the top level stay invisible inside the folder, with a top-level copy of one of them as the control proving the orphan check fires.
Manifest.Materialized maps a view name to its Parquet and the inputs it was built from, each carrying a size-and-mtime fingerprint. Freshness cannot key on a version, because report CSVs have none: report_<run>.csv is a fixed name overwritten on re-fetch. No manifest version bump is needed, since decoding already tolerates an absent field and the map is omitted when empty. Fresh compares the whole input set rather than only the recorded entries, so a run downloaded since the copy makes a union view stale even though every recorded input still matches. An input that will not stat records a sentinel that no real fingerprint can collide with, and because it compares like any other fingerprint, a file that was missing then and is missing now reads as fresh while one that has since appeared reads as stale. Purge clears the map alongside the four it already resets. Leaving entries that name files the same call is about to delete is what its own commit-ordering comment forbids. The requirements said Fresh returns false whenever any current input fails to stat, which would have made the sentinel carry no information and contradicted the plan's own round-trip test. Corrected there.
The registration loop gains one level: the materialized statement, then primary, then fallback. Every view keeps its exact behavior when nothing is materialized, and applyMaterialized is a post-pass over the statements rather than a change to how any of them is built. Membership of the materializable set is code-derived from two halves, declaring files and being a static view, so the writer and the reader cannot disagree and a view added later joins or stays out by its own construction. Both halves key on bareViewName, because a statement built for a named dataset carries a schema prefix that the manifest's map and StaticViewNames do not. A Parquet that is absent, corrupt or truncated all fail at CREATE VIEW rather than at query time, so one warning covers every shape and the view falls back to its raw artifacts rather than to typed-empty. Going stale is silent by contract. The tests drive a Parquet carrying a sentinel row the store does not have, so "the Parquet is the source" is proved rather than assumed, and they pin the two properties that would otherwise be discovered later: the sentinel survives a dataset rename, and in a two-dataset session it appears under the materialized dataset's schema and not the other's.
Materialize copies each materializable view to a ZSTD Parquet, reading through OpenRaw so a view is always copied from its raw artifacts: copying a view that was itself reading a stale Parquet would write that data back out with fresh fingerprints attached, which is the one way this feature could silently corrupt an answer. The copies run outside the mutation locks, which are held only to read the manifest and to repoint it, so a concurrent get does not fail as busy for the whole run. Each view's inputs are fingerprinted just before its copy starts and compared again at the repoint, and a view whose inputs moved in between is discarded, since its Parquet describes a state that is already gone. The temp files are renamed only after that re-check, keeping the manifest write the commit point as it is in merge and purge. Two refusals, per view rather than per command. A view that registered from its typed-empty fallback has no override, because COPY of a degraded view succeeds and writes a zero-row Parquet. A declared input missing from disk is refused unless --allow-partial is given, and the reason names both remedies plus the override, so the discoverable move is not to type the flag forever. A run under --allow-partial reports the shortfall as "N of M", which is what makes it land. A store-backed copy is additionally checked against the manifest's record. An incomplete download is a warning naming the runs, not a refusal: only a store fetch sets that flag and it merges once at the end, so the Parquet is identical either way. The view-to-download mapping is three rules because no single rule covers it. LockMutation is exported so the lock ordering keeps one implementation, and Engine.exec exists because running COPY through Query would leak its rows and deadlock Close.
dataset show and dataset list gain materialized_bytes beside size_bytes, which keeps its meaning: it is a documented stable contract that both surfaces publish, so the figure is additive rather than a redefinition. dirSize becomes dirSizes, returning both totals in one walk, so listing does not pay a second walk per dataset. The two warnings live in different places for a reason. The unreferenced-Parquet one needs only the folder listing against the manifest, so it goes straight into driftWarnings, lands inside the existing sort, and reaches the MCP dataset_show tool as well as the CLI without either changing. It covers both the state a reindex leaves and the orphan a discarded materialize run leaves, and its wording deliberately does not invite deletion: an external script may be reading that file, and it is still correct for that reader. Staleness cannot be decided there, because it needs the view's current input list and dataset cannot import duck. So duck exports the check plus one decorator, and both callers of the show contract use it, which is what keeps the two surfaces from publishing different warning sets. That also leaves exactly one implementation of freshness, shared by the reader, the writer and the warning. Reindex needs no code: reindexIdentity builds a fresh manifest, so the entries are gone by construction. It gets a test anyway, covering all three halves at once, since the half that matters is that the Parquet files are still on disk afterwards for the external readers this story exists for.
cc-data dataset materialize <ref> [--force] [--allow-partial], following the shape of the existing dataset subcommands, with progress and prose on stderr. There is no view-selection flag, deliberately: the folder stays a deterministic mirror of the materializable set, so an external script hardcoding a path can tell an absent file from absent data. A refusal exits non-zero, which is the point of reporting per view: a recipe running under set -euo pipefail stops rather than consuming a surface that is missing one. The class is exit 1, the table's internal/other, rather than exit 5, which the help documents as a server contract error; a refusal is local, and mis-classing it would mislead anyone scripting on the code. The MCP tool is annotated neither read-only nor destructive, since it writes only derived data that is regenerable and documented as safe to delete. Registering it turns the drift guard red until the catalog names it, which is the guard working, so the tools.md entry lands here rather than with the rest of the guidance.
…s [REPORT-113] The core says when materializing is worth it and carries both halves of the deletion sentence: the folder is safe to delete at any time, and cc-data itself removes it only when purging or deleting the dataset. The two sit together because the first alone reads like a warning that the tool might remove it, which is exactly what a script hardcoding the path needs to know is untrue. The CLI spelling lives in the skill header, since the core may not name a command. The researcher guide documents materialized/<view>.parquet as a stable path any Parquet reader can open without going through cc-data, which is what the study scripts depend on, and states the two measured duplication costs so the folder's size is expected rather than discovered: reports unions the same log CSVs logs reads, and a view's derived columns are free until they are written down.
Two assertions behind CC_DATA_MATERIALIZE_TIMING, because CI runs plain go test ./... with no -short convention and the fixture is roughly 540MB of generated JSONL. A column-pruned aggregate must be at least 10x faster materialized, measured at about 60x, so the threshold carries six times the headroom rather than being tuned until it passed. A high-cardinality aggregate is only required not to get slower, since that shape is bounded by the aggregation rather than the scan, and the check exists to catch a change that made materializing a pessimization for the queries it does not help. The generator's payloads vary per row deliberately. With an identical blob per row, ZSTD dictionary encoding compresses the column away and the measured speedup collapses, because the cost moves into the aggregation: such a generator measures compression rather than materialization.
…al [REPORT-113] Three requirements the earlier phases left unasserted. The identical-results fixture gains a log run, so logs joins the materializable set and its JSON, TIMESTAMP and LIST columns go through the round trip. Those columns are computed per query from the CSV rather than read from it, and they are the reason this story materializes views rather than artifacts, so the test names them instead of leaving them to the whole-map comparison. The Parquet footer carries the view name and the build time. Nothing in cc-data reads it back, so without a test nothing would have noticed it going missing. --force is not a second way past a missing input; that is --allow-partial's job alone, and the order of the two checks is what makes it so.
run_membership maps to store downloads by type, so a run whose answers and history downloads are both incomplete reached the warning twice and printed "runs 901, 901".
…-113] Every other entry in the tool catalog separates the name from its description with an em dash; this one used a colon.
…T-113] A COPY or row-count error aborted the whole command, so the deferred cleanup threw away the finished temp files of every view that had already succeeded and the manifest was never repointed. A disk filling partway through, or a concurrent get whose cleanup removes a store version mid-copy, cost the entire run rather than the one view it touched. Both now refuse the view and carry on, which is the discipline every other refusal here follows; only a cancelled context still stops the run. Also drops a test comment's reference to the spec document, which outlives the document, and shortens a three-line comment that fits on one.
…ead [REPORT-113] Nothing in the tree installs a signal handler, so interrupting a run kills it before its deferred cleanup can remove the temp files. One was then invisible forever: the unreferenced-Parquet warning matches only names ending .parquet, the derived-subfolder contract hides the folder from reindex and the orphan check, and the next run allocates a fresh name rather than reusing it. On the corpus this feature exists for, that is hundreds of megabytes counted in size_bytes and materialized_bytes with nothing to explain them. A run now holds a dataset-scoped materialize guard from start to finish and sweeps every leftover temp file before it copies. The two halves are one mechanism. Sweeping alone would be unsafe, because the copies deliberately run outside the mutation locks, so a second run's sweep would delete the first run's in-flight file and the first would die on the rename with a bare "no such file or directory"; that was measured before choosing this. The guard removes the possibility, since a live run still holds it and the kernel drops an flock when its holder dies however abruptly, which was measured too, by SIGKILLing a holder and re-acquiring. It also settles something the guard was not added for: two runs on one dataset used to copy every view twice and race at the repoint, with the loser's work discarded. The second now fails as busy, like every other mutating command. get does not take this guard, so the rule that a materialize must not make a concurrent fetch fail as busy is unchanged. The guidance and the researcher guide now say cc-data removes a *finished* Parquet only through purge and delete, since the sweep makes the blanket promise inaccurate for a half-written one.
…EPORT-113] The result carried Written, Skipped and Refused, and the repoint put a discarded copy into Skipped, which the CLI rendered as "already fresh". That is backwards in the one case where it matters: a discarded view is not materialized at all and running again picks it up, so the reader was told the opposite of what they needed to act on. Discarded is now its own status with its own reason, and the CLI names those views and says to run again. The shape changes rather than gaining a fourth list. A view now carries exactly one outcome instead of that holding only by construction, and the MCP tool returns the outcomes as they stand rather than assembling per-status keys by hand. That hand-assembled projection was the real hazard: adding a status later would have left the tool silently omitting it, which is the same two-lists-that-must-agree failure this story already rejected for the materializable predicate and for the show warnings. Callers keep Written(), Fresh(), Discarded() and Refused() accessors, so the CLI, the tests and the exit-code rule read as before. A test now holds the invariant the shape exists for: every materializable view appears exactly once, with a status, in view order.
Collapses requirements.md and implementation.md into one closed summary and removes the folder. The decisions are the part worth keeping, so all 26 are carried across with the options that were actually weighed and why each was settled the way it was, including the five that were reopened during implementation: the sentinel's freshness semantics, the temp-file guard and sweep, the per-view outcome list, a failed copy being per-view evidence, and the exit class for a refusal. Three items are recorded as not yet implemented: trimming the superseded source columns from logs, lifting the study's join keys out of the parameters JSON, and the pre-existing gap where dataset show raises MISSING_FILE for stores and membership but never for a download's report CSVs, which is the condition materialize now refuses on. The first two belong to REPORT-120; the third has no ticket yet.
The Windows job failed because the test provoked a failed copy by removing write permission from the target directory, which Windows ignores for directories, so every view was written and nothing was refused. A store whose bytes will not parse provokes it on every platform instead, and is the realistic shape: read_json with an explicit column map validates no content at CREATE VIEW, so the view registers without complaint and the failure lands in the COPY. That test then caught a leak the old one could not see. A COPY that fails partway leaves the engine's own scratch file beside the target, named after it, and only the file this package created was being removed. The next run's sweep would have cleared it, since the name still carries the temp infix, but a run should not leave litter for the next one to find. Cleanup now matches on the temp name rather than on a fixed prefix, so it does not depend on how the engine spells its scratch file.
There was a problem hiding this comment.
🟡 Changes recommended
Freshness and concurrency gaps can publish or retain stale materializations and disrupt dataset lifecycle operations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds dataset view materialization to Parquet for faster queries and direct access from external analysis tools.
Changes:
- Adds CLI and MCP materialization commands with freshness tracking and per-view outcomes.
- Integrates materialized views into querying, dataset lifecycle, summaries, and warnings.
- Adds documentation and extensive correctness, concurrency, and performance tests.
File summaries
| File | Description |
|---|---|
specs/REPORT-113-dataset-materialization.md |
Defines requirements and design decisions. |
internal/store/lock.go |
Adds the materialization lock. |
internal/mcpserver/types.go |
Defines MCP materialization input. |
internal/mcpserver/tools.go |
Registers materialize and show behavior. |
internal/mcpserver/server_test.go |
Updates MCP tool inventory tests. |
internal/guidance/src/tools.md |
Documents the MCP tool. |
internal/guidance/src/skill_header.md |
Adds CLI materialization guidance. |
internal/guidance/src/core.md |
Explains materialization behavior. |
internal/duck/views.go |
Selects fresh Parquets for views. |
internal/duck/materialized_test.go |
Tests materialized view reads and fallback. |
internal/duck/materialize.go |
Implements materialization and freshness handling. |
internal/duck/materialize_test.go |
Tests materialization outcomes and concurrency. |
internal/duck/materialize_bench_test.go |
Adds opt-in performance assertions. |
internal/duck/engine.go |
Adds raw-view opening and Parquet registration. |
internal/dataset/summary.go |
Reports materialized size and warnings. |
internal/dataset/summary_test.go |
Tests materialization summary fields. |
internal/dataset/reindex.go |
Exposes mutation locking for reindex. |
internal/dataset/reindex_test.go |
Tests derived-folder isolation. |
internal/dataset/manifest.go |
Stores materialization metadata. |
internal/dataset/fingerprint.go |
Implements input freshness fingerprints. |
internal/dataset/fingerprint_test.go |
Tests freshness behavior. |
internal/dataset/dataset.go |
Adds lifecycle and locking support. |
internal/dataset/dataset_test.go |
Tests purge and delete behavior. |
docs/researcher-guide.md |
Documents Parquet usage and lifecycle. |
cmd/dataset.go |
Registers the new subcommand. |
cmd/dataset_show.go |
Displays materialized size and staleness. |
cmd/dataset_materialize.go |
Implements the CLI command. |
cmd/dataset_materialize_test.go |
Tests CLI outcomes and exit codes. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Freshness now covers the view definition, not just the input files. A release that changes a view's projection, derived columns or ordering leaves every input byte untouched, so the older Parquet kept reading as fresh and queries would have returned the previous release's columns indefinitely, which is exactly the promise this feature makes. The same gap covered a manifest-only change racing a run: a dimension view embeds its download's fetch time as a SQL literal, so a reindex re-stamping a zero FetchedAt reorders the dedupe without touching a file, and the repoint would have re-recorded the pre-reindex copy as current. The signature neutralizes the dataset directory and the schema prefix, since neither changes what a view returns and both would otherwise break rename survival and multi-dataset resolution. A recorded entry is no longer enough to call a view fresh. The guidance tells researchers the folder is safe to delete at any time; doing so left the entry fresh, materialize reporting "already fresh" and doing nothing, no warning anywhere, and queries silently falling back to the raw artifacts, with --force the only repair and nothing pointing at it. Rename and delete now take the materialize guard. Its lock file lives inside the directory they move, and the copies hold no mutation lock, so on Windows a concurrent run would have blocked the directory move after rename had already rewritten the manifest name. Both existing functions already release their own handles for this exact reason. dataset list renders the materialized figure, which it published in --json but not in the table, so the requirement that both surfaces report it separately was only half met. And a comment claimed a discarded copy leaves an unreferenced Parquet, which stopped being true when the rename moved after the re-check. Not changed: a view observed fresh and then invalidated by a concurrent get still reports fresh. The observation was true when it was made, the on-disk state is correct, and the same race exists one instruction after the command returns.
emcelroy
left a comment
emcelroy
left a comment
There was a problem hiding this comment.
Looking good 👍 The review below generated with Claude includes a bunch of issues to consider addressing before merge. The first seems the most important.
Branch is green: go build ./..., go vet ./..., gofmt -l . and go test ./... -count=1 all pass.
Ten issues, most serious first. Items marked Confirmed were reproduced by running code.
1. An open session keeps reading an old Parquet after a fetch — Confirmed
Freshness is decided once, when the engine opens (internal/duck/engine.go:102). The repl opens one engine for the whole session (cmd/repl.go:34).
Reproduced: after a file changed, the open session returned 4 rows and missed the new row; a new session returned 5. Control: an open session on a dataset that was never materialized returned 5. So materializing changed the answer, which is the one thing the feature promises it never does.
Fix: re-check freshness per query, or re-register the affected views.
2. A fetch at the end of a run destroys all the work — Confirmed
repointManifest (internal/duck/materialize.go:255) asks for the mutation locks once and gives up if they are held. A fetch holds the activity lock in shared mode for its whole run (internal/fetch/report.go:345), and the mutation locks need that same lock exclusively.
Reproduced: the run ends with dataset is busy, no per-view outcomes, and every finished copy deleted. On a 15 GB dataset that is minutes of work lost because someone else started a get.
Fix: make only the final commit wait, and let the user stop the wait. Three constraints:
- A short fixed timeout is not enough. A paged fetch runs far longer.
- Do not make lock waiting general.
Rename,DeleteandPurgetake the materialize guard while holding the mutation locks. If they waited, and the materialize run waits for the mutation locks, the two would block each other forever. Those must stay non-blocking. - Cancelling needs wiring. The command passes
context.Background()(cmd/dataset_materialize.go:45).
Do not try to keep the temporary files for the next run. They carry no record of the fingerprints and signature they were built from, and the sweep deletes them at the start of every run.
3. A damaged Parquet can pass both checks and then fail the query — Confirmed
parquetReadable runs count(*), which is answered from the file's index at the end. Damage in the middle of the file leaves that index intact.
Reproduced: 16 changed bytes at 20% into the file. The view registered from the Parquet, count(*) succeeded, so materialize treats the copy as good, and SELECT * failed with Invalid Error: Out of buffer. There is no fallback, because the fallback only happens when the view is registered. Damage further into the file hit the index, registration failed, and the fallback worked — so the guard covers most cases, not this one.
Fix: read a small amount of real data in the check, not only the row count.
4. Purge does not take the materialize guard — Confirmed
Rename and Delete now wait for a running materialize. Purge (internal/dataset/dataset.go:298) does not, and it deletes the same folder.
Reproduced on macOS: Purge runs to completion in the middle of the copy loop. Nothing is corrupted, but the run then tells the user to re-fetch runs and reindex, when what really happened is that they purged the dataset. On Windows, deleting a folder can fail when an open file inside it prevents removal, and that would happen after the cleared manifest is already written. Not tested on Windows.
Fix: take LockMaterialize in Purge, as Rename and Delete do.
5. One failed rename throws away the whole commit
repointManifest (internal/duck/materialize.go:283) returns on the first rename error. Views renamed earlier keep their final names, the manifest is never written, and they are already out of built, so cleanup cannot remove them. The result is also returned empty, so successful outcomes are lost.
This contradicts the rule the rest of the file follows: a copy that fails costs its own view, not the run.
Fix: record the view as refused, continue the loop, write the manifest.
6. Over MCP, a partial build looks like a complete one
--allow-partial reports "built from N of M inputs" to warnOut. The MCP tool passes newProgress (internal/mcpserver/server.go:89), which discards everything when the client sends no progress token. The returned outcome is {"status":"written"} with an empty reason — the same as a complete build.
allow_partial is a tool input, so an agent can set it and then cannot see what it caused. A partial copy also replaces a complete one on disk, at a path outside scripts read directly.
Fix: put the input counts and the incomplete-download warning in the returned outcome, not only in the progress messages.
7. Rename and Delete let go of the guard before moving the folder
Both release the materialize guard just before os.Rename, so a new run can start in that gap and open a file inside the folder. On Windows the move can then fail, and Rename has already written the new name into the manifest. The manifest name and the folder name then disagree, and the dataset is hard to address.
Fix: prevent a new run from starting during the move — a guard held outside the folder, or one scoped to the parent. Putting the old name back is only a partial repair, and it would have to re-take the locks and re-read the manifest first, or it would undo someone else's change.
8. UNREFERENCED_MATERIALIZED names a fix that does not work — Confirmed
The warning (internal/dataset/summary.go:236) fires for any .parquet the manifest does not name, and tells the user to run dataset materialize. Materialize never removes a file it does not recognise.
Reproduced: a stray Parquet keeps the warning after two materialize --force runs. The phrase "queries are reading the raw artifacts" also makes no sense for a file that is not a view.
Fix: say that cc-data no longer uses the file, and that it can be kept, moved or deleted. Do not tell the user to delete it: a file that is not in the current view set may still be one an outside script reads.
9. Deleting the folder makes every query noisy and dataset show silent — Confirmed
The documentation says the folder is safe to delete at any time and that the fallback is silent. Reproduced after deleting it: dataset show gives zero warnings, and opening the engine prints one long error per view, with the full path and a piece of SQL. Six on a small fixture, about fifteen on a real dataset, on every query.
Warning here was a deliberate choice (specs/REPORT-113-dataset-materialization.md:248), so the warning itself is right. The problem is its size and the silence in dataset show.
Fix: separate "file is missing" from "file is damaged", print one short line for the missing case, and name the views in dataset show.
10. The attachment exclusion relies on incomplete file declarations
attachment_states and attachment_content read files but never fill in viewStmt.files (internal/duck/views.go:396 and 421), which is the only thing keeping them out of the materializable set.
The exclusion is intended (specs/REPORT-113-dataset-materialization.md:109), but nothing in the code states it. files exists for the degradation warning, so anyone fixing that warning for these two views would switch materialization on for the two views with the largest output.
Fix: exclude them by name with a comment, and add a test using a fixture that has attachments.
Smaller points
- The comment at internal/mcpserver/tools.go:276 says the tool is annotated neither read-only nor destructive. Absent annotations mean destructive, as the repo's own test states (internal/mcpserver/server_test.go:133). Correct the comment. Do not set the hint to false: materialize can replace a complete Parquet with a partial one, so the default is the right one.
MaterializableViews(internal/duck/views.go:636) has no caller outside tests. Unexport it or say who is meant to call it.dataset showreads the manifest twice, once inBuildShowJSONand once inAnnotateShowJSON. A change in between makes the two halves describe different versions. Pass one copy to both.internal/guidance/src/tools.mddescribes the tool as if every view is written and always read from Parquet. The MCP tool description already covers freshness, fallback and refusals, so this is optional.
A query session re-checks each Parquet-backed view's freshness on every query and puts a stale one back on its raw artifacts, so a repl that outlives a fetch answers the same as a session opened after it. The repoint waits for the mutation locks instead of failing busy and deleting every finished copy because a get is running; the wait ends with the caller's context, which the CLI cancels on interrupt. A copy whose rename fails is refused on its own rather than losing the commit. Purge takes the materialize guard like rename and delete. The partial input count and incomplete-run caveat ride on the view outcome so the MCP tool returns them. A missing Parquet warns in one line per view and dataset show names the views (MISSING_MATERIALIZED); the unreferenced warning no longer promises that materialize clears it. The attachment views declare their files and are excluded from the set by name. dataset show builds from one manifest read through duck.ShowJSON, the materializable predicate is unexported, and the tool's annotation comment says what an absent hint means.
|
Thanks for the thorough pass. Eight of the ten are fixed in febe18a, along with the smaller points; two I left as they are, with reasons below. Every "Confirmed" item was reproduced against the code before changing it, and each fix has a test that goes red without it. 1. Open session keeps reading an old Parquet. Agreed, and this was the important one. 2. A fetch at the end destroys the work. Agreed. The repoint tries the locks once and, if busy, prints that it is waiting and blocks on 3. A damaged Parquet passes both checks. Considered and not changed. Reading a small amount of real data only covers the first row group, so it would not catch the 20%-in case you reproduced either, and a full read on every no-op run defeats the point of the check, which is designed to be metadata-only. The failure is a visible query error rather than a wrong answer, which is the same outcome a corrupted raw JSONL gives today, and 4. 5. One failed rename throws away the whole commit. Agreed, fixed: the view is refused with the rename error, the loop continues, and the manifest is written with the views that did rename. 6. Over MCP a partial build looks complete. Agreed, fixed. The partial-input count ("built from 2 of 3 declared inputs (missing: report_700.csv)") and the incomplete-run caveat ("reads run 901 marked incomplete") now ride on 7. Rename and Delete let go of the guard before moving the folder. Not changed. The window is microseconds wide and the consequence is Windows-only, and the activity lock has had the identical window since before this PR: a fetch starting between the release and 8. 9. Deleting the folder makes every query noisy and 10. The attachment exclusion relies on incomplete file declarations. Agreed, fixed. Both attachment views now fill Smaller points. The annotation comment now says an absent hint means destructive and why that is right (an |
Adds
cc-data dataset materialize, which writes each of a dataset's file-backed views to a ZSTD Parquet atmaterialized/<view>.parquetinside the dataset folder. Queries read a view's Parquet while it is fresh and fall back to the raw artifacts otherwise, so materializing never changes an answer, and the file is readable by any DuckDB, pandas or Polars without going through cc-data at all. Closes REPORT-113; the closed spec isspecs/REPORT-113-dataset-materialization.md.Every query today re-reads the JSONL and CSV a fetch produced, which is right for a few thousand answers and wrong at the scale REPORT-110 brings, where a 6.38M-entry history store is 15 GB of JSONL. It is also the last piece of storage machinery the study code owns:
sources/clue/recipes/log-events/build-parquet.shmaintains its own column-type declarations and its own partial-build guard purely because the tool offers neither. REPORT-120 retires that script onto this command.Decisions worth a reviewer's attention
It materializes views, not stores, which is the opposite of what the ticket said. REPORT-120 is the consuming story and the dataset it materializes holds no stores at all: REPORT-119 puts the log events in as a report CSV, REPORT-118's cohort is another report CSV, and REPORT-112's
logsis a union over log-type CSVs. The CLUE stores do not arrive until REPORT-110 at 0.3.0, so store-scoped materialization would have run on REPORT-119's dataset and produced nothing. Materializing views also gives external readers REPORT-112's derived columns rather than the raw CSV's, which is what the study actually consumes. The ticket's persistent DuckDB is gone for a related reason: it has no counterpart incc-data-studies, where every invocation isduckdb -cagainst a fresh in-memory database, and a view stored in a.duckdbfile freezes the absolute Parquet path, sodataset renamebreaks it with a misleading sandbox permission error.Membership of the materializable set is code-derived from two halves, and both halves are load-bearing. A view qualifies when it declares files and is a static view. Declaring files alone selects nothing over an empty manifest, because every builder returns a stand-in that scans nothing, so
MaterializableViewstakes the manifest; without the static half it wrongly admitsreport_<run>, which multiplies with the run count. Both sides go through onebareViewNamehelper, because a statement built for a named dataset carries a schema prefix that neitherStaticViewNamesnor the manifest's map has. An earlier draft trimmed the prefix in the reader but not the predicate, which would have silently disabled the whole feature in any multi-dataset session; the named-dataset test is the only one that can catch that, since the prefix is empty everywhere else.The copies run outside the mutation locks, so a concurrent
getnever fails as busy, and a run that races one is discarded rather than recorded. Input fingerprints are taken just before each copy and compared again at the repoint; rebuilding them at the repoint would compare a value against itself. Temp files are renamed only after that re-check, keeping the manifest write the commit point as it is inmerge.goandPurge.A separate materialize guard is held for the whole run, and that is what makes the temp-file sweep provable rather than heuristic. Nothing in the tree installs a signal handler, so interrupting a run kills it before its deferred cleanup runs, and the leftover was invisible to every diagnostic: the unreferenced-Parquet warning matches only names ending
.parquet, the derived-subfolder contract hides the folder from reindex and the orphan check, andmaterialized_bytescounts it forever. Sweeping alone would have been worse than the leak: measured, a second run's sweep deletes the first run's in-flight file and the first then dies on the rename with a bare "no such file or directory". The guard removes the possibility, because a live run still holds it and the kernel drops anflockwhen its holder dies however abruptly, which was measured by SIGKILLing a holder and re-acquiring.getdoes not take this guard, so the concurrency rule above is unaffected, and two overlapping runs no longer copy every view twice and race at the repoint.Refusals are per view, never per command. A declared input missing from disk is refused unless
--allow-partialis passed, and the reason names both remedies plus the override, so the discoverable move is not to type the flag forever; under--allow-partialthe run reports "N of M", because a count is what makes a shortfall land. A view that registered from its typed-empty fallback is refused with no override at all, becauseCOPYof a degraded view succeeds and writes a 404-byte zero-row Parquet. A store-backed copy is additionally checked against the manifest's recorded count, which is the check both retired build scripts end with. A copy that fails outright, from a full disk or a concurrentgetremoving a store version mid-copy, is per-view evidence too, so it refuses that view rather than discarding every completed copy; only a cancelled context stops the run. The command exits non-zero if anything was refused, which is what stops aset -euo pipefailrecipe.The incomplete-download guard the ticket asked for is demoted to a warning, because it could not have protected anything. Exactly one place in the tree writes
Complete: false, the store fetch, and a paged fetch merges once at the end, so an incomplete store download leaves the Parquet byte-identical to one built if the run had never been requested. Under the obvious file-intersection mapping it could never have fired at all, since store downloads carry noFiles. The incident that justified it is a missing-files incident, which the refusal above covers. The view-to-download mapping the warning does use is three rules rather than one, and the reason is written down so nobody simplifies it back.A run reports one outcome per view rather than a list per status. An earlier shape carried
Written,SkippedandRefused, and a discarded copy landed inSkipped, which the CLI rendered as "already fresh": exactly backwards in the one case where the view is not materialized and running again would fix it. One list makes a view structurally incapable of landing in two statuses or none, and lets the MCP tool return the outcomes as they stand instead of assembling per-status keys by hand, which is the shape that would have silently stopped reporting a status added later.Reindex drops the manifest entries and deliberately does not delete the files. Carrying the entries forward was measured to be almost safe (a reindex of an unchanged dataset leaves 14 of 15 view statements byte-identical) and rejected anyway, because reindex has narrow paths that change a view's output while every input still fingerprints the same, and it is the one command whose purpose is recovery. Deleting the folder is what the "make the bad state impossible" rule points at, and is rejected because
materialized/<view>.parquetis this story's deliverable to tools outside cc-data:studies/clue-dataflow-behavior/lib.py:22-24hardcodes three such paths as its entire data source, so cc-data losing confidence in a file is not grounds for removing it.Staleness and the unreferenced-Parquet warning live in different places on purpose. The unreferenced one needs only the folder listing against the manifest, so it goes into
driftWarnings, lands inside the existing sort, and reaches the MCPdataset_showtool as well as the CLI without either changing. Staleness needs a view's current input list, whichinternal/datasetcannot compute without importinginternal/duck, so it comes fromduckthrough one decorator both callers ofBuildShowJSONuse. That keeps one implementation of freshness serving the reader, the writer and the warning, and stops the two surfaces publishing different warning sets.There is no view-selection flag.
--viewwould have saved single-digit percentages in the regime that matters (measured:reports.parquet5.4 MB beside a 9.5 MBlogs.parqueton a 200,000-row fixture, roughly 7% overlap once REPORT-110's 313 MBhistoryParquet dominates) at the cost of the property the external consumer depends on: without it,materialized/is a deterministic mirror of the dataset, and with it an outside reader cannot distinguish an absent file from an unselected one.Testing
Every new test was run against a deliberate break of the thing it guards rather than assumed to work. Mutations exercised: reverting the purge fix; dropping the
Freshset-size comparison; makingmaterializableFromignore the schema prefix; makingapplyMaterializeda no-op; skipping the post-copy freshness re-check; skipping the store row-count assertion; stopping the degraded-view and missing-input refusals; reporting a discarded copy as written and as fresh; a discard with no reason; dropping refused views from the outcome list; removing the temp sweep; removing the materialize guard; releasing that guard immediately instead of holding it; dropping the Parquet footer metadata; making a failed copy abort the run again; stopping the run-id dedupe in the incomplete warning; making a refusal exit zero; and leaving the engine's scratch file behind after a failed copy.Three of those caught defects in the tests themselves. The derived-folder orphan test was a pure negative assertion until a top-level copy of the same filename was planted as a control, so it could not have failed if the orphan regex had changed. The
Freshset-size comparison survived deletion, because an added input was already caught by the lookup miss and nothing covered the other direction, a view that stops declaring a recorded input. And the guard-duration property was untested until a check ran inside the window where the temp files exist, since taking the guard and releasing it immediately is otherwise indistinguishable from holding it, and is precisely the shape that would make the sweep unsafe.The command was also driven end to end through the built binary rather than fixtures alone: create, reindex, materialize, query reading the Parquet, a file change producing
STALE_MATERIALIZEDwhile the query stays correct, a deleted input refusing per view with both remedies and exit 1,--allow-partialreporting "1 of 2" and exit 0, a planted 195 KiB leftover temp counted inmaterialized_bytesand gone after the next run, and a second run reporting BUSY while another holds the guard.The timing assertion is opt-in behind
CC_DATA_MATERIALIZE_TIMINGbecause the fixture is roughly 540MB of generated JSONL and CI runs plaingo test ./.... Run once for real on a 1M-row fixture: materialized in 1.6s, a column-pruned aggregate 60x faster against a 10x threshold, and a high-cardinality aggregate 4.9x where the test only requires it not to get slower. The generator's payloads vary per row deliberately; with a constant blob, ZSTD dictionary encoding compresses the column away and the measurement becomes one of compression rather than of materialization.One Windows failure on the first CI run, fixed in
a88aba6, and it is worth reading because of what the fix turned up. The test for a failed copy provoked one by removing write permission from the target directory, which Windows ignores for directories, so every view was written and nothing was refused. A store whose bytes will not parse provokes it on every platform instead, and is the realistic shape:read_jsonwith an explicit column map validates no content atCREATE VIEW, so the view registers without complaint and the failure lands in theCOPY. That replacement immediately caught a leak the permission trick could not see: aCOPYthat fails partway leaves the engine's own scratch file beside the target, named after it, and only the file this package created was being removed. The sweep would have cleared it on the next run, since the name still carries the temp infix, but a run should not leave litter for the next one to find.Copilot's review found one problem worth calling out on its own, and it is fixed in
7a7c545along with four others. Freshness keyed on the input fingerprints alone, which see every byte a view reads and nothing about the view itself, so a cc-data release that changed a view's projection, derived columns or ordering left the older Parquet reading as fresh and queries returning the previous release's columns indefinitely.Materializednow records a signature over the view's statement, compared everywhere a copy's usability is decided. The dataset directory and the schema prefix are neutralized before hashing, since neither changes what a view returns and both would otherwise cause false invalidation; the prefix half was not hypothetical, the multi-dataset test went red until it was handled. That same signature closes a manifest-only race, since a dimension view embeds its download's fetch time as a SQL literal, so a reindex re-stamping a zeroFetchedAtreorders the dedupe without touching a file. Three further fixes from that review:dataset renameanddataset deletenow take the materialize guard, because its lock file lives inside the directory they move and Windows cannot rename a directory with an open handle inside it, which would have failed after the manifest name had already been rewritten; a recorded view is only skipped as fresh when its Parquet is still readable, since the folder is documented as safe to delete and doing so previously left a state repairable only through--force, with no warning anywhere; anddataset listrenders the materialized figure it had been publishing in--jsonalone.Measured on the head commit (7a7c545): 29 files changed, 3275 insertions, 54 deletions; 42 new test functions; 83 passing tests in
internal/duckand 69 ininternal/dataset; 16 packages green undergo build ./...,go vet ./...andgo test ./... -count=1, which are the three checks CI runs, plus a cleangofmt -l ..