From 2dc53b1946256ed348bb8fd53daaedac6e593d10 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 05:19:12 -0400 Subject: [PATCH 01/20] spec: materialize a dataset's query surface to Parquet [REPORT-113] 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_.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. --- .../implementation.md | 529 ++++++++++++++++++ .../requirements.md | 300 ++++++++++ 2 files changed, 829 insertions(+) create mode 100644 specs/REPORT-113-dataset-materialization/implementation.md create mode 100644 specs/REPORT-113-dataset-materialization/requirements.md diff --git a/specs/REPORT-113-dataset-materialization/implementation.md b/specs/REPORT-113-dataset-materialization/implementation.md new file mode 100644 index 0000000..23a65ef --- /dev/null +++ b/specs/REPORT-113-dataset-materialization/implementation.md @@ -0,0 +1,529 @@ +# Implementation Plan: Dataset materialization (Parquet query surface) + +**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-113 +**Requirements Spec**: [requirements.md](requirements.md) +**Status**: **In Development** + +## Shape of the change + +Three facts, all verified against the running engine, set the shape and remove most of the design freedom: + +- **`COPY TO` works under the fully locked sandbox** and is refused for a path outside the allowlist. So materialization reuses the ordinary `duck.Open` path rather than needing a privileged writer, and the sandbox keeps constraining where it can write. Nothing about the trust boundary changes. +- **`COPY (SELECT * FROM "")` preserves the view's exact column set and types**, JSON included, for both store-backed and CSV-union views. So materialization needs no access to how a view's SQL was built. It creates the views the normal way and copies them out by name, which means no refactor of `views.go`'s statement construction. +- **`internal/duck` imports `internal/dataset`, never the reverse.** Materialization therefore lives in `duck`, the manifest types live in `dataset`, and `reindex` cannot read a Parquet. + +The engine's registration loop gains one level. Today it tries `primary`, then `fallback`. It becomes: try the materialized statement if there is one, then `primary`, then `fallback`. Every existing view keeps its exact behavior when nothing is materialized, which is what keeps this change small. + +--- + +## Register derived subfolders and make purge remove them + +**Summary**: Adds the named predicate the requirements ask for and fixes the verified purge gap. Independent of everything else, so it lands first and alone. + +**Files affected**: +- `internal/dataset/dataset.go`: the predicate, the folder name, `deleteArtifacts` +- `internal/dataset/dataset_test.go`: purge coverage +- `internal/dataset/reindex_test.go`: the invisibility contract + +**Estimated diff size**: ~130 lines + +```go +// MaterializedDir is the derived subfolder holding a dataset's Parquet query surface. +const MaterializedDir = "materialized" + +// derivedSubdirs are subfolders holding data cc-data generated from the dataset's own +// artifacts. They are never adopted as artifacts, never reported as orphans, and are +// removed wholesale by purge and delete. REPORT-89 registers its exports folder here. +var derivedSubdirs = map[string]bool{MaterializedDir: true} + +// IsDerivedSubdir reports whether a directory name inside a dataset holds derived data. +func IsDerivedSubdir(name string) bool { return derivedSubdirs[name] } +``` + +In `deleteArtifacts`, the directory arm currently removes `segments` and `attachments` by literal name; it gains `|| IsDerivedSubdir(name)`. `Purge`'s other half, clearing `m.Materialized`, waits for the step that adds that field; putting it here would not compile. + +Reindex is deliberately **not** added to the predicate's contract. It drops the manifest entries but leaves the files, because `materialized/.parquet` is this story's deliverable to tools outside cc-data and stays correct for them; cc-data losing confidence in a file is not grounds for cc-data to remove it. The same reasoning covers REPORT-89's `exports/`, which is even more clearly a product than a cache. + +Tests are the point of this step, because the invisibility properties currently hold by accident: + +- Purge removes a populated `materialized/`. This fails before the change (verified: all three planted files survived a purge). +- Delete removes it too. This needs **no code**: `Delete` renames the whole dataset directory to a tombstone and `RemoveAll`s it (`internal/dataset/dataset.go:179`), so a subfolder goes with it. The test exists because the requirement names delete, and because it can fail: it goes red the moment someone rewrites `Delete` to remove known children selectively, which is how `Purge` came to have this gap in the first place. +- A file planted in `materialized/` that *would* be adopted at the top level (`answers.v99.jsonl`, `report_4242.csv`, `members_answers_5.v9.jsonl`) is not adopted by `Reindex` and raises no `ORPHAN_FILE` warning. This passes before the change; it is written to hold the contract against a future change that makes either walk subdirectories. + +--- + +## Record materialization in the manifest + +**Summary**: The manifest types and the fingerprint, with no producer or consumer yet. Splitting it out keeps the engine and writer steps reviewable on their own. + +**Files affected**: +- `internal/dataset/manifest.go`: the `Materialized` entry and its map +- `internal/dataset/dataset.go`: `Purge` clearing `m.Materialized` +- `internal/dataset/fingerprint.go` (new): fingerprint computation +- `internal/dataset/fingerprint_test.go` (new), `internal/dataset/dataset_test.go` + +**Estimated diff size**: ~160 lines + +```go +// Materialized records one view's Parquet and the inputs it was built from. Freshness is +// per input rather than per store version because report CSVs have no version: report_.csv +// is a fixed name overwritten on re-fetch. +type Materialized struct { + File string `json:"file"` // dataset-relative, e.g. materialized/logs.parquet + Inputs map[string]string `json:"inputs"` // dataset-relative input path -> fingerprint + BuiltAt time.Time `json:"built_at"` +} +``` + +`Manifest` gains `Materialized map[string]Materialized \`json:"materialized,omitempty"\`` keyed by view name, and `ensureMaps` initializes it. `Purge` clears it alongside the four maps it already resets, in the same step because that is where the field appears: committing a manifest that still names files the same call is about to delete is what `Purge`'s own commit-ordering comment forbids. No manifest version bump: `decodeManifest` already tolerates an absent field, and an older binary reading a newer manifest ignores the key rather than failing. + +```go +// Fingerprint identifies a file's content cheaply, as size and modification time. This is what +// Go's own build cache uses. It cannot distinguish a rewrite that preserved both, which a real +// re-fetch does not do; a content hash would be exact and would cost a full read of every input, +// which at 15 GB defeats the purpose of materializing. +// FingerprintAbsent is what a declared input that is not on disk records. It cannot collide +// with a real fingerprint, which is always -. A union view keeps declaring a CSV +// that has gone missing (internal/duck/views.go:218 appends dl.Files whether or not the file is +// there), so this is a normal state rather than an error, and Fresh must be able to tell "absent +// then and absent now" from "absent then, present now". +const FingerprintAbsent = "absent" + +func Fingerprint(path string) string { + fi, err := os.Stat(path) + if err != nil { + return FingerprintAbsent + } + return fmt.Sprintf("%d-%d", fi.Size(), fi.ModTime().UnixNano()) +} + +// Fresh reports whether the Parquet may be read: the view's current inputs are exactly the +// recorded ones, and each still fingerprints the same. The caller supplies the current list +// because only the engine knows which files a view reads. +// +// Comparing the set, not just the recorded entries, is load-bearing. A run downloaded since the +// materialization adds a file to a union view, and a Parquet built without it is stale even +// though every recorded input still matches perfectly. +func (m Materialized) Fresh(dir string, current []string) bool +``` + +Tests: a fingerprint changes when content is rewritten; `Fresh` is true unchanged, false after a rewrite, false when an input is deleted, and false when an input is added. And the absent round trip, verified ahead of implementation against the real view set: materialize a `reports` view one of whose CSVs is missing, and `Fresh` is true immediately afterwards and false the moment the CSV is restored from a backup. + +--- + +## Teach the engine to prefer a fresh materialized view + +**Summary**: The read path. After this step a hand-placed Parquet is used, which is what makes the step testable before a writer exists. + +**Files affected**: +- `internal/duck/views.go`: `viewStmt.materialized`, the resolver, `MaterializableViews(m)` +- `internal/duck/engine.go`: three-level registration +- `internal/duck/materialized_test.go` (new) + +**Estimated diff size**: ~210 lines + +`viewStmt` gains one field, and `viewSet` gains the resolved map: + +```go +type viewStmt struct { + name string + materialized string // optional read_parquet form, tried before primary + primary string + fallback string + files []string +} +``` + +`statements()` is unchanged. A single post-pass applies materialization, which keeps every builder untouched: + +```go +// applyMaterialized points a view at its Parquet when the manifest records one that is fresh. +// Membership of the materializable set is decided by materializableFrom over these same +// statements, so the writer and the reader can never disagree about which views have a Parquet. +// +// There is deliberately no presence check. A Parquet that is absent, corrupt or truncated all +// fail at CREATE VIEW rather than at query time, so the registration loop's warning covers every +// shape a bad file takes, and a stat per view per Open would buy only a differently worded +// message. A stale entry is silent by contract; an unreadable one is not. +func (vs viewSet) applyMaterialized(stmts []viewStmt) []viewStmt { + eligible := map[string]bool{} + for _, n := range materializableFrom(stmts, vs.prefix) { + eligible[n] = true + } + for i, st := range stmts { + bare := bareViewName(st.name, vs.prefix) + if !eligible[bare] { + continue + } + mat, ok := vs.m.Materialized[bare] + if !ok || !mat.Fresh(vs.canonDir, st.files) { + continue + } + stmts[i].materialized = fmt.Sprintf("CREATE VIEW %s AS SELECT * FROM read_parquet(%s)", + st.name, vs.file(mat.File)) + } + return stmts +} +``` + +`materializableFrom` takes the statements rather than the manifest because `applyMaterialized` +already holds them; the exported `MaterializableViews(m)` builds them once and delegates, so +`Open` never builds the view set twice. It takes the prefix with them, because a statement built +for a named dataset carries one (`"alpha"."answers"`) while the set is keyed on the bare name +`StaticViewNames` produces. Both callers go through `bareViewName` so there is one answer to what +a statement's view is called, rather than two that can disagree about the prefix. + +Registration in `Open` becomes an ordered attempt. The distinct warning matters: a materialized copy that fails is a different event from a view degrading to empty, and conflating them would hide a corrupt Parquet behind a message about missing artifacts. + +```go +for _, stmt := range vs.statements() { + if stmt.materialized != "" { + if _, err := conn.ExecContext(ctx, stmt.materialized); err == nil { + continue + } else { + fmt.Fprintf(warnOut, "warning: materialized %s is unreadable (%v); reading the raw artifacts instead\n", stmt.name, err) + } + } + // existing primary-then-fallback, unchanged +} +``` + +```go +// MaterializableViews returns the views of this dataset that can be materialized: those that +// scan files, minus the per-run views. Both halves are code-derived, so a view added later +// joins or stays out of the set by its own construction rather than by a list someone updates. +// +// It takes the manifest because "declares files" is a property of a populated dataset: over an +// empty manifest every builder returns its stand-in and no view declares anything. +func MaterializableViews(m *dataset.Manifest) []string { + vs := viewSet{m: m} + return materializableFrom(vs.statements(), vs.prefix) +} + +// materializableFrom takes statements the caller has already built, so Open does not build the +// full view set twice: applyMaterialized has the slice in hand and passes it straight through. +// Those statements carry the schema prefix of whichever dataset they were built for, so the +// caller passes it too and the set stays keyed on bare names. +func materializableFrom(stmts []viewStmt, prefix string) []string { + static := map[string]bool{} + for _, n := range StaticViewNames() { + static[n] = true + } + var out []string + for _, st := range stmts { + bare := bareViewName(st.name, prefix) + if len(st.files) > 0 && static[bare] { + out = append(out, bare) + } + } + return out +} + +// bareViewName is a statement's view name with its schema prefix and quoting removed. That is +// the form StaticViewNames produces and the form the manifest's Materialized map is keyed on, +// so every lookup against either has to go through it. +func bareViewName(name, prefix string) string { + return strings.Trim(strings.TrimPrefix(name, prefix), `"`) +} +``` + +The writer and the tests both consume the exported form rather than either hand-listing views. Verified against a populated fixture: the set comes out as `answers`, `logs`, `report_prompts`, `reports`, `run_membership`, `student_id_mapping`, `student_metadata`, plus `history` once that store exists, and the four VALUES- and `read_text`-backed views are excluded by declaring no files, exactly as the requirements state. + +Tests, all driven by materializing a Parquet that carries one sentinel row the raw artifacts do not, so "the Parquet is the source" is proved rather than assumed: + +- The sentinel is returned when the entry is fresh, and not returned when it is stale, when the Parquet is deleted, or when it is corrupt. Going stale is silent; a deleted, corrupt or truncated Parquet must warn and read the raw artifacts, never degrade to empty. All three unreadable shapes fail at `CREATE VIEW`, verified ahead of implementation, which is what lets one warning cover them. +- **The sentinel survives `dataset rename`.** Verified ahead of implementation: materialize, rename the dataset, re-open, sentinel still returned. This is the property whose absence disqualified the persistent DuckDB, so it is pinned rather than assumed. +- **Multi-dataset resolution.** Two datasets opened together, one materialized: `"alpha"."answers"` returns the sentinel and `"beta"."answers"` does not. Verified ahead of implementation. This is the test that covers `bareViewName`, and it has to be a *named*-dataset test to cover it at all: the prefix is empty in every single-dataset session, so a mistake in trimming it off is invisible to every other test in this plan, and it costs the whole feature rather than one view, since `materializableFrom` returning an empty set means no view is ever pointed at its Parquet. +- `MaterializableViews(m)` over a populated fixture matches the expected names, so a view added later that declares files is noticed here. + +--- + +## Add the materialize writer + +**Summary**: The producer. Opens the dataset with materialization suppressed, copies each materializable view out, and repoints the manifest. + +**Files affected**: +- `internal/duck/materialize.go` (new) +- `internal/duck/engine.go`: `OpenRaw`, the unexported `exec`, and the degraded-view set `OpenRaw` reports +- `internal/duck/materialize_test.go` (new) + +**Estimated diff size**: ~380 lines + +```go +type MaterializeOptions struct { + Force bool + AllowPartial bool + Progress func(view string, i, n int) +} + +type MaterializeResult struct { + Written []string // views materialized + Skipped []string // views already fresh (no --force) + Refused map[string]string // view -> why it was not materialized +} + +func Materialize(ctx context.Context, d *dataset.Dataset, opts MaterializeOptions, warnOut io.Writer) (MaterializeResult, error) +``` + +The order is load-bearing and mirrors `mergeUnderLock`'s discipline, for the reason the requirements give: the copy must not hold the mutation locks, or a concurrent `get` fails as busy for its whole duration. + +1. Read the manifest under the per-dataset lock, then release. Warn here, without refusing, for every `!Complete` download feeding a materializable view, naming the runs: only the store fetch can set that flag and it leaves the materialized bytes unchanged, so there is nothing to gate. The view-to-download mapping is three rules, per the requirements: file intersection for report-backed views, download `Type` for `answers` and `history`, and `Type` again for `run_membership`, since an incomplete run has no membership file for a per-run mapping to find. +2. Open an engine over the dataset with materialization suppressed, so a view is always copied from its raw artifacts and a stale Parquet can never be copied forward into a fresh-looking one. `Open` has three non-test callers (`cmd/query.go:60`, `cmd/repl.go:34`, `internal/mcpserver/tools.go:375`), so rather than changing its signature at all four sites this is a sibling `OpenRaw` sharing one unexported constructor with `Open`. +3. Per view, in order: refuse it when `OpenRaw` registered it from its `fallback` statement, which is total loss and has no override; refuse it when it declares an input that is not on disk and `--allow-partial` was not passed, naming the files; skip it when the manifest's entry is fresh and `--force` was not passed. Otherwise `COPY (SELECT * FROM "") TO '' (FORMAT parquet, COMPRESSION zstd, KV_METADATA {...})` into `materialized/.parquet.tmp-`, and stop there. For a store-backed view, assert the Parquet's row count against `m.Stores[typ].Count` and refuse the view on a mismatch. The KV metadata carries the view name and build time as human-readable provenance; nothing reads it back. +4. Re-take the lock, re-read the manifest, repoint only the views whose inputs still fingerprint as they did when copied, `fsutil.RenameAtomic` those temp files into place, delete the rest, and write the manifest last. Renaming after the re-check rather than before is what keeps the manifest write the commit point, as it is in `mergeUnderLock` and `Purge`; renaming in step 3 would leave a complete, unreferenced Parquet inside a folder that reindex and the orphan check are contractually blind to. A view whose inputs moved is discarded and reported as skipped: the Parquet describes a state that is already gone. + +The refusals in step 3 are per view rather than per command, because the evidence is per view. Clean views are still materialized, `Refused` carries a reason per view for the caller to render, and the command exits non-zero if the map is non-empty. + +Two message-content requirements ride on `Refused` and on the progress writer, and neither is decoration: + +- A missing-input refusal names the files **and both remedies**, because the point of refusing is to surface them rather than to leave a flag to be typed forever: re-fetch the run, or run `dataset reindex` to drop the download entry so the file stops being expected. Without the remedies the researcher's only discoverable move is `--allow-partial` on every run afterwards, which is the erosion the flag was kept separate from `--force` to avoid. + +``` +view reports declares 2 inputs and 1 is missing: report_200.csv + re-fetch run 200, or run cc-data dataset reindex to drop the entry so the file stops being expected + to build from what is on disk anyway, pass --allow-partial +``` + +- Under `--allow-partial` the run reports how many of the declared inputs it built from, per view, mirroring `clue-documents/build-parquet.sh:60`'s "--allow-partial given; building from N of M documents". A count is what makes a shortfall land: "1 of 2" is read, "proceeding anyway" is not. + +``` +--allow-partial given; building reports from 1 of 2 declared inputs +``` + +Both go through `output.Progressf` to stderr, like every other `dataset` subcommand's prose. + +The suppression flag in step 2 is the subtle part. Without it, `Materialize` would open the dataset, the engine would helpfully point `logs` at yesterday's Parquet, and the copy would write that Parquet back out with today's fingerprints attached, laundering stale data into a fresh-looking entry. + +Tests: materializing then querying returns identical rows and column types for every view in `MaterializableViews()`; a second run skips everything; `--force` rewrites; a view whose declared input is missing is refused while every other view is still written, and proceeds under `--allow-partial`; a view that fell back to typed-empty is refused with `--allow-partial` given; a store-backed Parquet short of `Stores[typ].Count` is refused; an incomplete download warns and blocks nothing; a `get` between the copy and the repoint leaves the view unmaterialized rather than wrongly fresh (driven through the same `testHookAfterWrite` seam `merge.go` uses); and no `.tmp-` file survives any of those paths. + +--- + +## Report materialization in show and list + +**Summary**: The user-visible reporting, separated from the mechanics so its contract change is reviewable on its own. + +**Files affected**: +- `internal/dataset/summary.go`: `materialized_bytes`, the unreferenced-Parquet warning +- `internal/duck/materialize.go`: `StaleMaterializedViews`, `AnnotateShowJSON` +- `cmd/dataset_show.go`, `internal/mcpserver/tools.go`: both callers annotate +- `internal/dataset/summary_test.go` + +**Estimated diff size**: ~190 lines + +`ShowJSON` and `ListRowJSON` each gain `MaterializedBytes int64 \`json:"materialized_bytes"\``. Both are additive: `size_bytes` keeps its meaning, since it is a documented stable contract that `dataset list` also publishes. + +`dirSize` currently walks the tree once. It becomes a single walk returning both totals, so `dataset list` does not pay a second walk per dataset: + +```go +// dirSizes returns the dataset's total bytes and the subset under registered derived +// subfolders, in one walk. Derived bytes are the ones a researcher can reclaim for free. +func dirSizes(dir string) (total, derived int64) +``` + +One warning is added, matching the existing `MISSING_FILE` / `INCOMPLETE` / `ORPHAN_FILE` style: + +Two warnings are added, matching the existing `MISSING_FILE` / `INCOMPLETE` / `ORPHAN_FILE` style, and they live in different places for a reason worth stating. + +``` +UNREFERENCED_MATERIALIZED: materialized/logs.parquet is on disk but the manifest no longer records it; queries are reading the raw artifacts, run cc-data dataset materialize to restore it (external tools can still read the file) +STALE_MATERIALIZED: view logs was materialized from inputs that have since changed; run cc-data dataset materialize to refresh it +``` + +The first needs no view knowledge, only the folder listing against `m.Materialized`, so it goes straight into `driftWarnings`, lands inside the existing `sort.Strings`, and reaches every caller of the show contract without any of them changing. It covers both the state a reindex leaves and the orphan a discarded materialize run leaves. Its wording deliberately does not tell the researcher to delete anything: an external script may be reading that file, and it is still correct for that reader. + +**The second cannot be produced by `driftWarnings`.** Deciding staleness needs the view's current input list, which only `internal/duck` knows, and `internal/dataset` cannot import it. So `duck` exports the check plus one decorator, and both callers of `BuildShowJSON` use it: + +```go +// StaleMaterializedViews names the views whose recorded Parquet no longer matches the inputs. +// It lives here, not in dataset, because a view's input list is a property of the view set. +func StaleMaterializedViews(d *dataset.Dataset) ([]string, error) + +// AnnotateShowJSON appends the warnings only the view set can decide, and re-sorts. Both +// surfaces call it: the CLI at cmd/dataset_show.go:32 and the MCP dataset_show tool at +// internal/mcpserver/tools.go:193, which publish the same documented ShowJSON contract. +func AnnotateShowJSON(d *dataset.Dataset, s *dataset.ShowJSON) error +``` + +Neither file imports `duck` today, so both gain the import. Routing them through one decorator rather than letting each remember is what keeps the two surfaces from publishing different warning sets, and keeps the re-sort inside the function that owns the ordering invariant. + +That also keeps exactly one implementation of freshness, shared by the reader, the writer and the warning. A second copy in `dataset` would be the "one source of truth" problem in its most literal form: two answers to "is this Parquet usable" that can disagree. + +`renderShow` prints the derived figure beside the total when non-zero. `reindex` needs no code to drop the entries: `reindexIdentity` constructs a fresh `Manifest` without a `Materialized` map, so they are already gone by construction. Three tests rather than one change: the entries are gone, `dataset show` then raises `UNREFERENCED_MATERIALIZED`, and the Parquet files are still on disk. The last is the one that catches a later "tidy-up" making reindex delete the folder, which would break the external readers this story exists for. + +--- + +## Wire the CLI command and the MCP tool + +**Summary**: The two surfaces, together, because the drift guard checks the MCP tool inventory in both directions and would go red on a split. + +**Files affected**: +- `cmd/dataset_materialize.go` (new), `cmd/dataset.go` +- `cmd/dataset_materialize_test.go` (new) +- `internal/mcpserver/tools.go`, `internal/mcpserver/server_test.go` + +**Estimated diff size**: ~250 lines + +The command follows the shape of the existing `dataset` subcommands exactly: `resolveExistingRef`, `echoRef`, `notFound`, `mutationErr`, and progress through `output.Progressf` so prose goes to stderr. + +``` +cc-data dataset materialize [--force] [--allow-partial] +``` + +There is no view-selection flag, deliberately: `materialized/` stays a deterministic mirror of the materializable set, so an external script hardcoding a path can tell an absent file from absent data. The command renders `MaterializeResult` as written, skipped and refused counts, names each refused view with its reason, and returns a non-zero exit when any view was refused, so a `set -euo pipefail` recipe stops rather than consuming a surface missing a view. + +The MCP tool is annotated neither read-only nor destructive: it writes, but only derived data that is regenerable and documented as safe to delete. `server_test.go:109`'s pinned inventory gains `dataset_materialize`, which is what makes forgetting the catalog entry a CI failure rather than a discovery. + +Tests, following `cmd/get_report_test.go`'s shape: a clean run exits zero and names the views written; a run where one view's input is missing exits **non-zero** while the other views are still written, and the message carries both remedies; the same run under `--allow-partial` exits zero and reports "N of M" for the affected view. The exit code is the assertion that matters, since it is what stops a `set -euo pipefail` recipe from consuming a surface missing a view, and it is the one always-on test in the requirements with no home anywhere else in this plan. + +--- + +## Single-source the guidance and the researcher guide + +**Summary**: The prose the drift guard requires, plus the external-path documentation the study scripts depend on. + +**Files affected**: +- `internal/guidance/src/core.md`: when to materialize, and that cc-data never removes the folder on its own +- `internal/guidance/src/skill_header.md`: the CLI spelling +- `internal/guidance/src/tools.md`: the tool catalog entry +- `docs/researcher-guide.md`: the `materialized/` layout + +**Estimated diff size**: ~140 lines + +The split between core and skill header is forced by `TestCoreNamesNoCommand`, which rejects any "`cc-data `" spelling in the core. So the core says *when* materializing is worth it, and carries both halves of the deletion sentence: the folder is safe for the researcher to delete at any time, and cc-data itself removes it only through purge and delete. The two sit together because the first alone reads like a warning that the tool might remove it. The skill header carries the command spelling. The tool catalog entry is what `TestGuidanceDocumentsEveryTool` requires. + +The researcher guide gains the layout as a documented, stable contract, since REPORT-120 depends on reading it directly: + +``` +//datasets//materialized/.parquet +``` + +with three notes. These are ordinary Parquet files carrying each view's own columns, readable by any DuckDB, pandas or Polars without cc-data. They are safe for the researcher to delete at any time, **and** cc-data itself removes them only through `dataset purge` and `dataset delete`; the two sentences sit together, because read alone the first one reads like a warning that the tool might delete them, which is exactly what a script hardcoding a path needs to know is untrue. And the folder is larger than the view a study reads, for two measured reasons: `reports` unions the same log CSVs `logs` reads, so both carry those rows (5.4 MB beside 9.5 MB on a 200,000-row fixture, roughly 7% overlap once REPORT-110's 313 MB `history` dominates), and a view's derived columns become disk when materialized rather than being computed per query (`parameters` 2.62 MB beside `parameters_json` 2.62 MB, 45% of `logs.parquet` in total). + +--- + +## Add the opt-in timing test + +**Summary**: The measurement, gated so it does not run in CI. Last because it is the only step nothing else depends on. + +**Files affected**: +- `internal/duck/materialize_bench_test.go` (new) + +**Estimated diff size**: ~150 lines + +Skipped unless an environment variable is set, because CI runs plain `go test ./...` with no `-short` convention and the fixture is roughly 455MB of generated JSONL. + +Two assertions, and the fixture generator carries a comment saying why its payloads vary: a first attempt used an identical blob per row, ZSTD dictionary encoding took the Parquet to 0.9% of the JSONL, and the measured speedup collapsed from 54x to 2.6x because the cost had moved entirely into the aggregation. A generator emitting a constant blob measures compression, not materialization. + +- A column-pruned aggregate (`GROUP BY question_id`) is at least 10x faster materialized. Measured 54x at 1M rows, so the threshold carries five times the headroom rather than being tuned until it passed. The test names the shape and says column pruning is the mechanism under test, so nobody later "fixes" a regression by swapping the query. +- A high-cardinality aggregate (`count(DISTINCT remote_endpoint)`, measured 3.7x) is **not slower**. That is a not-worse check rather than a speedup check, and it is what would catch a change that made materialization a pessimization for the shapes it does not help. + +--- + +## Open Questions + +None. + +## Self-Review + +Roles chosen for what this document has to survive: the engineer reviewing the resulting commits (are the steps really independent, does step N compile without N+1), the engineer writing the named tests (can each be written against the harness this plan provides), and the operator. Every finding was checked by running code against the real view set; the ones that did not survive are not recorded. + +### Commit reviewer + +#### RESOLVED: The materializable predicate, as written, selected nothing + +The plan derived `MaterializableViews()` "from the statement set over an empty manifest filtered to those declaring files", copying how `StaticViewNames()` works. Ran it: over an empty manifest, **0 of 12** views declare files. Every builder returns its stand-in when there is nothing to read, and a stand-in scans no files, so the function would have returned an empty set and materialization would have silently done nothing on every dataset. + +"Declares files" is a property of a *populated* manifest, unlike "is a static view", which is exactly why `StaticViewNames` can use an empty one. `MaterializableViews` now takes the manifest. + +#### RESOLVED: The same predicate wrongly included the per-run report views + +Over a populated manifest the views declaring files came out as `reports`, `report_prompts`, `answers`, `run_membership` and **`report_7`**. `perDownloadViews` sets `files: dl.Files` on both `report_` and `report__job_` (`internal/duck/views.go:451`, `:456`), so the bare predicate contradicts the requirements' Out of Scope, which excludes per-run views because they multiply with the run count. + +Fixed by intersecting with `StaticViewNames()`. Both halves stay code-derived, so a view added later joins or stays out of the set by its own construction rather than by a list someone remembers to update. The requirements' wording is corrected to match, since it stated the bare predicate too. + +#### RESOLVED: `Fresh` was declared with one signature and called with another + +Declared as `Fresh(dir string) bool` in the manifest step and called as `mat.Fresh(vs.canonDir, st.files)` in the engine step, one step later. The prose underneath already said the caller passes the view's current file list, so the declaration was simply stale. Step 2 would not have compiled against step 3. + +### Test author + +#### RESOLVED: The staleness warning cannot be written where the plan put it + +The plan put `STALE_MATERIALIZED` in `driftWarnings`, which lives in `internal/dataset`. Deciding staleness needs the view's current input list, and that is a property of the view set in `internal/duck`, which `dataset` cannot import: the dependency runs `duck` to `dataset` and never back (`internal/duck/engine.go:13`). Writing it as planned would have forced a second view-to-files mapping inside `dataset`, which is two answers to "is this Parquet usable" that can drift apart. + +Moved to a `duck.StaleMaterializedViews` helper called from `cmd/dataset_show.go`, which already imports both, so one implementation of freshness serves the reader, the writer and the warning. + +### Operator + +#### RESOLVED: Changing `Open`'s signature touches every caller for one internal need + +Materialization needs an engine that ignores existing Parquet, which the plan added as a flag on `Open`. `Open` has three non-test callers (`cmd/query.go:60`, `cmd/repl.go:34`, `internal/mcpserver/tools.go:375`), none of which has any interest in the flag. Changed to a sibling `OpenRaw` over a shared unexported constructor, leaving all three untouched. + +#### Checked and not a problem: materialize inherits the sandbox's 2GB memory cap + +Because materialization reuses the sandboxed engine, its `COPY` runs under `memory_limit = 2GB` (`internal/duck/engine.go:111`), which looked like a risk against a 15 GB corpus. Already measured: the 6.38M-row, 2.92 GB fixture materialized in 5.0s under exactly that cap, because `COPY` streams rather than buffering. Recorded rather than actioned, so the next person does not re-open it. + +--- + +## Self-Review (round 2) + +Roles: the engineer reviewing the resulting commits, the engineer writing the named tests, and the operator. Each finding was checked against the running code; two candidates that did not survive are noted at the end. + +### Commit reviewer + +#### RESOLVED: `cmd/dataset_show.go` does not import `duck`, and it is not the only caller of `BuildShowJSON` + +The plan places `STALE_MATERIALIZED` in `cmd/dataset_show.go` "which already imports both". It does not: its imports are `fmt`, `sort`, `text/tabwriter`, `internal/dataset`, `internal/output` and `cobra`. Adding the import is trivial, so the step still works, but the reason given for choosing that file is not the reason that holds. + +The consequence that does matter is the second caller. `BuildShowJSON` has exactly two non-test callers: `cmd/dataset_show.go:32` and `internal/mcpserver/tools.go:193`, where the `dataset_show` tool returns the summary struct unchanged. Under the plan the CLI shows the staleness warning and the MCP tool does not, so the same documented `ShowJSON` contract carries different warning sets on the two surfaces. Since materialize is itself exposed over MCP, the surface that can act on the warning is the one that never sees it. + +A smaller version of the same problem: `driftWarnings` ends with `sort.Strings(warnings)` (`internal/dataset/summary.go:236`), so warnings appended by a caller land after the sort and break the ordering the struct has always had. + +Suggested resolution: have both callers go through one decorator rather than each remembering, for instance a `duck.AnnotateShowJSON(d, s) error` that appends the warnings and re-sorts, called from `cmd/dataset_show.go` and from the MCP tool. One implementation, two call sites, and the sort stays inside the function that owns the invariant. + +**Resolved as suggested, and the Q2 resolution shrinks it.** The unreferenced-Parquet warning needs no view knowledge, so it goes straight into `driftWarnings` and reaches both surfaces for free. Only `STALE_MATERIALIZED` still needs `internal/duck`, and it goes through one `duck.AnnotateShowJSON(d, s) error` that appends and re-sorts, called from `cmd/dataset_show.go:32` and `internal/mcpserver/tools.go:193`. Both call sites gain the import; the file list for this step gains `cmd/dataset_show.go` and `internal/mcpserver/tools.go`. + +#### RESOLVED: Step 3 renames into place, but step 4 discards by removing "its temp file" + +The two steps disagree about when the rename happens. Step 3 copies to a temp name and renames to `materialized/.parquet`; step 4 then says a view whose inputs moved "is discarded, its temp file removed". After step 3 there is no temp file: what exists is a complete, final-named Parquet that the manifest does not reference. + +That orphan is unusually well hidden. It sits inside a registered derived subfolder, so by this spec's own contract `Reindex` will not adopt it and the orphan-file check will not flag it, and it is counted in `materialized_bytes` forever. It also inverts the commit discipline the rest of the package follows, where the manifest write is the commit point and files land after it (`internal/dataset/dataset.go:268`, `internal/dataset/merge.go:146`). + +Suggested resolution: hold the temp name until after the repoint. Copy every view to `materialized/.parquet.tmp-`, re-take the lock, re-check the fingerprints, then rename the survivors and delete the rest, and write the manifest last. Add a test asserting no `.tmp-` file survives a discarded run, which the plan already lists but which cannot pass under the current ordering. + +**Resolved as suggested.** Step 3 copies to `materialized/.parquet.tmp-` and stops there; step 4 re-takes the lock, re-checks the fingerprints, renames the survivors with `fsutil.RenameAtomic`, deletes the rest, and writes the manifest last, so the manifest write stays the commit point as it is in `merge.go` and `Purge`. The named test for no surviving `.tmp-` file can then actually fail, which under the old ordering it could not. + +#### RESOLVED: The writer needs a statement-executing path that `Engine` does not expose + +`Engine`'s only exported method is `Query`, which returns `*sql.Rows`. Running `COPY ... TO` through it and dropping the result leaks the rows and deadlocks `Close`: reproduced here, and the test hung for the full ten-minute timeout in `sql.(*Conn).close` waiting on `closemu` while five Parquet files sat correctly written on disk. `Materialize` lives in `package duck` so it can reach `e.conn` directly, but the writer step's file list names only `materialize.go` and its test, and the choice is worth making deliberately rather than discovering. + +Suggested resolution: add an unexported `func (e *Engine) exec(ctx, sql string) error` in `engine.go`, list `engine.go` in the writer step's files, and use it for both the `COPY` and any `PRAGMA` the writer needs. + +**Resolved as suggested.** `engine.go` joins the writer step's file list. + +### Test author + +#### RESOLVED: The reindex test as described cannot fail for the right reason + +The plan says reindex "needs no code" because `reindexIdentity` builds a fresh manifest, and gives it a test rather than a change. That test is a legitimate regression guard against a future refactor that starts carrying the manifest forward, and it should stay. But it is the only test the plan names for the reindex requirement, and the requirement has two halves: entries are dropped, and `dataset show` then reports the dataset as needing re-materializing. The second half is untested because, as the requirements-side finding records, it has no mechanism. A reader of the plan sees one requirement with one test and no gap. + +Suggested resolution: whichever way the requirements-side question is settled, name the test for it here. If the warning moves to a folder-based check, the test is "reindex, then `dataset show` names the leftover Parquet"; if the second half is dropped, say so in this step so the missing test is a decision rather than an omission. + +**Resolved.** The requirements-side question settled on the folder-based check, so this step keeps the existing regression guard (`reindexIdentity` drops the map by construction, and the test goes red if a refactor starts carrying it forward) and adds two more: after a reindex, `dataset show` names the unreferenced Parquet, and the Parquet files are still on disk afterward. The second is the one that would catch someone later "tidying up" by making reindex delete the folder, which would break the external readers this story exists for. Because the new warning needs no view knowledge, it goes in `driftWarnings` rather than through the caller-side decorator, so its test lives in `internal/dataset/summary_test.go` alongside the other warning codes. + +### Operator + +#### RESOLVED: `Open` now builds the full statement set twice per dataset + +`applyMaterialized` calls `MaterializableViews(vs.m)`, which constructs a second `viewSet` and calls `statements()` on it, while the caller has already called `statements()` to get the slice being annotated. Every view's SQL, including one `report_` view per report download and one `answers_` per membership entry, is built twice on every `Open`, `query`, `repl` and MCP `query` call, materialized or not. + +This is string building rather than I/O and it will not be visible at 20 runs. It is worth one line of thought at REPORT-110's scale, and it is trivially avoidable. + +Suggested resolution: give `MaterializableViews` an unexported sibling taking the already-built `[]viewStmt`, and let the exported form build the statements once and delegate. The writer and the tests keep the exported signature. + +**Resolved as suggested.** No behavior change, so no new test; the existing `MaterializableViews` test covers the exported form and `applyMaterialized`'s tests cover the sibling. + +### Checked and not a problem + +- **Materializing a degraded view does not launder its warning.** `reportUnionView` calls `vs.warnf` while *constructing* the statement, so the "report CSV is missing on disk" warning fires on every `Open` whether the view is later read from Parquet or not. Verified. +- **A corrupt store JSONL cannot be materialized into an empty Parquet.** `read_json` with an explicit `columns=` map does not validate content at `CREATE VIEW`; it fails at query time. Verified: the view registers with no warning and the query fails outright, so the `COPY` fails too rather than writing zero rows. The typed-empty degradation applies to files that are missing or unopenable, which is the case the fingerprint already covers. diff --git a/specs/REPORT-113-dataset-materialization/requirements.md b/specs/REPORT-113-dataset-materialization/requirements.md new file mode 100644 index 0000000..f8bdf2c --- /dev/null +++ b/specs/REPORT-113-dataset-materialization/requirements.md @@ -0,0 +1,300 @@ +# Dataset materialization (Parquet query surface) + +**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-113 +**Repo**: https://github.com/concord-consortium/cc-data-cli +**Implementation Spec**: [implementation.md](implementation.md) +**Status**: **In Development** + +## Overview + +Add `cc-data dataset materialize`, which writes each of a dataset's file-backed views to a ZSTD Parquet file 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. Because the output is a plain Parquet file at a predictable path, an external tool can open it directly without going through cc-data at all. + +## Project Owner Overview + +cc-data answers every query by re-reading the raw JSONL and CSV a fetch produced. That is right for a few thousand answers and wrong at the scale a CLUE corpus reaches, where a dataset is measured in gigabytes and every question pays the full parse again. It is also the last piece of storage machinery our study code still owns: the Dataflow study maintains a hand-written Parquet build, with its own column-type declarations and its own guard against a partial build replacing a complete one, purely because the tool does not offer one. + +This story takes over both jobs. A researcher runs one command and gets a fast, typed, columnar copy of their dataset that their existing analysis scripts can read by path, and the hand-written build script is deleted rather than reimplemented. + +## Background + +The query engine opens an ephemeral in-memory DuckDB per invocation, registers a view per store, per report CSV and per join dimension from the manifest, then locks a sandbox confining file access to the dataset folders (`internal/duck/engine.go:31`, `internal/duck/views.go:56`, `internal/duck/engine.go:116`). Store views read JSONL through `read_json` and report views read CSV through `read_csv`, both with explicit typed column maps, so every query re-parses everything it touches. + +**The consuming story is what sets this story's scope.** REPORT-120 exists to retire `sources/clue/recipes/log-events/build-parquet.sh` onto `cc-data dataset materialize`, and its acceptance criteria are about the *materialized query surface*: that the study's queries return the same rows against the materialized dataset as against the hand-built Parquet, and that id columns land as VARCHAR so a join needs no cast. + +The dataset REPORT-120 materializes contains **no stores at all**. REPORT-119 puts the log events in as a Student Actions report run downloaded with `get report`, which writes `report_.csv` (`internal/fetch/report.go:332`); REPORT-118 puts the cohort in as a Student ID Mapping run, also a report CSV; and REPORT-112's `logs` view is a union over log-type report CSVs with derived columns. The CLUE stores do not arrive until REPORT-110, which is 0.3.0. So materialization scoped to stores would run on REPORT-119's dataset and produce nothing, and REPORT-120 could not land. + +That also inverts the usual reading of this story. At 0.2.0 the data is 258,916 log rows, which is small, and the need is ownership of a typed surface rather than speed. At 0.3.0 REPORT-110 lands a 6.38M-entry, 15 GB `clue_history` store and names this story as its scale answer. Materializing views rather than artifacts serves both. + +**External readability is a first-class requirement, not a side effect.** `sources/researcher-access.md:609` records that what this story is depended on for is the Parquet landing on disk where an external DuckDB can read it, not only being queryable through the CLI: every analysis script in `cc-data-studies` is plain Python invoking `duckdb` as a subprocess with no dependency on cc-data, and that independence is what makes the scripts outlive the tool. `studies/clue-dataflow-behavior/lib.py:22-24` names three Parquet paths as its entire data source. + +## Requirements + +- `cc-data dataset materialize /` writes one ZSTD Parquet per materializable view to `/materialized/.parquet`. +- A view is materializable when it is one of the static views **and** declares the files it reads. Both halves are derived from the view set rather than listed by hand. The static half is what excludes the per-run views (`report_`, `answers_`), which declare files but multiply with the run count. Together they select `reports`, `report_prompts`, `logs`, `answers`, `history`, `run_membership`, `student_id_mapping` and `student_metadata`, and excludes the manifest-derived VALUES tables (`downloads`, `attachment_files`), where materializing is pointless, and the `read_text` attachment views (`attachment_states`, `attachment_content`), which would copy every attachment's bytes. +- The Parquet carries the view's own output, derived columns included, so `materialized/logs.parquet` holds REPORT-112's `parameters_json`, `extras_json`, `event_time` and `received_time`, not the raw CSV's columns. +- Each file is written to a temporary name and renamed into place, so a partial file is never mistaken for a complete one. +- Materialization refuses a view that declares an input the manifest names but that is not on disk, unless `--allow-partial` is passed, and names the missing files. This is the whole of the guard, and the condition it exists for: `sources/clue/recipes/clue-documents/build-parquet.sh:34` states the invariant as "every document that should have history must have a .jsonl right now", and it is what a glob matching fewer files once turned into 6.27M entries replaced by 110k. The manifest is cc-data's source list, so the same invariant reads as: every file a view declares must be on disk right now. When overridden, the run reports how many of the declared inputs it built from, matching the source script's "building from N of M documents". +- The refusal is per view, not per command, because the evidence is per view: this view declares a file that is not there. Clean views are materialized, refused views are named with the files they are missing, and the run exits non-zero so a `set -euo pipefail` recipe stops. That is the shape `sources/clue/recipes/clue-documents/download-history.ts:206` already uses for a multi-item operation: work every item, summarize per item, exit non-zero if any failed. A dataset with a subset of its views materialized is an ordinary state, since freshness is per view and it is the same state a newly downloaded run produces. +- A download marked `!Complete` is reported as a warning naming the runs, not a refusal, and `--allow-partial` does not gate it. Exactly one place in the tree writes `Complete: false` (`internal/fetch/paged.go:195`, the store fetch), because report, report_job and attachment downloads write to a `.tmp` and record their entry only after the rename succeeds. A paged fetch merges once at the end, so an incomplete store download leaves the store file untouched and the Parquet byte-identical to one built if the run had never been requested. There is no shortfall to refuse. `dataset show` already carries the same fact as `INCOMPLETE`. +- The view-to-download mapping the warning needs is stated rather than left to be inferred, because no single rule covers it. Report-backed views map to their downloads by file intersection. `answers` and `history` map by download `Type`, since the store is one merged file across every run and carries no per-run identity. `run_membership` maps by `Type` too: its filenames do encode `(type, run)`, but an incomplete run has no membership file at all, so a per-run mapping would never see the run the warning is about. +- The refusal names both remedies for a missing input, since the point of refusing is to surface them rather than to leave a flag to be typed forever: re-fetch the run, or run `dataset reindex` to drop the download entry so the file stops being expected. +- Materialization refuses outright, with no flag to override it, for a view that registered from its typed-empty fallback rather than its primary statement. That is total loss rather than a shortfall, always a broken dataset, and there is no legitimate reason to publish a knowingly empty Parquet. The engine already distinguishes the two cases; it is the branch that warns "degraded to empty". +- After copying a store-backed view, the run asserts the Parquet's row count against the manifest's recorded store count and fails the view if they disagree. Both hand-written build scripts this story retires end with exactly this check (`log-events/build-parquet.sh:97`, `clue-documents/build-content-parquet.sh:102`). The union views get no such check: the pseudo-header filtering makes the arithmetic inexact, and an approximate assertion is worse than none. +- `--force` re-materializes a view whose recorded inputs are unchanged, which is otherwise a no-op. It does not override the missing-input refusal; that is `--allow-partial`'s job alone. +- Freshness is one rule for every view: the manifest records each materialized view's input files with a fingerprint, and the view's Parquet is used only while every input still matches. Otherwise the view is registered from its raw artifacts, with no error and no user action required. +- An input that cannot be stat-ed records a reserved sentinel fingerprint, which no real fingerprint can collide with because a real one is always `-`. `Fresh` returns false whenever any current input fails to stat. This is the read side, and it is needed whichever way the write side is gated: a file can go missing after a successful materialization, and the Parquet must then fall back to the raw artifacts rather than be trusted. +- A Parquet the manifest names but that is absent on disk falls back to the raw artifacts, with a warning. The typed-empty fallback keeps the meaning it already has: nothing readable at all. +- Materialization does its copies outside the dataset's mutation locks, holding them only to read the manifest at the start and repoint it at the end, and discards a result whose inputs moved in between. A materialize run must not make a concurrent `get` fail as busy. +- `dataset show` reports a view whose Parquet is stale, naming the command that refreshes it, and reports the materialized bytes separately from the dataset's total size. `dataset list` reports the same separate figure. +- `dataset purge` and `dataset delete` remove `materialized/` along with everything else they remove, and `Purge` clears `m.Materialized` alongside the four maps it already resets. Committing a manifest that still names files the same call is about to delete is what its own commit-ordering comment forbids (`internal/dataset/dataset.go:268`). +- `dataset reindex` drops the manifest's materialization entries rather than carrying them forward. Carrying them would need no Parquet reading and so no import cycle, and a reindex of an unchanged dataset leaves the materializable views byte-identical, so the files stay valid. It is still wrong: reindex is the command for recovering from a manifest cc-data cannot trust, and it has narrow paths that change a view's output while every declared input still fingerprints the same, notably a zero `FetchedAt` re-stamped from the clock, which silently reorders the dimension views' dedupe. Trading that against a rebuild measured in seconds is the wrong trade in the one command whose purpose is recovery. +- `dataset reindex` never deletes `materialized/`, and neither does anything else except `dataset purge` and `dataset delete`. The Parquet is this story's deliverable to tools outside cc-data, which need no manifest entry to read it, so cc-data losing confidence in a file is not grounds for cc-data to remove it. The derived-subfolder contract is therefore "never adopted, never reported as orphans, removed wholesale by purge and delete", and reindex is deliberately not on that list, which is also the right answer for REPORT-89's `exports/`. +- `dataset show` names any Parquet in `materialized/` that no manifest entry references, which is the state a reindex leaves and also the state a materialize run leaves if it discards a view after copying it. The message states what cc-data can and cannot vouch for rather than telling the researcher to clear it: the manifest no longer records these files, queries have gone back to the raw artifacts, `dataset materialize` restores them, and they stay readable by external tools meanwhile. Deciding this needs no view knowledge, only the folder listing against `Materialized`, so unlike the staleness warning it belongs in `driftWarnings` and reaches every caller of the show contract for free. +- Materialization reads only raw artifacts, never an existing Parquet. Copying a view that was itself reading a stale Parquet would write that stale data back out with fresh fingerprints attached, which is the one way this feature could silently corrupt an answer. +- Each Parquet carries its own provenance in the file footer: the view it holds and when it was built. Nothing in cc-data reads it back, since reindex deliberately does not try to revalidate a Parquet; it is there so a Parquet found outside the dataset, in a backup or a copied folder, can say what it is. +- A materialized view survives `dataset rename`. The manifest records dataset-relative paths resolved against the dataset directory at query time, so the folder moves with the rename. Verified, and asserted by a test, because unrelocatable stored paths are exactly why the persistent DuckDB was rejected. +- A materialized view resolves correctly in a multi-dataset session, under its schema prefix, and a dataset with no Parquet in the same session is unaffected. Verified, and asserted by a test. +- The derived-subfolder rule is a named predicate rather than a special case for one folder name, so REPORT-89 can register `exports/` against it. `materialized` and `exports` must not collide, and a test holds that a file planted inside a registered derived folder stays invisible to reindex and to the orphan-file warning. +- `dataset materialize` is exposed as an MCP tool, documented in the shared guidance catalog, and listed in the pinned MCP tool inventory. +- Guidance says when to materialize, that `materialized/` is safe for the researcher to delete at any time, and that cc-data itself removes it only through `dataset purge` and `dataset delete`. The two sentences sit together: read alone, the first one reads like a warning that the tool might delete it, which is exactly what an external script hardcoding the path needs to know is untrue. The "when" goes in the shared core both surfaces render; the CLI spelling goes in the skill header, since the core may not name a command. +- The researcher guide documents the `materialized/.parquet` layout as a stable path external tools may read directly, since that is what the study scripts depend on. +- `materialize` has no view-selection flag. `materialized/` is a deterministic mirror of the materializable set, so what it holds is a function of what the dataset holds, which is what lets an external script treat a path as a contract. A flag would make the contents a function of which flags someone last ran, and an outside reader could not tell an absent file from an unselected one. +- The researcher guide states the two measured duplication costs, so the folder size is expected rather than discovered. `reports` unions the log CSVs that `logs` also reads, so both carry the same rows: on a 200,000-row log fixture, `reports.parquet` was 5.4 MB beside a 9.5 MB `logs.parquet`, 36% of a folder whose largest member that study never opens. At REPORT-110's scale the same overlap is roughly 7%, because `reports` unions report CSVs only and the 313 MB `history` Parquet dominates. +- The identical-results test iterates the materializable views derived from the view set, not a hand-written list, so a view added later cannot go uncompared. +- Always-on tests, on a small fixture: identical results and identical column types for every materialized view; staleness detected after a `get`; fallback when the Parquet is deleted; per-view refusal on an input missing from disk, with the other views still materialized and a non-zero exit, and the same run proceeding under `--allow-partial`; an incomplete download warning without blocking anything; the un-overridable refusal for a view that fell back to typed-empty; the store row-count assertion failing a view whose Parquet is short; `Fresh` returning false once a recorded input is deleted; `--force` rebuilding an unchanged view; reindex dropping the entries and `dataset show` then naming the unreferenced Parquet, with the files themselves left on disk; and a file planted in a registered derived folder staying invisible to reindex and the orphan warning. +- The timing assertion needs a 1M-row fixture, roughly 455MB of generated JSONL, so it is opt-in through an environment variable rather than run on every CI job. It asserts a column-pruned aggregate at 10x against a measured 54x, and separately that a high-cardinality aggregate is not slower. Its fixture generator must emit varied payloads. + +## Technical Notes + +### Verified: the DuckDB mechanics + +Checked against the bundled engine (`github.com/duckdb/duckdb-go/v2 v2.10504.0`, `go.mod:6`, reporting `v1.5.4`), since the sandbox's `enable_external_access = false` made it an open question whether any of this was reachable: + +- `COPY () TO '' (FORMAT parquet, COMPRESSION zstd)` succeeds. +- `read_parquet` over a file inside an allowed directory succeeds **after** `lock_configuration = true`, exactly as `read_csv` and `read_json` already do. +- `COPY ... (KV_METADATA {...})` writes custom key/value pairs into the Parquet footer and `parquet_kv_metadata()` reads them back, including under the locked sandbox. +- A store's typed column map round-trips exactly: JSONL copied to Parquet and read back yields identical column types and an empty `EXCEPT` in both directions. + +### Verified: a view's declared types survive the round trip + +Copying a view shaped like `logs` to Parquet and reading it back preserves `TIMESTAMP`, `JSON` and `VARCHAR[]` exactly. `json_extract_string` still works against the materialized `parameters_json`, and a `TRY_CAST` that produced NULL for invalid JSON is preserved as NULL, so no column needs re-declaring on read. + +### Verified: every view is identical with its source swapped to Parquet + +A dataset was built with two merged runs and a report CSV, its `answers` store copied to Parquet, and a second engine opened whose `answers` view reads `read_parquet` while every other statement is byte-for-byte as `viewSet.statements()` produces it. All views were compared on column names, types and full row content: **13 compared, 0 differing**, including the per-run views that join the swapped store to a membership file still read as JSONL. The comparison needs `ORDER BY ALL` to be deterministic; that does sort a `JSON` column under the bundled engine, so re-measure if the engine is upgraded. + +### Verified: the speedup is real but strongly query-shaped + +On a generated 6.38M-row history-shaped fixture (2.92 GB JSONL), materialized to 313 MB of ZSTD Parquet (10.7%) in 5.0s, under the sandbox's own caps (`memory_limit = 2GB`, `threads = 4`): + +| Query shape | JSONL | Parquet | Speedup | +| --- | --- | --- | --- | +| `count(*)` | 785ms | 2ms | 327x | +| `GROUP BY question_id` | 992ms | 11ms | 87x | +| `GROUP BY question_id` + `count(DISTINCT remote_endpoint)` | 1.272s | 238ms | 5.3x | +| per-learner row count | 1.11s | 262ms | 4.2x | +| `json_extract_string(state, ...)` over the blob column | 1.591s | 1.138s | 1.4x | +| `SELECT * ... LIMIT 100` | 5ms | 33ms | 0.15x | + +Column pruning is where the order of magnitude comes from. A query that reads the wide state blob is bounded by JSON parsing rather than scanning. An unfiltered `LIMIT` is slower materialized, because JSONL streams and stops while Parquet decompresses a row group. + +`run_membership` on its own, over 20 membership files totaling 741 MB, materializes to 3.7 MB (0.49%) and gains 36x on `count(*)`. + +### Measured: a view's derived columns are free until they are materialized + +`logs` appends `parameters_json`, `extras_json`, `event_time` and `received_time` beside the source columns they derive from. In a view that costs nothing, since they are computed per query. Materialized, they are disk: on a 200,000-row fixture the derived columns are 5.22 MB of an 11.69 MB `logs.parquet`, so 45% of the file is a second encoding of data already in it. This story does not act on it (see Not Yet Implemented). + +### The fingerprint is the one new mechanism + +Freshness cannot key on a version, because report CSVs do not have one: `report_.csv` is a fixed name overwritten on re-fetch. Stores and membership files do carry versions in their names, but a single rule covering all three is worth more than three rules. Size and mtime is what Go's own build cache uses; it cannot distinguish a rewrite that preserved both, which a real re-fetch does not do, and a content hash would cost a full read of every input. + +### Lifecycle: verified by running the real code paths + +- **Purge missed derived folders.** `Purge` left a planted derived folder and its files intact, because `deleteArtifacts` removed only `segments` and `attachments` by literal name. +- **`dataset show` size includes it.** `dirSize` counted every planted file. +- **Reindex and the orphan check already ignore it**, but by accident: `Reindex` skips all directories and the orphan scan is files-only. The named predicate turns the accident into a contract. + +### Constraints inherited from elsewhere + +- Everything stays inside the dataset folder, so the sandbox allowlist is unchanged. What changes is the shape a planted file can take, from JSONL or CSV to a Parquet container with its own compression codecs: same boundary, wider parser behind it. +- The drift guard requires every registered view and every MCP tool to appear in the guidance catalog, in both directions, and separately checks the researcher guide's view table. `TestCoreNamesNoCommand` forbids naming a `cc-data` command in the core, so the "when" and the CLI spelling live in different files. +- `internal/duck` imports `internal/dataset`, never the reverse, which is why reindex cannot read a Parquet footer. + +## Out of Scope + +- Materializing `attachment_states` and `attachment_content`. They `read_text` every attachment, so materializing them copies the attachment corpus a second time. +- Materializing per-run views (`answers_`, `report_`). They multiply with the run count and are cheap joins over views that are themselves materialized. +- Pre-parsing the state blob into a typed structure. That is what would move the JSON-decoding shape past 1.4x, and it is a data-model change rather than a storage-format change. +- Any change to the sandbox allowlist, the trust boundary, or the resource caps. +- REPORT-89's `exports/` subfolder. This story owns the mechanism; 89 registers against it. +- Automatic materialization on `get`, or any background refresh. + +## Not Yet Implemented + +- **Trimming the superseded source columns from `logs`.** The derived columns are 45% of `logs.parquet`, a second encoding of `parameters` and `time`. Materialization must carry the view's exact column set or a query stops returning the same answer from the Parquet as from the raw artifacts, and whether `logs` should carry both forms at all is REPORT-112's contract, which would change results for every consumer. Deferred so REPORT-120 can answer the question that decides it: whether the study ever reads the raw `parameters` and `time` columns, or only the derived ones. +- **Lifting `doc_key`, `doc_type`, `doc_uid` and `tile_id` out of the `parameters` JSON.** These are the study's hottest join keys and the retired `build-parquet.sh` lifted them into real columns. Deferred to the study's own first derived step in REPORT-120, because a CLUE-specific extraction in a view spanning every portal and app is the wrong side of the generic-transform line, and `parameters_json` being JSON-typed does not recover the cost (1.4x on a wide-blob query against 87x on a column-pruned one). +- **`driftWarnings` raises `MISSING_FILE` for stores and membership only, never for a download's report CSVs.** So `dataset show` stays silent about the condition that now makes `materialize` refuse. This is a pre-existing hole rather than one this story opened, and closing it changes a documented stable contract, so it belongs in its own ticket. Not yet filed. + +## Decisions + +### Should materialization be scoped to stores (as the ticket said) or to views? + +**Context**: The ticket specified writing each *store* to Parquet. REPORT-120 is the consuming story and its dataset contains no stores at all. + +**Options considered**: +- A) Stores, as specified: one Parquet per `answers`/`history` store file. +- B) Views: one Parquet per materializable view, carrying the view's own derived columns. + +**Decision**: B. REPORT-119 puts the log events in as a report CSV, REPORT-118's cohort is another report CSV, and REPORT-112's `logs` is a union over log-type CSVs; the CLUE stores do not arrive until REPORT-110 at 0.3.0. Store-scoped materialization would have run on REPORT-119's dataset and produced nothing, so REPORT-120 could not have deleted `build-parquet.sh`. Views also give external readers REPORT-112's derived columns rather than the raw CSV's, which is what the study actually consumes. + +--- + +### Should the dataset keep a persistent DuckDB file alongside the Parquet? + +**Context**: The ticket's title specified "Parquet + persistent DuckDB". + +**Options considered**: +- A) A `.duckdb` file whose views point at the Parquet, as specified. +- B) Parquet only, queried through the existing ephemeral in-memory engine. + +**Decision**: B. It has no counterpart in the pipeline it was modeled on: no `.duckdb` file, no `ATTACH`, no `duckdb.connect(path)` anywhere in `cc-data-studies`, where every invocation is `duckdb -c` against a fresh in-memory database and `lib.py:22-24` names three Parquet files as the whole data source. Measured, a native DuckDB table beat Parquet by about 1.4x for a second full copy on disk, and views stored inside a DuckDB file freeze the absolute Parquet path, so `dataset rename` breaks them with a misleading sandbox permission error. + +--- + +### Fixed filenames or version-stamped ones? + +**Context**: Stores are versioned files swapped atomically on merge, so a version-stamped Parquet would follow the existing convention. + +**Options considered**: +- A) `materialized/.v.parquet`, following the store convention. +- B) `materialized/.parquet`, fixed. + +**Decision**: B. External scripts hardcode paths (`lib.py:22-24`), and a name that changes on every fetch breaks them. The cost, verified: replacing a fixed-name Parquet under a live session changes that session's answers mid-query rather than pinning a snapshot, since DuckDB re-opens by path. That is acceptable for a user-initiated command, and better than what JSONL does today, where `cleanupOldVersions` deletes the file a live session is reading. On Windows a rename over an open Parquet can fail, which `fsutil.RenameAtomic` retries for 100ms. + +--- + +### `materialized/` or `.materialized/`? + +**Context**: The folder holds derived data, which would normally argue for hiding it. + +**Options considered**: +- A) Hidden, `.materialized/`. +- B) Visible, `materialized/`. + +**Decision**: B. Once an external script is meant to hardcode the path, a hidden directory is the wrong choice, and it sits alongside the existing non-hidden `segments/` and `attachments/`. + +--- + +### Should `materialize` take a view-selection flag? + +**Context**: `reports` unions the same log CSVs `logs` reads, so a log-heavy dataset writes those rows twice; `--view` would let REPORT-120 materialize only what it reads. + +**Options considered**: +- A) `--view `, repeatable, defaulting to the full set. +- B) No flag; the folder is always the full materializable set. + +**Decision**: B, and document the cost. The magnitude did not survive measurement: `reports.parquet` was 5.4 MB beside a 9.5 MB `logs.parquet` on a 200,000-row fixture, and roughly 7% overlap at REPORT-110's scale where the 313 MB `history` Parquet dominates. Against single-digit percentages, a flag would cost the property the external consumer depends on: without it `materialized/` is a deterministic mirror of the dataset, and with it the contents become a function of which flags someone last ran, so an outside reader cannot distinguish an absent file from an unselected one. It is also the kind of flag that breeds: `--view` invites `--exclude`, then its interaction with `--force`. + +--- + +### Should the union fact views denormalize `slug` and `hide_names`? + +**Context**: REPORT-112 deferred this onto this story, because materialization writes a column set into Parquet and an external reader has no `downloads` table to join against. `username` carries five meanings across the log slugs and the `hide_names` split. + +**Options considered**: +- A) Denormalize `slug` and `hide_names` onto `reports`, `logs` and REPORT-111's CLUE views. +- B) Materialize `downloads` so the dimension travels with the fact tables. +- C) Neither; the join stays a CLI and MCP concern. + +**Decision**: C. The hazard belongs to CLI and MCP consumers, who have `downloads` and are taught the join by the catalog entries. The bare-Parquet reader this story exists for does not hit it: across `studies/clue-dataflow-behavior/`, `username`, `student_name`, `student_id`, `primary_user_id`, `learner_id`, `application`, `slug` and `hide_names` appear zero times in production queries, and what the study selects is `doc_key`, `offering_id`, `class_id`, `session`, `event`, `event_time` and `extras`. B was rejected with it: `downloads` scans no files, so it satisfies neither half of the materializable predicate, and admitting it would need either a false `manifest.json` input or a hand-listed exception. If a future consumer needs provenance beside the fact tables, write `materialized/downloads.parquet` unconditionally on every run, outside the `Materialized` map. + +--- + +### What exactly does `--allow-partial` override? + +**Context**: The guard exists because a glob matching fewer files once replaced 6.27M history entries with 110k. An early draft keyed it on `Download.Complete`. + +**Options considered**: +- A) An incomplete download (`Complete: false`). +- B) A declared input missing from disk. +- C) Both. + +**Decision**: B, with the incomplete download demoted to an ungated warning. Grepping every assignment of `Complete` finds exactly one that writes `false`: the store fetch, which 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 a file-based mapping the A guard could never fire at all, since store downloads carry no `Files`. So the draft protected nothing in either direction, and the incident it cited is a missing-files incident. The sibling repo has no completion flag of any kind, because tmp-and-rename makes presence on disk the signal. + +--- + +### Should `--force` and `--allow-partial` be one flag? + +**Context**: Both are overrides on the same command. + +**Options considered**: +- A) One flag covering both. +- B) Two flags. + +**Decision**: B. Collapsing them would make one flag mean "I accept incomplete data" in one situation and "spend the time again" in another, which is the shape of flag that gets passed reflexively until it stops protecting anything. + +--- + +### Is `materialized_bytes` a new field or a redefinition of `size_bytes`? + +**Context**: `ShowJSON` is documented as the stable `dataset show --json` contract and `dataset list` publishes the same figure. + +**Options considered**: +- A) Redefine `size_bytes` to exclude derived data. +- B) Add `materialized_bytes` beside it. + +**Decision**: B. Redefining would break both published surfaces. The same field is added in both places or neither. + +--- + +### Is materialize exposed over MCP? + +**Context**: An MCP client may not be able to run a terminal command. + +**Options considered**: +- A) CLI only, with the guidance telling an agent to send the user to a terminal. +- B) A `dataset_materialize` MCP tool alongside the CLI command. + +**Decision**: B. The "when to materialize" prose lands in the shared core both surfaces render, so an MCP client is told when to materialize whether or not it can; without the tool its only recourse is to send the user to a terminal, which is the shape the MCP header exists to avoid. The tool is annotated neither read-only nor destructive: it writes, but only derived data that is regenerable and documented as safe to delete. + +--- + +### What should a missing Parquet do at query time? + +**Context**: The first draft said a missing materialized copy degrades as a missing store file does. + +**Options considered**: +- A) Degrade to the typed-empty view, as a missing store does. +- B) Fall back to the raw artifacts, with a warning. + +**Decision**: B. A silently empty result over a completely intact dataset is the worst failure this feature could have. Verified that both `read_json` and `read_parquet` over a missing file fail at view *creation* rather than at query time, which is what makes primary-then-fallback registration work, so the engine can try the Parquet statement and fall through. Absent, corrupt and truncated Parquets all fail the same way, so one warning covers every shape. + +--- + +### How long should a materialize run hold the dataset's mutation locks? + +**Context**: `lockMutation` takes both locks non-blocking and returns `ErrBusy` rather than waiting. Materializing 6.38M records took 5s locally and would be far longer at 15 GB. + +**Options considered**: +- A) Hold them for the whole run, as every other mutating command does. +- B) Hold them only to read the manifest and to repoint it, re-checking before committing. + +**Decision**: B, following `mergeUnderLock`'s existing discipline: do the expensive work against what was read, re-check before committing, discard if it moved. Under A a concurrent `get` would fail outright for the whole copy window. + +--- + +### What happens to a view whose inputs move while its copy is running? + +**Context**: Because the copies run outside the mutation locks, a `get` can merge mid-run. + +**Options considered**: +- A) Record it anyway; the Parquet is internally consistent. +- B) Discard it and report it. +- C) Retry the copy against the new inputs. + +**Decision**: B. The Parquet describes a state that is already gone, and recording it would attach fresh fingerprints to stale data. C was rejected during implementation: a retry inside a command already doing gigabyte copies turns a predictable run into an unpredictable one, and a fetch loop could keep invalidating it. The input fingerprints are taken just before each copy starts and compared again at the repoint, since rebuilding them at the repoint would compare a value against itself. + +--- + +### Does a missing input abort the run, skip the view, or record something? + +**Context**: A union view keeps declaring a CSV that has gone missing; this is a supported, warned-about state, not corruption. `Fingerprint` returning an error left three implementations that diverge sharply. + +**Options considered**: +- A) Abort the run, so one missing CSV makes the whole dataset unmaterializable. +- B) Skip the view silently. +- C) Refuse the view with a named override, and record a reserved sentinel on the read side. + +**Decision**: C. Read back from the code being retired: `clue-documents/build-parquet.sh` offers `--allow-partial` for precisely the source-list-versus-disk mismatch, `log-events/build-parquet.sh` refuses on any missing input, and neither lets a missing input yield a short Parquet. The sentinel is needed on the read side regardless, because a file can go missing after a successful materialization. Two further facts, both verified: a missing store file degrades at `CREATE VIEW` and `COPY` of the degraded view succeeds, writing a 404-byte zero-row Parquet, which is why total loss needs a refusal with no override; and the manifest's store count is an exact invariant for a store view's rows, which makes the row-count assertion both build scripts end with directly portable. + +--- + +### Is the refusal per view or per command? + +**Context**: A dataset can have one broken view and seven healthy ones. + +**Options considered**: +- A) Per command: refuse the run if any view is short. +- B) Per view: materialize the clean ones, name the refused ones, exit non-zero. + +**Decision**: B, following `download-history.ts:206`, which works every item, summarizes per item, and exits non-zero if any failed. A dataset with a subset of its views materialized is an ordinary state, since freshness is per view and it is the same state a newly downloaded run produces. Extended during implementation to cover a copy that fails outright (a full disk, or a concurrent `get` removing a store version mid-copy): that is per-view evidence too, so it refuses the view rather than aborting the run and discarding every completed copy. Only a cancelled context stops the run. + +--- + +### Should reindex carry the materialization entries forward? + +**Context**: Carrying them needs no Parquet reading and therefore no import cycle, so the plan's stated reason for dropping them was not the real one. Measured: a reindex of an unchanged dataset left 14 of 15 view statements byte-identical and every artifact fingerprint unchanged. + +**Options considered**: +- A) Carry the entries forward. +- B) Drop the entries and delete `materialized/`. +- C) Drop the entries, keep the files, and warn from the folder listing. + +**Decision**: C. A was rejected because reindex has narrow paths that change a view's output while every input still fingerprints the same, notably a zero `FetchedAt` re-stamped from the clock silently reordering the dimension dedupe; trading that against a rebuild measured in seconds is the wrong trade in the one command whose purpose is recovery. B is what the "make the bad state impossible" rule points at, and was rejected for a reason that outranks it here: `materialized/.parquet` is this story's deliverable to tools outside cc-data, so deleting a still-correct file over cc-data's internal bookkeeping would break a running study script. + +--- + +### Where does each drift warning live? + +**Context**: Deciding staleness needs the view's current input list, which only `internal/duck` knows, and `internal/dataset` cannot import it. `BuildShowJSON` has two non-test callers, the CLI and the MCP `dataset_show` tool. + +**Options considered**: +- A) Both warnings in `driftWarnings`, duplicating a view-to-files mapping inside `dataset`. +- B) Both appended by each caller. +- C) The unreferenced-Parquet warning in `driftWarnings`, the staleness warning through one exported decorator both callers use. + +**Decision**: C. The unreferenced warning needs only the folder listing against `Materialized`, so it goes straight into `driftWarnings`, lands inside the existing sort, and reaches both surfaces for free. Staleness goes through `duck.AnnotateShowJSON`, which appends and re-sorts, so the two surfaces cannot publish different warning sets and there is exactly one implementation of freshness shared by the reader, the writer and the warning. A would have been two answers to "is this Parquet usable" that can drift apart; B would have let the surface that can act on the warning be the one that never sees it. + +--- + +### How is the materializable set derived? + +**Context**: The first draft derived it "from the statement set over an empty manifest filtered to those declaring files", copying `StaticViewNames`. Run against an empty manifest, 0 of 12 views declare files, so the function returned nothing and materialization would have silently done nothing on every dataset. Over a *populated* manifest the bare predicate wrongly included `report_`. + +**Options considered**: +- A) A hand-maintained list of view names. +- B) Declares-files over an empty manifest. +- C) Declares-files over the real manifest, intersected with `StaticViewNames()`. + +**Decision**: C. "Declares files" is a property of a populated dataset, unlike "is a static view", so `MaterializableViews` takes the manifest. Both halves stay code-derived, so a view added later joins or stays out of the set by its own construction rather than by a list someone remembers to update. Because the statements carry a schema prefix in a multi-dataset session while the set is keyed on bare names, both the predicate and the reader go through one `bareViewName` helper; an earlier draft trimmed the prefix in only one of the two, which would have disabled the feature entirely whenever more than one dataset was registered. + +--- + +### How does materialization get an engine that ignores existing Parquet? + +**Context**: Copying a view that was itself reading a stale Parquet would launder stale data into a fresh-looking entry. `Open` has three non-test callers with no interest in the distinction. + +**Options considered**: +- A) A flag on `Open`, updating all callers. +- B) A sibling `OpenRaw` over a shared unexported constructor. + +**Decision**: B, leaving all three callers untouched. `OpenRaw` also returns the set of views that registered from their typed-empty fallback, which is what the un-overridable refusal keys on. Separately, the writer needs a statement-executing path that `Engine` does not expose: running `COPY ... TO` through `Query` and dropping the result leaks the rows and deadlocks `Close`, reproduced with a test that hung for the full ten-minute timeout while five Parquet files sat correctly written on disk. Hence an unexported `Engine.exec`. + +--- + +### When are the temp files renamed into place? + +**Context**: The first draft renamed at the end of each copy and then spoke of removing "its temp file" at the repoint, by which point no temp file exists. + +**Options considered**: +- A) Rename as each copy finishes; delete the final-named file if the view is later discarded. +- B) Hold the temp name until after the re-check, then rename the survivors and delete the rest. + +**Decision**: B. Under A a discarded view leaves a complete, unreferenced Parquet inside a folder that reindex and the orphan check are contractually blind to, counted in `materialized_bytes` forever, and it inverts the commit discipline the rest of the package follows, where the manifest write is the commit point. + +--- + +### How is a temp file left by an interrupted run cleaned up? + +**Context**: Nothing in the tree installs a signal handler, so Ctrl-C kills a run before its deferred cleanup can run. 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, and `materialized_bytes` counts it forever. + +**Options considered**: +- A) Report it in the unreferenced-Parquet warning and let the researcher delete it. +- B) Sweep files older than a cutoff. +- C) Hold a dedicated materialize guard for the whole run, then sweep unconditionally. + +**Decision**: C. A records a broken state and asks the user to clear it, which is the shape to avoid when the state can be made impossible. B is a heuristic that a slow copy over 15 GB can outlast. A naive sweep without the guard was measured to be actively harmful: deleting a concurrent run's in-flight copy makes that run die on the rename with a bare "no such file or directory", writing nothing. The guard removes the possibility, because a live run still holds it and the kernel drops an `flock` when its holder dies however abruptly, both verified. It also settles something it was not added for: two overlapping runs used to copy every view twice and race at the repoint. `get` never takes this guard, so the rule that a materialize must not make a concurrent fetch fail as busy is unchanged. + +--- + +### How does a run report what happened to each view? + +**Context**: 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 that matters: a discarded view is not materialized and running again picks it up. + +**Options considered**: +- A) Reword the CLI label to "skipped", which is true of both outcomes. +- B) Add a fourth list, `Discarded`. +- C) One outcome per view, carrying a status and a reason. + +**Decision**: C. A leaves the reader no reason to re-run and does not touch the MCP surface at all. B keeps a shape where nothing stops a view landing in two lists or none, and where the MCP tool assembles per-status keys by hand, so a status added later would be silently omitted: that is the same two-lists-that-must-agree failure this story already rejected for the materializable predicate and for the show warnings. Under C a view structurally has exactly one outcome, the MCP tool returns the list as it stands, and callers keep `Written()`, `Fresh()`, `Discarded()` and `Refused()` accessors so the CLI and the exit-code rule read as before. + +--- + +### What exit class does a refusal use? + +**Context**: The spec said only "non-zero". The help documents exit 5 as a *server* contract error. + +**Options considered**: +- A) Exit 5, the contract class. +- B) Exit 1, the "internal/other" class. + +**Decision**: B, as `MATERIALIZE_REFUSED`. A refusal is entirely local, and using the server-contract class would mislead anyone scripting on the code. + +--- + +### Does a second materialize run on the same dataset wait or fail? + +**Context**: The materialize guard is non-blocking, like every other lock in the package. + +**Options considered**: +- A) Wait for the first run. +- B) Fail as busy. + +**Decision**: B, matching every other mutating command, which returns `ErrBusy` rather than queuing. There is no point running two at once; they would do identical work. The generic busy message is kept rather than adding a materialize-specific sentinel, since the remedy is the same either way and `lockMutation` already collapses two different contended locks into one `ErrBusy`. + +--- + +### Checked and recorded rather than actioned + +- **The sandbox's 2GB memory cap does not bind.** `COPY` streams rather than buffering: the 6.38M-row, 2.92 GB fixture materialized in 5.0s under exactly that cap. +- **Materializing a degraded view does not launder its warning.** `reportUnionView` warns while *constructing* the statement, so the "report CSV is missing on disk" warning fires on every `Open` whether the view is later read from Parquet or not. +- **A corrupt store JSONL cannot be materialized into an empty Parquet.** `read_json` with an explicit `columns=` map does not validate content at `CREATE VIEW`; it fails at query time, so the `COPY` fails too rather than writing zero rows. +- **The trust boundary is unchanged, but the parsing surface is not.** A dataset folder already inherits the trust of whoever can write to it. What changes is that a hostile file there can now be a Parquet container rather than JSONL or CSV. +- **Membership files are not versioned in lockstep with the store.** `mergeUnderLock` repoints only the runs in the merge, so `membershipUnionMulti` reads every other run's membership at its own recorded version. Moot under the view-scoped design, and part of why a per-input fingerprint beats a per-artifact version rule. diff --git a/specs/REPORT-113-dataset-materialization/implementation.md b/specs/REPORT-113-dataset-materialization/implementation.md deleted file mode 100644 index eeeb333..0000000 --- a/specs/REPORT-113-dataset-materialization/implementation.md +++ /dev/null @@ -1,552 +0,0 @@ -# Implementation Plan: Dataset materialization (Parquet query surface) - -**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-113 -**Requirements Spec**: [requirements.md](requirements.md) -**Status**: **In Development** - -## Shape of the change - -Three facts, all verified against the running engine, set the shape and remove most of the design freedom: - -- **`COPY TO` works under the fully locked sandbox** and is refused for a path outside the allowlist. So materialization reuses the ordinary `duck.Open` path rather than needing a privileged writer, and the sandbox keeps constraining where it can write. Nothing about the trust boundary changes. -- **`COPY (SELECT * FROM "")` preserves the view's exact column set and types**, JSON included, for both store-backed and CSV-union views. So materialization needs no access to how a view's SQL was built. It creates the views the normal way and copies them out by name, which means no refactor of `views.go`'s statement construction. -- **`internal/duck` imports `internal/dataset`, never the reverse.** Materialization therefore lives in `duck`, the manifest types live in `dataset`, and `reindex` cannot read a Parquet. - -The engine's registration loop gains one level. Today it tries `primary`, then `fallback`. It becomes: try the materialized statement if there is one, then `primary`, then `fallback`. Every existing view keeps its exact behavior when nothing is materialized, which is what keeps this change small. - ---- - -## Register derived subfolders and make purge remove them - -**Summary**: Adds the named predicate the requirements ask for and fixes the verified purge gap. Independent of everything else, so it lands first and alone. - -**Files affected**: -- `internal/dataset/dataset.go`: the predicate, the folder name, `deleteArtifacts` -- `internal/dataset/dataset_test.go`: purge coverage -- `internal/dataset/reindex_test.go`: the invisibility contract - -**Estimated diff size**: ~130 lines - -```go -// MaterializedDir is the derived subfolder holding a dataset's Parquet query surface. -const MaterializedDir = "materialized" - -// derivedSubdirs are subfolders holding data cc-data generated from the dataset's own -// artifacts. They are never adopted as artifacts, never reported as orphans, and are -// removed wholesale by purge and delete. REPORT-89 registers its exports folder here. -var derivedSubdirs = map[string]bool{MaterializedDir: true} - -// IsDerivedSubdir reports whether a directory name inside a dataset holds derived data. -func IsDerivedSubdir(name string) bool { return derivedSubdirs[name] } -``` - -In `deleteArtifacts`, the directory arm currently removes `segments` and `attachments` by literal name; it gains `|| IsDerivedSubdir(name)`. `Purge`'s other half, clearing `m.Materialized`, waits for the step that adds that field; putting it here would not compile. - -Reindex is deliberately **not** added to the predicate's contract. It drops the manifest entries but leaves the files, because `materialized/.parquet` is this story's deliverable to tools outside cc-data and stays correct for them; cc-data losing confidence in a file is not grounds for cc-data to remove it. The same reasoning covers REPORT-89's `exports/`, which is even more clearly a product than a cache. - -Tests are the point of this step, because the invisibility properties currently hold by accident: - -- Purge removes a populated `materialized/`. This fails before the change (verified: all three planted files survived a purge). -- Delete removes it too. This needs **no code**: `Delete` renames the whole dataset directory to a tombstone and `RemoveAll`s it (`internal/dataset/dataset.go:179`), so a subfolder goes with it. The test exists because the requirement names delete, and because it can fail: it goes red the moment someone rewrites `Delete` to remove known children selectively, which is how `Purge` came to have this gap in the first place. -- A file planted in `materialized/` that *would* be adopted at the top level (`answers.v99.jsonl`, `report_4242.csv`, `members_answers_5.v9.jsonl`) is not adopted by `Reindex` and raises no `ORPHAN_FILE` warning. This passes before the change; it is written to hold the contract against a future change that makes either walk subdirectories. - ---- - -## Record materialization in the manifest - -**Summary**: The manifest types and the fingerprint, with no producer or consumer yet. Splitting it out keeps the engine and writer steps reviewable on their own. - -**Files affected**: -- `internal/dataset/manifest.go`: the `Materialized` entry and its map -- `internal/dataset/dataset.go`: `Purge` clearing `m.Materialized` -- `internal/dataset/fingerprint.go` (new): fingerprint computation -- `internal/dataset/fingerprint_test.go` (new), `internal/dataset/dataset_test.go` - -**Estimated diff size**: ~160 lines - -```go -// Materialized records one view's Parquet and the inputs it was built from. Freshness is -// per input rather than per store version because report CSVs have no version: report_.csv -// is a fixed name overwritten on re-fetch. -type Materialized struct { - File string `json:"file"` // dataset-relative, e.g. materialized/logs.parquet - Inputs map[string]string `json:"inputs"` // dataset-relative input path -> fingerprint - BuiltAt time.Time `json:"built_at"` -} -``` - -`Manifest` gains `Materialized map[string]Materialized \`json:"materialized,omitempty"\`` keyed by view name, and `ensureMaps` initializes it. `Purge` clears it alongside the four maps it already resets, in the same step because that is where the field appears: committing a manifest that still names files the same call is about to delete is what `Purge`'s own commit-ordering comment forbids. No manifest version bump: `decodeManifest` already tolerates an absent field, and an older binary reading a newer manifest ignores the key rather than failing. - -```go -// Fingerprint identifies a file's content cheaply, as size and modification time. This is what -// Go's own build cache uses. It cannot distinguish a rewrite that preserved both, which a real -// re-fetch does not do; a content hash would be exact and would cost a full read of every input, -// which at 15 GB defeats the purpose of materializing. -// FingerprintAbsent is what a declared input that is not on disk records. It cannot collide -// with a real fingerprint, which is always -. A union view keeps declaring a CSV -// that has gone missing (internal/duck/views.go:218 appends dl.Files whether or not the file is -// there), so this is a normal state rather than an error, and Fresh must be able to tell "absent -// then and absent now" from "absent then, present now". -const FingerprintAbsent = "absent" - -func Fingerprint(path string) string { - fi, err := os.Stat(path) - if err != nil { - return FingerprintAbsent - } - return fmt.Sprintf("%d-%d", fi.Size(), fi.ModTime().UnixNano()) -} - -// FingerprintInputs fingerprints a view's declared files, deduplicated, in the form -// Materialized.Inputs records and Fresh compares against. The writer records through it rather -// than building the map itself, so "how an input is recorded" has one definition. -func FingerprintInputs(dir string, files []string) map[string]string - -// Fresh reports whether the Parquet may be read: the view's current inputs are exactly the -// recorded ones, and each still fingerprints the same. The caller supplies the current list -// because only the engine knows which files a view reads. -// -// Comparing the set, not just the recorded entries, is load-bearing. A run downloaded since the -// materialization adds a file to a union view, and a Parquet built without it is stale even -// though every recorded input still matches perfectly. -func (m Materialized) Fresh(dir string, current []string) bool -``` - -Tests: a fingerprint changes when content is rewritten and when only the mtime moves; `Fresh` is true unchanged, false after a rewrite, false when an input is deleted, false when an input is added, and false when the view stops declaring one of the recorded inputs. That last one is what the set comparison is for, and it is the only assertion that fails if the comparison is dropped: an added input is already caught by the lookup miss. And the absent round trip, verified ahead of implementation against the real view set: materialize a `reports` view one of whose CSVs is missing, and `Fresh` is true immediately afterwards and false the moment the CSV is restored from a backup. - ---- - -## Teach the engine to prefer a fresh materialized view - -**Summary**: The read path. After this step a hand-placed Parquet is used, which is what makes the step testable before a writer exists. - -**Files affected**: -- `internal/duck/views.go`: `viewStmt.materialized`, the resolver, `MaterializableViews(m)` -- `internal/duck/engine.go`: three-level registration -- `internal/duck/materialized_test.go` (new) - -**Estimated diff size**: ~210 lines - -`viewStmt` gains one field, and `viewSet` gains the resolved map: - -```go -type viewStmt struct { - name string - materialized string // optional read_parquet form, tried before primary - primary string - fallback string - files []string -} -``` - -`statements()` is unchanged. A single post-pass applies materialization, which keeps every builder untouched: - -```go -// applyMaterialized points a view at its Parquet when the manifest records one that is fresh. -// Membership of the materializable set is decided by materializableFrom over these same -// statements, so the writer and the reader can never disagree about which views have a Parquet. -// -// There is deliberately no presence check. A Parquet that is absent, corrupt or truncated all -// fail at CREATE VIEW rather than at query time, so the registration loop's warning covers every -// shape a bad file takes, and a stat per view per Open would buy only a differently worded -// message. A stale entry is silent by contract; an unreadable one is not. -func (vs viewSet) applyMaterialized(stmts []viewStmt) []viewStmt { - if len(vs.m.Materialized) == 0 { - return stmts // the common case, and it skips building the eligible set - } - eligible := map[string]bool{} - for _, n := range materializableFrom(stmts, vs.prefix) { - eligible[n] = true - } - for i, st := range stmts { - bare := bareViewName(st.name, vs.prefix) - if !eligible[bare] { - continue - } - mat, ok := vs.m.Materialized[bare] - if !ok || !mat.Fresh(vs.canonDir, st.files) { - continue - } - stmts[i].materialized = fmt.Sprintf("CREATE VIEW %s AS SELECT * FROM read_parquet(%s)", - st.name, vs.file(mat.File)) - } - return stmts -} -``` - -`materializableFrom` takes the statements rather than the manifest because `applyMaterialized` -already holds them; the exported `MaterializableViews(m)` builds them once and delegates, so -`Open` never builds the view set twice. It takes the prefix with them, because a statement built -for a named dataset carries one (`"alpha"."answers"`) while the set is keyed on the bare name -`StaticViewNames` produces. Both callers go through `bareViewName` so there is one answer to what -a statement's view is called, rather than two that can disagree about the prefix. - -Registration in `Open` becomes an ordered attempt. The distinct warning matters: a materialized copy that fails is a different event from a view degrading to empty, and conflating them would hide a corrupt Parquet behind a message about missing artifacts. - -```go -for _, stmt := range vs.statements() { - if stmt.materialized != "" { - if _, err := conn.ExecContext(ctx, stmt.materialized); err == nil { - continue - } else { - fmt.Fprintf(warnOut, "warning: materialized %s is unreadable (%v); reading the raw artifacts instead\n", stmt.name, err) - } - } - // existing primary-then-fallback, unchanged -} -``` - -```go -// MaterializableViews returns the views of this dataset that can be materialized: those that -// scan files, minus the per-run views. Both halves are code-derived, so a view added later -// joins or stays out of the set by its own construction rather than by a list someone updates. -// -// It takes the manifest because "declares files" is a property of a populated dataset: over an -// empty manifest every builder returns its stand-in and no view declares anything. -func MaterializableViews(m *dataset.Manifest) []string { - vs := viewSet{m: m} - return materializableFrom(vs.statements(), vs.prefix) -} - -// materializableFrom takes statements the caller has already built, so Open does not build the -// full view set twice: applyMaterialized has the slice in hand and passes it straight through. -// Those statements carry the schema prefix of whichever dataset they were built for, so the -// caller passes it too and the set stays keyed on bare names. -func materializableFrom(stmts []viewStmt, prefix string) []string { - static := map[string]bool{} - for _, n := range StaticViewNames() { - static[n] = true - } - var out []string - for _, st := range stmts { - bare := bareViewName(st.name, prefix) - if len(st.files) > 0 && static[bare] { - out = append(out, bare) - } - } - return out -} - -// bareViewName is a statement's view name with its schema prefix and quoting removed. That is -// the form StaticViewNames produces and the form the manifest's Materialized map is keyed on, -// so every lookup against either has to go through it. -func bareViewName(name, prefix string) string { - return strings.Trim(strings.TrimPrefix(name, prefix), `"`) -} -``` - -The writer and the tests both consume the exported form rather than either hand-listing views. Verified against a populated fixture: the set comes out as `answers`, `logs`, `report_prompts`, `reports`, `run_membership`, `student_id_mapping`, `student_metadata`, plus `history` once that store exists, and the four VALUES- and `read_text`-backed views are excluded by declaring no files, exactly as the requirements state. - -The identical-results test's fixture includes a log run, so `logs` is in the set and its `JSON`, `TIMESTAMP` and `VARCHAR[]` columns are exercised: those are computed per query from the CSV, and they are the reason this story materializes views rather than artifacts, so the comparison names them rather than leaving them to the whole-map check. - -Tests, all driven by materializing a Parquet that carries one sentinel row the raw artifacts do not, so "the Parquet is the source" is proved rather than assumed: - -- The sentinel is returned when the entry is fresh, and not returned when it is stale, when the Parquet is deleted, or when it is corrupt. Going stale is silent; a deleted, corrupt or truncated Parquet must warn and read the raw artifacts, never degrade to empty. All three unreadable shapes fail at `CREATE VIEW`, verified ahead of implementation, which is what lets one warning cover them. -- **The sentinel survives `dataset rename`.** Verified ahead of implementation: materialize, rename the dataset, re-open, sentinel still returned. This is the property whose absence disqualified the persistent DuckDB, so it is pinned rather than assumed. -- **Multi-dataset resolution.** Two datasets opened together, one materialized: `"alpha"."answers"` returns the sentinel and `"beta"."answers"` does not. Verified ahead of implementation. This is the test that covers `bareViewName`, and it has to be a *named*-dataset test to cover it at all: the prefix is empty in every single-dataset session, so a mistake in trimming it off is invisible to every other test in this plan, and it costs the whole feature rather than one view, since `materializableFrom` returning an empty set means no view is ever pointed at its Parquet. -- `MaterializableViews(m)` over a populated fixture matches the expected names, so a view added later that declares files is noticed here. - ---- - -## Add the materialize writer - -**Summary**: The producer. Opens the dataset with materialization suppressed, copies each materializable view out, and repoints the manifest. - -**Files affected**: -- `internal/duck/materialize.go` (new) -- `internal/duck/engine.go`: `OpenRaw`, the unexported `exec`, and the degraded-view set `OpenRaw` reports -- `internal/store/lock.go`: `MaterializeLockFile` and `MaterializeLockFor` -- `internal/dataset/dataset.go`: `lockMutation` exported as `LockMutation`, plus `LockMaterialize` -- `internal/duck/materialize_test.go` (new) - -**Estimated diff size**: ~380 lines - -```go -type MaterializeOptions struct { - Force bool - AllowPartial bool - Progress func(view string, i, n int) -} - -// One outcome per view, in view order, rather than a list per status. A view -// cannot then land in two of them or in none, and the MCP tool returns this -// slice as it stands instead of a per-status projection that someone has to -// remember to extend when a status is added. -type MaterializeResult struct { - Views []ViewOutcome `json:"views"` -} - -type ViewOutcome struct { - View string `json:"view"` - Status MaterializeStatus `json:"status"` // written | fresh | discarded | refused - Reason string `json:"reason,omitempty"` -} - -func Materialize(ctx context.Context, d *dataset.Dataset, opts MaterializeOptions, warnOut io.Writer) (MaterializeResult, error) -``` - -The order is load-bearing and mirrors `mergeUnderLock`'s discipline, for the reason the requirements give: the copy must not hold the mutation locks, or a concurrent `get` fails as busy for its whole duration. - -0. Take the dataset's materialize guard (`.materialize.lock`, a third `flock` beside the two existing ones, reusing `store.DatasetLock`), non-blocking, and hold it until the run returns. Then sweep every `*.parquet.tmp-*` in `materialized/`. The two halves are one mechanism: the guard is what makes the sweep provable rather than a heuristic, since a live run would still hold it. `get` does not take this guard, so nothing about the mutation-lock rule changes. The temp-name infix has a single definition that the writer and the sweep share. - -1. Read the manifest under the per-dataset lock, then release. Warn here, without refusing, for every `!Complete` download feeding a materializable view, naming the runs: only the store fetch can set that flag and it leaves the materialized bytes unchanged, so there is nothing to gate. The view-to-download mapping is three rules, per the requirements: file intersection for report-backed views, download `Type` for `answers` and `history`, and `Type` again for `run_membership`, since an incomplete run has no membership file for a per-run mapping to find. -2. Open an engine over the dataset with materialization suppressed, so a view is always copied from its raw artifacts and a stale Parquet can never be copied forward into a fresh-looking one. `Open` has three non-test callers (`cmd/query.go:60`, `cmd/repl.go:34`, `internal/mcpserver/tools.go:375`), so rather than changing its signature at all four sites this is a sibling `OpenRaw` sharing one unexported constructor with `Open`. -3. Per view, in order: refuse it when `OpenRaw` registered it from its `fallback` statement, which is total loss and has no override; refuse it when it declares an input that is not on disk and `--allow-partial` was not passed, naming the files; skip it when the manifest's entry is fresh and `--force` was not passed. Otherwise `COPY (SELECT * FROM "") TO '' (FORMAT parquet, COMPRESSION zstd, KV_METADATA {...})` into a temp name in `materialized/`, and stop there. The temp name comes from `os.CreateTemp` with a `.parquet.tmp-*` pattern rather than from the pid: the copies deliberately run outside the mutation locks, so one process can have two runs in flight over the same dataset, and a pid-derived name would have them writing the same file. For a store-backed view, assert the Parquet's row count against `m.Stores[typ].Count` and refuse the view on a mismatch. The KV metadata carries the view name and build time as human-readable provenance; nothing reads it back. -4. Re-take the lock (`dataset.LockMutation`, exported for this, so the lock ordering has one implementation), re-read the manifest, repoint only the views whose inputs still fingerprint as they did when copied and that the view still declares, `fsutil.RenameAtomic` those temp files into place, delete the rest, and write the manifest last. The fingerprints to compare against are taken just before each copy starts, not rebuilt at the repoint, or the comparison is against itself. Renaming after the re-check rather than before is what keeps the manifest write the commit point, as it is in `mergeUnderLock` and `Purge`; renaming in step 3 would leave a complete, unreferenced Parquet inside a folder that reindex and the orphan check are contractually blind to. A view whose inputs moved is discarded, with its own status rather than being folded in with the views that needed no work: the Parquet describes a state that is already gone, so the view is not materialized and running again picks it up, which is the opposite of what "already fresh" would tell the reader. - -The refusals in step 3 are per view rather than per command, because the evidence is per view. Clean views are still materialized, the refused view's outcome carries the reason for the caller to render, and the command exits non-zero if any view was refused. - -Two message-content requirements ride on `Refused` and on the progress writer, and neither is decoration: - -- A missing-input refusal names the files **and both remedies**, because the point of refusing is to surface them rather than to leave a flag to be typed forever: re-fetch the run, or run `dataset reindex` to drop the download entry so the file stops being expected. Without the remedies the researcher's only discoverable move is `--allow-partial` on every run afterwards, which is the erosion the flag was kept separate from `--force` to avoid. - -``` -view reports declares 2 inputs and 1 is missing: report_200.csv - re-fetch run 200, or run cc-data dataset reindex to drop the entry so the file stops being expected - to build from what is on disk anyway, pass --allow-partial -``` - -- Under `--allow-partial` the run reports how many of the declared inputs it built from, per view, mirroring `clue-documents/build-parquet.sh:60`'s "--allow-partial given; building from N of M documents". A count is what makes a shortfall land: "1 of 2" is read, "proceeding anyway" is not. - -``` ---allow-partial given; building reports from 1 of 2 declared inputs -``` - -Both go through `output.Progressf` to stderr, like every other `dataset` subcommand's prose. - -The suppression flag in step 2 is the subtle part. Without it, `Materialize` would open the dataset, the engine would helpfully point `logs` at yesterday's Parquet, and the copy would write that Parquet back out with today's fingerprints attached, laundering stale data into a fresh-looking entry. - -Tests: materializing then querying returns identical rows and column types for every view in `MaterializableViews()`; a second run skips everything; `--force` rewrites; a view whose declared input is missing is refused while every other view is still written, and proceeds under `--allow-partial`; a view that fell back to typed-empty is refused with `--allow-partial` given; a store-backed Parquet short of `Stores[typ].Count` is refused; an incomplete download warns and blocks nothing; a `get` between the copy and the repoint leaves the view unmaterialized rather than wrongly fresh (driven through a `testHookBeforeRepoint` seam in `duck`, the same shape as `merge.go`'s hook, which also proves the mutation locks are free in that window, since the `get` would otherwise fail as busy); and no `.tmp-` file survives any of those paths. For the guard: a second run fails as busy while the first holds it; the guard is free again once a run returns; a temp file planted before a run is swept by it; and the guard is still held at the point the temp files exist, checked through the same `testHookBeforeRepoint` seam. That last one is the only test that can fail if the guard is taken and released immediately, which is otherwise indistinguishable from holding it and is the shape that would make the sweep unsafe. - ---- - -## Report materialization in show and list - -**Summary**: The user-visible reporting, separated from the mechanics so its contract change is reviewable on its own. - -**Files affected**: -- `internal/dataset/summary.go`: `materialized_bytes`, the unreferenced-Parquet warning -- `internal/duck/materialize.go`: `StaleMaterializedViews`, `AnnotateShowJSON` -- `cmd/dataset_show.go`, `internal/mcpserver/tools.go`: both callers annotate -- `internal/dataset/summary_test.go` - -**Estimated diff size**: ~190 lines - -`ShowJSON` and `ListRowJSON` each gain `MaterializedBytes int64 \`json:"materialized_bytes"\``. Both are additive: `size_bytes` keeps its meaning, since it is a documented stable contract that `dataset list` also publishes. - -`dirSize` currently walks the tree once. It becomes a single walk returning both totals, so `dataset list` does not pay a second walk per dataset: - -```go -// dirSizes returns the dataset's total bytes and the subset under registered derived -// subfolders, in one walk. Derived bytes are the ones a researcher can reclaim for free. -func dirSizes(dir string) (total, derived int64) -``` - -One warning is added, matching the existing `MISSING_FILE` / `INCOMPLETE` / `ORPHAN_FILE` style: - -Two warnings are added, matching the existing `MISSING_FILE` / `INCOMPLETE` / `ORPHAN_FILE` style, and they live in different places for a reason worth stating. - -``` -UNREFERENCED_MATERIALIZED: materialized/logs.parquet is on disk but the manifest no longer records it; queries are reading the raw artifacts, run cc-data dataset materialize to restore it (external tools can still read the file) -STALE_MATERIALIZED: view logs was materialized from inputs that have since changed; run cc-data dataset materialize to refresh it -``` - -The first needs no view knowledge, only the folder listing against `m.Materialized`, so it goes straight into `driftWarnings`, lands inside the existing `sort.Strings`, and reaches every caller of the show contract without any of them changing. It covers both the state a reindex leaves and the orphan a discarded materialize run leaves. Its wording deliberately does not tell the researcher to delete anything: an external script may be reading that file, and it is still correct for that reader. - -**The second cannot be produced by `driftWarnings`.** Deciding staleness needs the view's current input list, which only `internal/duck` knows, and `internal/dataset` cannot import it. So `duck` exports the check plus one decorator, and both callers of `BuildShowJSON` use it: - -```go -// StaleMaterializedViews names the views whose recorded Parquet no longer matches the inputs. -// It lives here, not in dataset, because a view's input list is a property of the view set. -func StaleMaterializedViews(d *dataset.Dataset) ([]string, error) - -// AnnotateShowJSON appends the warnings only the view set can decide, and re-sorts. Both -// surfaces call it: the CLI at cmd/dataset_show.go:32 and the MCP dataset_show tool at -// internal/mcpserver/tools.go:193, which publish the same documented ShowJSON contract. -func AnnotateShowJSON(d *dataset.Dataset, s *dataset.ShowJSON) error -``` - -Neither file imports `duck` today, so both gain the import. Routing them through one decorator rather than letting each remember is what keeps the two surfaces from publishing different warning sets, and keeps the re-sort inside the function that owns the ordering invariant. - -That also keeps exactly one implementation of freshness, shared by the reader, the writer and the warning. A second copy in `dataset` would be the "one source of truth" problem in its most literal form: two answers to "is this Parquet usable" that can disagree. - -`renderShow` prints the derived figure beside the total when non-zero. `reindex` needs no code to drop the entries: `reindexIdentity` constructs a fresh `Manifest` without a `Materialized` map, so they are already gone by construction. Three tests rather than one change: the entries are gone, `dataset show` then raises `UNREFERENCED_MATERIALIZED`, and the Parquet files are still on disk. The last is the one that catches a later "tidy-up" making reindex delete the folder, which would break the external readers this story exists for. - ---- - -## Wire the CLI command and the MCP tool - -**Summary**: The two surfaces, together, because the drift guard checks the MCP tool inventory in both directions and would go red on a split. - -**Files affected**: -- `cmd/dataset_materialize.go` (new), `cmd/dataset.go` -- `cmd/dataset_materialize_test.go` (new) -- `internal/mcpserver/tools.go`, `internal/mcpserver/types.go`, `internal/mcpserver/server_test.go` -- `internal/guidance/src/tools.md`: the catalog entry, which the drift guard requires as soon as the tool is registered - -**Estimated diff size**: ~250 lines - -The command follows the shape of the existing `dataset` subcommands exactly: `resolveExistingRef`, `echoRef`, `notFound`, `mutationErr`, and progress through `output.Progressf` so prose goes to stderr. - -``` -cc-data dataset materialize [--force] [--allow-partial] -``` - -There is no view-selection flag, deliberately: `materialized/` stays a deterministic mirror of the materializable set, so an external script hardcoding a path can tell an absent file from absent data. The command renders `MaterializeResult` as written, skipped and refused counts, names each refused view with its reason, and returns a non-zero exit when any view was refused, so a `set -euo pipefail` recipe stops rather than consuming a surface missing a view. The exit is `MATERIALIZE_REFUSED` in the table's exit-1 "internal/other" class rather than the exit-5 contract class, which `cmd/root.go:23` documents as a *server* contract error: a refusal is entirely local, and mis-classing it would mislead anyone scripting on the code. - -The MCP tool is annotated neither read-only nor destructive: it writes, but only derived data that is regenerable and documented as safe to delete. `server_test.go:109`'s pinned inventory gains `dataset_materialize`, which is what makes forgetting the catalog entry a CI failure rather than a discovery. That guard fires the moment the tool is registered, so the `tools.md` catalog entry lands in this step rather than with the rest of the guidance; the core and skill-header prose stays in the next one. - -Tests, following `cmd/get_report_test.go`'s shape: a clean run exits zero and names the views written; a run where one view's input is missing exits **non-zero** while the other views are still written, and the message carries both remedies; the same run under `--allow-partial` exits zero and reports "N of M" for the affected view. The exit code is the assertion that matters, since it is what stops a `set -euo pipefail` recipe from consuming a surface missing a view, and it is the one always-on test in the requirements with no home anywhere else in this plan. - ---- - -## Single-source the guidance and the researcher guide - -**Summary**: The prose the drift guard requires, plus the external-path documentation the study scripts depend on. - -**Files affected**: -- `internal/guidance/src/core.md`: when to materialize, and that cc-data never removes the folder on its own -- `internal/guidance/src/skill_header.md`: the CLI spelling -- `internal/guidance/src/tools.md`: the tool catalog entry -- `docs/researcher-guide.md`: the `materialized/` layout - -**Estimated diff size**: ~140 lines - -The split between core and skill header is forced by `TestCoreNamesNoCommand`, which rejects any "`cc-data `" spelling in the core. So the core says *when* materializing is worth it, and carries both halves of the deletion sentence: the folder is safe for the researcher to delete at any time, and cc-data itself removes it only through purge and delete. The two sit together because the first alone reads like a warning that the tool might remove it. The skill header carries the command spelling. The tool catalog entry is what `TestGuidanceDocumentsEveryTool` requires. - -The researcher guide gains the layout as a documented, stable contract, since REPORT-120 depends on reading it directly: - -``` -//datasets//materialized/.parquet -``` - -with three notes. These are ordinary Parquet files carrying each view's own columns, readable by any DuckDB, pandas or Polars without cc-data. They are safe for the researcher to delete at any time, **and** cc-data itself removes them only through `dataset purge` and `dataset delete`; the two sentences sit together, because read alone the first one reads like a warning that the tool might delete them, which is exactly what a script hardcoding a path needs to know is untrue. And the folder is larger than the view a study reads, for two measured reasons: `reports` unions the same log CSVs `logs` reads, so both carry those rows (5.4 MB beside 9.5 MB on a 200,000-row fixture, roughly 7% overlap once REPORT-110's 313 MB `history` dominates), and a view's derived columns become disk when materialized rather than being computed per query (`parameters` 2.62 MB beside `parameters_json` 2.62 MB, 45% of `logs.parquet` in total). - ---- - -## Add the opt-in timing test - -**Summary**: The measurement, gated so it does not run in CI. Last because it is the only step nothing else depends on. - -**Files affected**: -- `internal/duck/materialize_bench_test.go` (new) - -**Estimated diff size**: ~150 lines - -Skipped unless an environment variable is set, because CI runs plain `go test ./...` with no `-short` convention and the fixture is roughly 540MB of generated JSONL. - -Two assertions, and the fixture generator carries a comment saying why its payloads vary: a first attempt used an identical blob per row, ZSTD dictionary encoding took the Parquet to 0.9% of the JSONL, and the measured speedup collapsed from 54x to 2.6x because the cost had moved entirely into the aggregation. A generator emitting a constant blob measures compression, not materialization. - -- A column-pruned aggregate (`GROUP BY question_id`) is at least 10x faster materialized. Measured 60x at 1M rows, so the threshold carries six times the headroom rather than being tuned until it passed. The test names the shape and says column pruning is the mechanism under test, so nobody later "fixes" a regression by swapping the query. -- A high-cardinality aggregate (`count(DISTINCT remote_endpoint)`, measured 4.9x) is **not slower**. That is a not-worse check rather than a speedup check, and it is what would catch a change that made materialization a pessimization for the shapes it does not help. - ---- - -## Open Questions - -None. - -## Self-Review - -Roles chosen for what this document has to survive: the engineer reviewing the resulting commits (are the steps really independent, does step N compile without N+1), the engineer writing the named tests (can each be written against the harness this plan provides), and the operator. Every finding was checked by running code against the real view set; the ones that did not survive are not recorded. - -### Commit reviewer - -#### RESOLVED: The materializable predicate, as written, selected nothing - -The plan derived `MaterializableViews()` "from the statement set over an empty manifest filtered to those declaring files", copying how `StaticViewNames()` works. Ran it: over an empty manifest, **0 of 12** views declare files. Every builder returns its stand-in when there is nothing to read, and a stand-in scans no files, so the function would have returned an empty set and materialization would have silently done nothing on every dataset. - -"Declares files" is a property of a *populated* manifest, unlike "is a static view", which is exactly why `StaticViewNames` can use an empty one. `MaterializableViews` now takes the manifest. - -#### RESOLVED: The same predicate wrongly included the per-run report views - -Over a populated manifest the views declaring files came out as `reports`, `report_prompts`, `answers`, `run_membership` and **`report_7`**. `perDownloadViews` sets `files: dl.Files` on both `report_` and `report__job_` (`internal/duck/views.go:451`, `:456`), so the bare predicate contradicts the requirements' Out of Scope, which excludes per-run views because they multiply with the run count. - -Fixed by intersecting with `StaticViewNames()`. Both halves stay code-derived, so a view added later joins or stays out of the set by its own construction rather than by a list someone remembers to update. The requirements' wording is corrected to match, since it stated the bare predicate too. - -#### RESOLVED: `Fresh` was declared with one signature and called with another - -Declared as `Fresh(dir string) bool` in the manifest step and called as `mat.Fresh(vs.canonDir, st.files)` in the engine step, one step later. The prose underneath already said the caller passes the view's current file list, so the declaration was simply stale. Step 2 would not have compiled against step 3. - -### Test author - -#### RESOLVED: The staleness warning cannot be written where the plan put it - -The plan put `STALE_MATERIALIZED` in `driftWarnings`, which lives in `internal/dataset`. Deciding staleness needs the view's current input list, and that is a property of the view set in `internal/duck`, which `dataset` cannot import: the dependency runs `duck` to `dataset` and never back (`internal/duck/engine.go:13`). Writing it as planned would have forced a second view-to-files mapping inside `dataset`, which is two answers to "is this Parquet usable" that can drift apart. - -Moved to a `duck.StaleMaterializedViews` helper called from `cmd/dataset_show.go`, which already imports both, so one implementation of freshness serves the reader, the writer and the warning. - -### Operator - -#### RESOLVED: Changing `Open`'s signature touches every caller for one internal need - -Materialization needs an engine that ignores existing Parquet, which the plan added as a flag on `Open`. `Open` has three non-test callers (`cmd/query.go:60`, `cmd/repl.go:34`, `internal/mcpserver/tools.go:375`), none of which has any interest in the flag. Changed to a sibling `OpenRaw` over a shared unexported constructor, leaving all three untouched. - -#### Checked and not a problem: materialize inherits the sandbox's 2GB memory cap - -Because materialization reuses the sandboxed engine, its `COPY` runs under `memory_limit = 2GB` (`internal/duck/engine.go:111`), which looked like a risk against a 15 GB corpus. Already measured: the 6.38M-row, 2.92 GB fixture materialized in 5.0s under exactly that cap, because `COPY` streams rather than buffering. Recorded rather than actioned, so the next person does not re-open it. - ---- - -## Self-Review (round 2) - -Roles: the engineer reviewing the resulting commits, the engineer writing the named tests, and the operator. Each finding was checked against the running code; two candidates that did not survive are noted at the end. - -### Commit reviewer - -#### RESOLVED: `cmd/dataset_show.go` does not import `duck`, and it is not the only caller of `BuildShowJSON` - -The plan places `STALE_MATERIALIZED` in `cmd/dataset_show.go` "which already imports both". It does not: its imports are `fmt`, `sort`, `text/tabwriter`, `internal/dataset`, `internal/output` and `cobra`. Adding the import is trivial, so the step still works, but the reason given for choosing that file is not the reason that holds. - -The consequence that does matter is the second caller. `BuildShowJSON` has exactly two non-test callers: `cmd/dataset_show.go:32` and `internal/mcpserver/tools.go:193`, where the `dataset_show` tool returns the summary struct unchanged. Under the plan the CLI shows the staleness warning and the MCP tool does not, so the same documented `ShowJSON` contract carries different warning sets on the two surfaces. Since materialize is itself exposed over MCP, the surface that can act on the warning is the one that never sees it. - -A smaller version of the same problem: `driftWarnings` ends with `sort.Strings(warnings)` (`internal/dataset/summary.go:236`), so warnings appended by a caller land after the sort and break the ordering the struct has always had. - -Suggested resolution: have both callers go through one decorator rather than each remembering, for instance a `duck.AnnotateShowJSON(d, s) error` that appends the warnings and re-sorts, called from `cmd/dataset_show.go` and from the MCP tool. One implementation, two call sites, and the sort stays inside the function that owns the invariant. - -**Resolved as suggested, and the Q2 resolution shrinks it.** The unreferenced-Parquet warning needs no view knowledge, so it goes straight into `driftWarnings` and reaches both surfaces for free. Only `STALE_MATERIALIZED` still needs `internal/duck`, and it goes through one `duck.AnnotateShowJSON(d, s) error` that appends and re-sorts, called from `cmd/dataset_show.go:32` and `internal/mcpserver/tools.go:193`. Both call sites gain the import; the file list for this step gains `cmd/dataset_show.go` and `internal/mcpserver/tools.go`. - -#### RESOLVED: Step 3 renames into place, but step 4 discards by removing "its temp file" - -The two steps disagree about when the rename happens. Step 3 copies to a temp name and renames to `materialized/.parquet`; step 4 then says a view whose inputs moved "is discarded, its temp file removed". After step 3 there is no temp file: what exists is a complete, final-named Parquet that the manifest does not reference. - -That orphan is unusually well hidden. It sits inside a registered derived subfolder, so by this spec's own contract `Reindex` will not adopt it and the orphan-file check will not flag it, and it is counted in `materialized_bytes` forever. It also inverts the commit discipline the rest of the package follows, where the manifest write is the commit point and files land after it (`internal/dataset/dataset.go:268`, `internal/dataset/merge.go:146`). - -Suggested resolution: hold the temp name until after the repoint. Copy every view to `materialized/.parquet.tmp-`, re-take the lock, re-check the fingerprints, then rename the survivors and delete the rest, and write the manifest last. Add a test asserting no `.tmp-` file survives a discarded run, which the plan already lists but which cannot pass under the current ordering. - -**Resolved as suggested.** Step 3 copies to `materialized/.parquet.tmp-` and stops there; step 4 re-takes the lock, re-checks the fingerprints, renames the survivors with `fsutil.RenameAtomic`, deletes the rest, and writes the manifest last, so the manifest write stays the commit point as it is in `merge.go` and `Purge`. The named test for no surviving `.tmp-` file can then actually fail, which under the old ordering it could not. - -#### RESOLVED: The writer needs a statement-executing path that `Engine` does not expose - -`Engine`'s only exported method is `Query`, which returns `*sql.Rows`. Running `COPY ... TO` through it and dropping the result leaks the rows and deadlocks `Close`: reproduced here, and the test hung for the full ten-minute timeout in `sql.(*Conn).close` waiting on `closemu` while five Parquet files sat correctly written on disk. `Materialize` lives in `package duck` so it can reach `e.conn` directly, but the writer step's file list names only `materialize.go` and its test, and the choice is worth making deliberately rather than discovering. - -Suggested resolution: add an unexported `func (e *Engine) exec(ctx, sql string) error` in `engine.go`, list `engine.go` in the writer step's files, and use it for both the `COPY` and any `PRAGMA` the writer needs. - -**Resolved as suggested.** `engine.go` joins the writer step's file list. - -### Test author - -#### RESOLVED: The reindex test as described cannot fail for the right reason - -The plan says reindex "needs no code" because `reindexIdentity` builds a fresh manifest, and gives it a test rather than a change. That test is a legitimate regression guard against a future refactor that starts carrying the manifest forward, and it should stay. But it is the only test the plan names for the reindex requirement, and the requirement has two halves: entries are dropped, and `dataset show` then reports the dataset as needing re-materializing. The second half is untested because, as the requirements-side finding records, it has no mechanism. A reader of the plan sees one requirement with one test and no gap. - -Suggested resolution: whichever way the requirements-side question is settled, name the test for it here. If the warning moves to a folder-based check, the test is "reindex, then `dataset show` names the leftover Parquet"; if the second half is dropped, say so in this step so the missing test is a decision rather than an omission. - -**Resolved.** The requirements-side question settled on the folder-based check, so this step keeps the existing regression guard (`reindexIdentity` drops the map by construction, and the test goes red if a refactor starts carrying it forward) and adds two more: after a reindex, `dataset show` names the unreferenced Parquet, and the Parquet files are still on disk afterward. The second is the one that would catch someone later "tidying up" by making reindex delete the folder, which would break the external readers this story exists for. Because the new warning needs no view knowledge, it goes in `driftWarnings` rather than through the caller-side decorator, so its test lives in `internal/dataset/summary_test.go` alongside the other warning codes. - -### Operator - -#### RESOLVED: `Open` now builds the full statement set twice per dataset - -`applyMaterialized` calls `MaterializableViews(vs.m)`, which constructs a second `viewSet` and calls `statements()` on it, while the caller has already called `statements()` to get the slice being annotated. Every view's SQL, including one `report_` view per report download and one `answers_` per membership entry, is built twice on every `Open`, `query`, `repl` and MCP `query` call, materialized or not. - -This is string building rather than I/O and it will not be visible at 20 runs. It is worth one line of thought at REPORT-110's scale, and it is trivially avoidable. - -Suggested resolution: give `MaterializableViews` an unexported sibling taking the already-built `[]viewStmt`, and let the exported form build the statements once and delegate. The writer and the tests keep the exported signature. - -**Resolved as suggested.** No behavior change, so no new test; the existing `MaterializableViews` test covers the exported form and `applyMaterialized`'s tests cover the sibling. - -### Checked and not a problem - -- **Materializing a degraded view does not launder its warning.** `reportUnionView` calls `vs.warnf` while *constructing* the statement, so the "report CSV is missing on disk" warning fires on every `Open` whether the view is later read from Parquet or not. Verified. -- **A corrupt store JSONL cannot be materialized into an empty Parquet.** `read_json` with an explicit `columns=` map does not validate content at `CREATE VIEW`; it fails at query time. Verified: the view registers with no warning and the query fails outright, so the `COPY` fails too rather than writing zero rows. The typed-empty degradation applies to files that are missing or unopenable, which is the case the fingerprint already covers. diff --git a/specs/REPORT-113-dataset-materialization/requirements.md b/specs/REPORT-113-dataset-materialization/requirements.md deleted file mode 100644 index 1ecaedb..0000000 --- a/specs/REPORT-113-dataset-materialization/requirements.md +++ /dev/null @@ -1,303 +0,0 @@ -# Dataset materialization (Parquet query surface) - -**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-113 -**Repo**: https://github.com/concord-consortium/cc-data-cli -**Implementation Spec**: [implementation.md](implementation.md) -**Status**: **In Development** - -## Overview - -Add `cc-data dataset materialize`, which writes each of a dataset's file-backed views to a ZSTD Parquet file 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. Because the output is a plain Parquet file at a predictable path, an external tool can open it directly without going through cc-data at all. - -## Project Owner Overview - -cc-data answers every query by re-reading the raw JSONL and CSV a fetch produced. That is right for a few thousand answers and wrong at the scale a CLUE corpus reaches, where a dataset is measured in gigabytes and every question pays the full parse again. It is also the last piece of storage machinery our study code still owns: the Dataflow study maintains a hand-written Parquet build, with its own column-type declarations and its own guard against a partial build replacing a complete one, purely because the tool does not offer one. - -This story takes over both jobs. A researcher runs one command and gets a fast, typed, columnar copy of their dataset that their existing analysis scripts can read by path, and the hand-written build script is deleted rather than reimplemented. - -## Background - -The query engine opens an ephemeral in-memory DuckDB per invocation, registers a view per store, per report CSV and per join dimension from the manifest, then locks a sandbox confining file access to the dataset folders (`internal/duck/engine.go:31`, `internal/duck/views.go:56`, `internal/duck/engine.go:116`). Store views read JSONL through `read_json` and report views read CSV through `read_csv`, both with explicit typed column maps, so every query re-parses everything it touches. - -**The consuming story is what sets this story's scope.** REPORT-120 exists to retire `sources/clue/recipes/log-events/build-parquet.sh` onto `cc-data dataset materialize`, and its acceptance criteria are about the *materialized query surface*: that the study's queries return the same rows against the materialized dataset as against the hand-built Parquet, and that id columns land as VARCHAR so a join needs no cast. - -The dataset REPORT-120 materializes contains **no stores at all**. REPORT-119 puts the log events in as a Student Actions report run downloaded with `get report`, which writes `report_.csv` (`internal/fetch/report.go:332`); REPORT-118 puts the cohort in as a Student ID Mapping run, also a report CSV; and REPORT-112's `logs` view is a union over log-type report CSVs with derived columns. The CLUE stores do not arrive until REPORT-110, which is 0.3.0. So materialization scoped to stores would run on REPORT-119's dataset and produce nothing, and REPORT-120 could not land. - -That also inverts the usual reading of this story. At 0.2.0 the data is 258,916 log rows, which is small, and the need is ownership of a typed surface rather than speed. At 0.3.0 REPORT-110 lands a 6.38M-entry, 15 GB `clue_history` store and names this story as its scale answer. Materializing views rather than artifacts serves both. - -**External readability is a first-class requirement, not a side effect.** `sources/researcher-access.md:609` records that what this story is depended on for is the Parquet landing on disk where an external DuckDB can read it, not only being queryable through the CLI: every analysis script in `cc-data-studies` is plain Python invoking `duckdb` as a subprocess with no dependency on cc-data, and that independence is what makes the scripts outlive the tool. `studies/clue-dataflow-behavior/lib.py:22-24` names three Parquet paths as its entire data source. - -## Requirements - -- `cc-data dataset materialize /` writes one ZSTD Parquet per materializable view to `/materialized/.parquet`. -- A view is materializable when it is one of the static views **and** declares the files it reads. Both halves are derived from the view set rather than listed by hand. The static half is what excludes the per-run views (`report_`, `answers_`), which declare files but multiply with the run count. Together they select `reports`, `report_prompts`, `logs`, `answers`, `history`, `run_membership`, `student_id_mapping` and `student_metadata`, and excludes the manifest-derived VALUES tables (`downloads`, `attachment_files`), where materializing is pointless, and the `read_text` attachment views (`attachment_states`, `attachment_content`), which would copy every attachment's bytes. -- The Parquet carries the view's own output, derived columns included, so `materialized/logs.parquet` holds REPORT-112's `parameters_json`, `extras_json`, `event_time` and `received_time`, not the raw CSV's columns. -- Each file is written to a temporary name and renamed into place, so a partial file is never mistaken for a complete one. -- Materialization refuses a view that declares an input the manifest names but that is not on disk, unless `--allow-partial` is passed, and names the missing files. This is the whole of the guard, and the condition it exists for: `sources/clue/recipes/clue-documents/build-parquet.sh:34` states the invariant as "every document that should have history must have a .jsonl right now", and it is what a glob matching fewer files once turned into 6.27M entries replaced by 110k. The manifest is cc-data's source list, so the same invariant reads as: every file a view declares must be on disk right now. When overridden, the run reports how many of the declared inputs it built from, matching the source script's "building from N of M documents". -- A run reports one outcome per materializable view, each being written, already fresh, discarded or refused, with a reason on the ones that need explaining. One list rather than a list per outcome, so a view cannot be reported twice or not at all, and so the MCP tool returns the outcomes as they stand rather than a hand-assembled projection that stops mentioning an outcome added later. A discarded view in particular must not be reported as already fresh: it is the one case where the view is not materialized and running again would fix it. -- The refusal is per view, not per command, because the evidence is per view: this view declares a file that is not there. Clean views are materialized, refused views are named with the files they are missing, and the run exits non-zero so a `set -euo pipefail` recipe stops. That is the shape `sources/clue/recipes/clue-documents/download-history.ts:206` already uses for a multi-item operation: work every item, summarize per item, exit non-zero if any failed. A dataset with a subset of its views materialized is an ordinary state, since freshness is per view and it is the same state a newly downloaded run produces. -- A download marked `!Complete` is reported as a warning naming the runs, not a refusal, and `--allow-partial` does not gate it. Exactly one place in the tree writes `Complete: false` (`internal/fetch/paged.go:195`, the store fetch), because report, report_job and attachment downloads write to a `.tmp` and record their entry only after the rename succeeds. A paged fetch merges once at the end, so an incomplete store download leaves the store file untouched and the Parquet byte-identical to one built if the run had never been requested. There is no shortfall to refuse. `dataset show` already carries the same fact as `INCOMPLETE`. -- The view-to-download mapping the warning needs is stated rather than left to be inferred, because no single rule covers it. Report-backed views map to their downloads by file intersection. `answers` and `history` map by download `Type`, since the store is one merged file across every run and carries no per-run identity. `run_membership` maps by `Type` too: its filenames do encode `(type, run)`, but an incomplete run has no membership file at all, so a per-run mapping would never see the run the warning is about. -- The refusal names both remedies for a missing input, since the point of refusing is to surface them rather than to leave a flag to be typed forever: re-fetch the run, or run `dataset reindex` to drop the download entry so the file stops being expected. -- Materialization refuses outright, with no flag to override it, for a view that registered from its typed-empty fallback rather than its primary statement. That is total loss rather than a shortfall, always a broken dataset, and there is no legitimate reason to publish a knowingly empty Parquet. The engine already distinguishes the two cases; it is the branch that warns "degraded to empty". -- After copying a store-backed view, the run asserts the Parquet's row count against the manifest's recorded store count and fails the view if they disagree. Both hand-written build scripts this story retires end with exactly this check (`log-events/build-parquet.sh:97`, `clue-documents/build-content-parquet.sh:102`). The union views get no such check: the pseudo-header filtering makes the arithmetic inexact, and an approximate assertion is worse than none. -- `--force` re-materializes a view whose recorded inputs are unchanged, which is otherwise a no-op. It does not override the missing-input refusal; that is `--allow-partial`'s job alone. -- Freshness is one rule for every view: the manifest records each materialized view's input files with a fingerprint, and the view's Parquet is used only while every input still matches. Otherwise the view is registered from its raw artifacts, with no error and no user action required. -- An input that cannot be stat-ed records a reserved sentinel fingerprint, which no real fingerprint can collide with because a real one is always `-`. The sentinel then compares like any other fingerprint: an input that was absent when the Parquet was built and is absent still reads as fresh, while one that has since appeared, or has since gone missing, reads as stale. Telling those apart is the whole reason the sentinel is recorded rather than the input being skipped. This is the read side, and it is needed whichever way the write side is gated: a file can go missing after a successful materialization, and the Parquet must then fall back to the raw artifacts rather than be trusted. -- A Parquet the manifest names but that is absent on disk falls back to the raw artifacts, with a warning. The typed-empty fallback keeps the meaning it already has: nothing readable at all. -- Materialization does its copies outside the dataset's mutation locks, holding them only to read the manifest at the start and repoint it at the end, and discards a result whose inputs moved in between. A materialize run must not make a concurrent `get` fail as busy. -- A run holds a separate, dataset-scoped materialize guard for its whole duration, so two runs never work the same dataset at once and a second one fails as busy. `get` never takes this guard, so the rule above is unaffected. Two things follow. The duplicated work and the repoint race between overlapping runs are gone, and, because the kernel drops an `flock` when its holder dies however abruptly, any temp file in `materialized/` when a run acquires the guard is provably the work of a run that is no longer alive. -- A run sweeps those temp files unconditionally before it starts copying. This is the whole answer to an interrupted run: nothing in the tree installs a signal handler, so a killed run's deferred cleanup never executes and its temp Parquet would otherwise sit in a folder that no warning names, that reindex and the orphan check are contractually blind to, and that `materialized_bytes` counts forever. Making the next run clear it is preferred over reporting it and asking the researcher to act, and the guard is what makes the sweep safe rather than a heuristic: without it, a sweep would delete a concurrent run's in-flight copy and that run would then fail on the rename with a bare "no such file or directory". -- `dataset show` reports a view whose Parquet is stale, naming the command that refreshes it, and reports the materialized bytes separately from the dataset's total size. `dataset list` reports the same separate figure. -- `dataset purge` and `dataset delete` remove `materialized/` along with everything else they remove, and `Purge` clears `m.Materialized` alongside the four maps it already resets. Committing a manifest that still names files the same call is about to delete is what its own commit-ordering comment forbids (`internal/dataset/dataset.go:268`). -- `dataset reindex` drops the manifest's materialization entries rather than carrying them forward. Carrying them would need no Parquet reading and so no import cycle, and a reindex of an unchanged dataset leaves the materializable views byte-identical, so the files stay valid. It is still wrong: reindex is the command for recovering from a manifest cc-data cannot trust, and it has narrow paths that change a view's output while every declared input still fingerprints the same, notably a zero `FetchedAt` re-stamped from the clock, which silently reorders the dimension views' dedupe. Trading that against a rebuild measured in seconds is the wrong trade in the one command whose purpose is recovery. -- `dataset reindex` never deletes `materialized/`, and neither does anything else except `dataset purge` and `dataset delete`. The Parquet is this story's deliverable to tools outside cc-data, which need no manifest entry to read it, so cc-data losing confidence in a file is not grounds for cc-data to remove it. The derived-subfolder contract is therefore "never adopted, never reported as orphans, removed wholesale by purge and delete", and reindex is deliberately not on that list, which is also the right answer for REPORT-89's `exports/`. -- `dataset show` names any Parquet in `materialized/` that no manifest entry references, which is the state a reindex leaves and also the state a materialize run leaves if it discards a view after copying it. The message states what cc-data can and cannot vouch for rather than telling the researcher to clear it: the manifest no longer records these files, queries have gone back to the raw artifacts, `dataset materialize` restores them, and they stay readable by external tools meanwhile. Deciding this needs no view knowledge, only the folder listing against `Materialized`, so unlike the staleness warning it belongs in `driftWarnings` and reaches every caller of the show contract for free. -- Materialization reads only raw artifacts, never an existing Parquet. Copying a view that was itself reading a stale Parquet would write that stale data back out with fresh fingerprints attached, which is the one way this feature could silently corrupt an answer. -- Each Parquet carries its own provenance in the file footer: the view it holds and when it was built. Nothing in cc-data reads it back, since reindex deliberately does not try to revalidate a Parquet; it is there so a Parquet found outside the dataset, in a backup or a copied folder, can say what it is. -- A materialized view survives `dataset rename`. The manifest records dataset-relative paths resolved against the dataset directory at query time, so the folder moves with the rename. Verified, and asserted by a test, because unrelocatable stored paths are exactly why the persistent DuckDB was rejected. -- A materialized view resolves correctly in a multi-dataset session, under its schema prefix, and a dataset with no Parquet in the same session is unaffected. Verified, and asserted by a test. -- The derived-subfolder rule is a named predicate rather than a special case for one folder name, so REPORT-89 can register `exports/` against it. `materialized` and `exports` must not collide, and a test holds that a file planted inside a registered derived folder stays invisible to reindex and to the orphan-file warning. -- `dataset materialize` is exposed as an MCP tool, documented in the shared guidance catalog, and listed in the pinned MCP tool inventory. -- Guidance says when to materialize, that `materialized/` is safe for the researcher to delete at any time, and that cc-data itself removes it only through `dataset purge` and `dataset delete`. The two sentences sit together: read alone, the first one reads like a warning that the tool might delete it, which is exactly what an external script hardcoding the path needs to know is untrue. The "when" goes in the shared core both surfaces render; the CLI spelling goes in the skill header, since the core may not name a command. -- The researcher guide documents the `materialized/.parquet` layout as a stable path external tools may read directly, since that is what the study scripts depend on. -- `materialize` has no view-selection flag. `materialized/` is a deterministic mirror of the materializable set, so what it holds is a function of what the dataset holds, which is what lets an external script treat a path as a contract. A flag would make the contents a function of which flags someone last ran, and an outside reader could not tell an absent file from an unselected one. -- The researcher guide states the two measured duplication costs, so the folder size is expected rather than discovered. `reports` unions the log CSVs that `logs` also reads, so both carry the same rows: on a 200,000-row log fixture, `reports.parquet` was 5.4 MB beside a 9.5 MB `logs.parquet`, 36% of a folder whose largest member that study never opens. At REPORT-110's scale the same overlap is roughly 7%, because `reports` unions report CSVs only and the 313 MB `history` Parquet dominates. -- The identical-results test iterates the materializable views derived from the view set, not a hand-written list, so a view added later cannot go uncompared. -- Always-on tests, on a small fixture: identical results and identical column types for every materialized view; staleness detected after a `get`; fallback when the Parquet is deleted; per-view refusal on an input missing from disk, with the other views still materialized and a non-zero exit, and the same run proceeding under `--allow-partial`; an incomplete download warning without blocking anything; the un-overridable refusal for a view that fell back to typed-empty; the store row-count assertion failing a view whose Parquet is short; `Fresh` returning false once a recorded input is deleted; `--force` rebuilding an unchanged view; reindex dropping the entries and `dataset show` then naming the unreferenced Parquet, with the files themselves left on disk; and a file planted in a registered derived folder staying invisible to reindex and the orphan warning. -- The timing assertion needs a 1M-row fixture, roughly 540MB of generated JSONL, so it is opt-in through an environment variable rather than run on every CI job. It asserts a column-pruned aggregate at 10x against a measured 60x, and separately that a high-cardinality aggregate is not slower. Its fixture generator must emit varied payloads. - -## Technical Notes - -### Verified: the DuckDB mechanics - -Checked against the bundled engine (`github.com/duckdb/duckdb-go/v2 v2.10504.0`, `go.mod:6`, reporting `v1.5.4`), since the sandbox's `enable_external_access = false` made it an open question whether any of this was reachable: - -- `COPY (