diff --git a/cmd/dataset.go b/cmd/dataset.go index 6f2eeee..91c6c54 100644 --- a/cmd/dataset.go +++ b/cmd/dataset.go @@ -21,6 +21,7 @@ func newDatasetCmd() *cobra.Command { newDatasetPurgeCmd(), newDatasetShowCmd(), newDatasetListCmd(), + newDatasetMaterializeCmd(), newReindexCmd(), ) return cmd diff --git a/cmd/dataset_list.go b/cmd/dataset_list.go index c15c87e..76c9ac7 100644 --- a/cmd/dataset_list.go +++ b/cmd/dataset_list.go @@ -45,9 +45,13 @@ func renderList(list *dataset.ListJSON) { tw := tabwriter.NewWriter(out, 0, 2, 2, ' ', 0) fmt.Fprintln(tw, "REF\tDESCRIPTION\tAGE\tANSWERS\tHISTORY\tREPORT\tATTACH\tSIZE") for _, d := range list.Datasets { + size := humanBytes(d.SizeBytes) + if d.MaterializedBytes > 0 { + size += fmt.Sprintf(" (%s materialized)", humanBytes(d.MaterializedBytes)) + } fmt.Fprintf(tw, "%s\t%s\t%s\t%d\t%d\t%d\t%d\t%s\n", d.Ref, d.Description, humanAge(d.AgeSeconds), - d.Totals["answers"], d.Totals["history"], d.Totals["report"], d.Totals["attachments"], humanBytes(d.SizeBytes)) + d.Totals["answers"], d.Totals["history"], d.Totals["report"], d.Totals["attachments"], size) } tw.Flush() } diff --git a/cmd/dataset_materialize.go b/cmd/dataset_materialize.go new file mode 100644 index 0000000..bb0086a --- /dev/null +++ b/cmd/dataset_materialize.go @@ -0,0 +1,113 @@ +package cmd + +import ( + "context" + "os" + "os/signal" + "sort" + "strings" + + "github.com/concord-consortium/cc-data-cli/internal/dataset" + "github.com/concord-consortium/cc-data-cli/internal/duck" + "github.com/concord-consortium/cc-data-cli/internal/output" + "github.com/spf13/cobra" +) + +func newDatasetMaterializeCmd() *cobra.Command { + var force, allowPartial bool + cmd := &cobra.Command{ + Use: "materialize ", + Short: "Write each of a dataset's views to a Parquet file queries can read", + Long: "Write each of a dataset's file-backed views to " + dataset.MaterializedDir + + "/.parquet. Queries read a view's Parquet while it is fresh and fall back to the raw " + + "artifacts otherwise, so materializing never changes an answer. The files are ordinary Parquet " + + "and any tool can read them by path without going through cc-data.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + cfg, root, err := loadRuntime() + if err != nil { + return err + } + ref, err := resolveExistingRef(cfg, root, args[0]) + if err != nil { + return err + } + echoRef(ref) + d := dataset.Open(root, ref) + if !d.Exists() { + return notFound(ref) + } + opts := duck.MaterializeOptions{ + Force: force, + AllowPartial: allowPartial, + Progress: func(view string, i, n int) { + output.Progressf("[%d/%d] %s", i, n, view) + }, + } + ctx, stop := interruptible() + defer stop() + res, err := duck.Materialize(ctx, d, opts, output.Stderr()) + if err != nil { + return mutationErr(err) + } + return reportMaterialize(res) + }, + } + cmd.Flags().BoolVar(&force, "force", false, "re-materialize a view whose inputs are unchanged") + cmd.Flags().BoolVar(&allowPartial, "allow-partial", false, "build a view even though a file it declares is missing") + return cmd +} + +// interruptible returns a context the first interrupt cancels, so a run that is +// waiting to commit, or mid-copy, cleans up its temp files instead of leaving +// them for the next run's sweep. The handler is removed once it has fired, so a +// second interrupt kills the process as usual; the caller's stop removes it +// when the run ends without one. +func interruptible() (context.Context, context.CancelFunc) { + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) + go func() { + <-ctx.Done() + stop() + }() + return ctx, stop +} + +// reportMaterialize renders the per-view outcome and returns a non-zero exit when +// any view was refused, so a set -euo pipefail recipe stops rather than consuming +// a surface that is missing a view. +func reportMaterialize(res duck.MaterializeResult) error { + written, fresh := res.Written(), res.Fresh() + discarded, refused := res.Discarded(), res.Refused() + output.Progressf("written %d, already fresh %d, discarded %d, refused %d", + len(written), len(fresh), len(discarded), len(refused)) + if len(written) > 0 { + output.Progressf(" written: %s", strings.Join(written, ", ")) + } + for _, o := range res.Views { + if (o.Status == duck.StatusWritten || o.Status == duck.StatusFresh) && o.Reason != "" { + output.Progressf(" %s: %s", o.View, o.Reason) + } + } + if len(discarded) > 0 { + output.Progressf(" discarded, because a concurrent change moved their inputs; run again to pick them up: %s", + strings.Join(discarded, ", ")) + } + if !res.Refusals() { + return nil + } + views := make([]string, 0, len(refused)) + for view := range refused { + views = append(views, view) + } + sort.Strings(views) + for _, view := range views { + output.Progressf("view %s %s", view, refused[view]) + } + // Exit 1, the table's "internal/other" class: a refusal is a local condition, + // so the server-contract class would be a lie to anyone scripting on the code. + return &output.CLIError{ + ExitCode: output.ExitInternal, + Code: "MATERIALIZE_REFUSED", + Message: "refused to materialize " + strings.Join(views, ", "), + } +} diff --git a/cmd/dataset_materialize_test.go b/cmd/dataset_materialize_test.go new file mode 100644 index 0000000..8829c46 --- /dev/null +++ b/cmd/dataset_materialize_test.go @@ -0,0 +1,126 @@ +package cmd + +import ( + "os" + "runtime" + "strconv" + "strings" + "testing" + "time" + + "github.com/concord-consortium/cc-data-cli/internal/config" + "github.com/concord-consortium/cc-data-cli/internal/dataset" + "github.com/concord-consortium/cc-data-cli/internal/output" +) + +func materializeFixture(t *testing.T) *dataset.Dataset { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + if runtime.GOOS == "windows" { + t.Setenv("USERPROFILE", home) + } + root := t.TempDir() + t.Setenv("CC_DATA_ROOT", root) + + d, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, "") + if err != nil { + t.Fatal(err) + } + for _, run := range []int{100, 200} { + name := writeReportCSV(t, d, run) + rowCount, cols, order, dialect, err := dataset.DetectCSV(d.Path(name), "answers") + if err != nil { + t.Fatal(err) + } + if err := d.UpsertDownload(dataset.Download{ + Type: "report", RunID: run, ReportType: "answers", Slug: "slug", + Files: []string{name}, RowCount: &rowCount, Columns: cols, ColumnOrder: order, + CSVDialect: &dialect, Complete: true, FetchedAt: time.Unix(int64(run), 0).UTC(), + }); err != nil { + t.Fatal(err) + } + } + // A dimension view reading its own CSV, so one view stays clean when another + // view's input is deleted and the refusal can be shown to be per view. + mapping := "report_300.csv" + body := "learner_id,user_id,primary_user_id,student_id,class_id,offering_id,runnable_url,run_remote_endpoint\n" + + "1,11,11,s1,30,70,https://act/1,https://portal/dataservice/external_activity_data/AAA\n" + if err := os.WriteFile(d.Path(mapping), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + rowCount, cols, order, dialect, err := dataset.DetectCSV(d.Path(mapping), dataset.ReportTypePortal) + if err != nil { + t.Fatal(err) + } + if err := d.UpsertDownload(dataset.Download{ + Type: "report", RunID: 300, ReportType: dataset.ReportTypePortal, Slug: "student-id-mapping", + Files: []string{mapping}, RowCount: &rowCount, Columns: cols, ColumnOrder: order, + CSVDialect: &dialect, Complete: true, FetchedAt: time.Unix(300, 0).UTC(), + }); err != nil { + t.Fatal(err) + } + return d +} + +// writeReportCSV writes one report CSV and returns its dataset-relative name. +func writeReportCSV(t *testing.T, d *dataset.Dataset, run int) string { + t.Helper() + name := "report_" + strconv.Itoa(run) + ".csv" + body := "student_id,res_1_q1_answer\nPrompt,What?\nCorrect answer,42\n1,hello\n" + if err := os.WriteFile(d.Path(name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return name +} + +func TestDatasetMaterializeReportsAndExitsZero(t *testing.T) { + materializeFixture(t) + + _, errOut, code := runArgs(t, "dataset", "materialize", "learn.concord.org/ds") + if code != output.ExitSuccess { + t.Fatalf("clean run exit = %d, want 0: %s", code, errOut) + } + for _, want := range []string{"reports", "report_prompts", "written:"} { + if !strings.Contains(errOut, want) { + t.Fatalf("output should name %q: %s", want, errOut) + } + } +} + +// TestDatasetMaterializeExitsNonZeroOnARefusal is the assertion that matters: the +// exit code is what stops a set -euo pipefail recipe from consuming a surface +// that is missing a view. +func TestDatasetMaterializeExitsNonZeroOnARefusal(t *testing.T) { + d := materializeFixture(t) + if err := os.Remove(d.Path("report_200.csv")); err != nil { + t.Fatal(err) + } + + _, errOut, code := runArgs(t, "dataset", "materialize", "learn.concord.org/ds") + if code == output.ExitSuccess { + t.Fatalf("a refused view must not exit 0: %s", errOut) + } + if !strings.Contains(errOut, "report_200.csv") { + t.Fatalf("the message must name the missing file: %s", errOut) + } + if !strings.Contains(errOut, "re-fetch run 200") || !strings.Contains(errOut, "dataset reindex") { + t.Fatalf("the message must carry both remedies: %s", errOut) + } + if !strings.Contains(errOut, "view reports declares 3 inputs") { + t.Fatalf("the message must name the view and the shortfall: %s", errOut) + } + // The view that does not read the deleted CSV is still written, which is what + // makes the refusal per view rather than per command. + if !strings.Contains(errOut, "written: student_id_mapping") { + t.Fatalf("a clean view should still be written: %s", errOut) + } + + _, partialOut, partialCode := runArgs(t, "dataset", "materialize", "learn.concord.org/ds", "--allow-partial") + if partialCode != output.ExitSuccess { + t.Fatalf("--allow-partial should exit 0, got %d: %s", partialCode, partialOut) + } + if !strings.Contains(partialOut, "reports: built from 2 of 3 declared inputs") { + t.Fatalf("--allow-partial should report the shortfall as a count: %s", partialOut) + } +} diff --git a/cmd/dataset_show.go b/cmd/dataset_show.go index 1be1505..9b8170f 100644 --- a/cmd/dataset_show.go +++ b/cmd/dataset_show.go @@ -6,6 +6,7 @@ import ( "text/tabwriter" "github.com/concord-consortium/cc-data-cli/internal/dataset" + "github.com/concord-consortium/cc-data-cli/internal/duck" "github.com/concord-consortium/cc-data-cli/internal/output" "github.com/spf13/cobra" ) @@ -29,7 +30,7 @@ func newDatasetShowCmd() *cobra.Command { if !d.Exists() { return notFound(ref) } - summary, err := d.BuildShowJSON(full) + summary, err := duck.ShowJSON(d, full) if err != nil { return output.Internalf("%v", err) } @@ -51,7 +52,11 @@ func renderShow(s *dataset.ShowJSON, full bool) { if s.Description != "" { fmt.Fprintf(out, "%s\n", s.Description) } - fmt.Fprintf(out, "created %s, %s on disk\n\n", s.CreatedAt.Format("2006-01-02"), humanBytes(s.SizeBytes)) + disk := humanBytes(s.SizeBytes) + if s.MaterializedBytes > 0 { + disk += fmt.Sprintf(" (%s materialized)", humanBytes(s.MaterializedBytes)) + } + fmt.Fprintf(out, "created %s, %s on disk\n\n", s.CreatedAt.Format("2006-01-02"), disk) fmt.Fprintln(out, "Totals:") types := make([]string, 0, len(s.Totals)) diff --git a/docs/researcher-guide.md b/docs/researcher-guide.md index ccdb73c..0c7af75 100644 --- a/docs/researcher-guide.md +++ b/docs/researcher-guide.md @@ -344,6 +344,48 @@ When you (or Claude) query a dataset, the data is exposed as a set of SQL | `student_metadata` | One row per learner from Student Metadata runs: name, username, class, school, teachers, permission forms. | | `run_membership`, `downloads` | Provenance: which run's fetch covered which records, and what each download was, including whether its run hid names. | +### Speeding up a large dataset + +Every query re-reads the JSONL and CSV files your downloads produced. That is +fine for a few thousand answers and slow once a dataset runs to gigabytes. +`cc-data dataset materialize ` writes each view to a compressed Parquet file +that queries read instead, while the copy is still current: + +``` +//datasets//materialized/.parquet +``` + +A few things worth knowing about that folder. + +- **It never changes an answer.** A query returns the same rows either way. When a + later download changes a file a view reads, that view quietly goes back to + reading the raw files, and `cc-data dataset show ` tells you which views + are worth refreshing. +- **The path is stable, and the files are ordinary Parquet.** Any tool that reads + Parquet, DuckDB, pandas or Polars, can open one directly without going through + cc-data, which is the point of the fixed path: analysis scripts can hardcode it. +- **You can delete the folder whenever you like**; queries go back to the raw + files, `cc-data dataset show ` names the views that did, and materializing + again rebuilds them. cc-data itself removes a finished `.parquet` only when you + purge or delete the whole dataset, so a script of yours can rely on one being + there until you say otherwise. The one exception is housekeeping: if a run is + interrupted partway through, the next run clears the half-written file it left + behind. +- **It is bigger than the one view you care about**, for two reasons. `reports` + unions the same log CSVs `logs` reads, so both carry those rows (on a + 200,000-row log fixture, a 5.4 MB `reports.parquet` beside a 9.5 MB + `logs.parquet`). And a view's derived columns cost nothing until they are + written down: `logs` carries both `parameters` and its parsed `parameters_json`, + which together with the other derived columns is about 45% of that file. +- **`dataset show` and `dataset list` report the folder's size separately**, so + you can see at a glance what deleting it would reclaim. + +If a view reads a file that is no longer on disk, materializing refuses that view +rather than writing a short Parquet, names the file, and tells you both ways to +fix it; other views are still written, and the command exits non-zero so a script +stops. `--allow-partial` builds from what is there anyway and says how much of +the declared input it used. + ### Report types You generate report runs in the report server, and `cc-data` downloads whichever diff --git a/internal/dataset/dataset.go b/internal/dataset/dataset.go index 272a22e..0ab1d47 100644 --- a/internal/dataset/dataset.go +++ b/internal/dataset/dataset.go @@ -1,6 +1,7 @@ package dataset import ( + "context" "errors" "fmt" "os" @@ -29,6 +30,7 @@ type Dataset struct { Dir string dsLock *store.DatasetLock actLock *store.ActivityLock + matLock *store.DatasetLock } // Open returns a handle for a ref under a data root; it does not create anything. @@ -39,6 +41,7 @@ func Open(dataRoot string, ref Ref) *Dataset { Dir: dir, dsLock: store.DatasetLockFor(dir), actLock: store.ActivityLockFor(dir), + matLock: store.MaterializeLockFor(dir), } } @@ -101,9 +104,11 @@ func Create(dataRoot string, ref Ref, description string) (*Dataset, error) { return d, nil } -// lockMutation acquires the activity (exclusive) then per-dataset lock, both -// non-blocking, returning a release func or ErrBusy. -func (d *Dataset) lockMutation() (func(), error) { +// LockMutation acquires the activity (exclusive) then per-dataset lock, both +// non-blocking, returning a release func or ErrBusy. Callers outside this package +// hold it only for as long as they are reading or repointing the manifest: a +// command that holds it for a long copy makes every concurrent get fail as busy. +func (d *Dataset) LockMutation() (func(), error) { ok, err := d.actLock.TryLock() if err != nil { return nil, err @@ -126,9 +131,43 @@ func (d *Dataset) lockMutation() (func(), error) { }, nil } +// LockMutationWait is LockMutation for a caller that would rather wait than +// fail: a materialize run committing minutes of copies while a fetch holds the +// activity lock. It is the only waiting acquisition, and it must stay the only +// one: Rename, Delete and Purge take the materialize guard while holding these +// locks, so if they waited for the guard a run waiting here would deadlock them. +func (d *Dataset) LockMutationWait(ctx context.Context) (func(), error) { + if err := d.actLock.LockContext(ctx); err != nil { + return nil, err + } + if err := d.dsLock.LockContext(ctx); err != nil { + d.actLock.Unlock() + return nil, err + } + return func() { + d.dsLock.Unlock() + d.actLock.Unlock() + }, nil +} + +// LockMaterialize takes the materialize guard without blocking, returning a +// release func or ErrBusy. A materialize run holds it from start to finish, +// which both keeps two runs off the same dataset and makes any temp file left in +// the derived folder provably the work of a run that has died. +func (d *Dataset) LockMaterialize() (func(), error) { + ok, err := d.matLock.TryLock() + if err != nil { + return nil, err + } + if !ok { + return nil, ErrBusy + } + return d.matLock.Unlock, nil +} + // Edit updates the dataset description under the mutation locks. func (d *Dataset) Edit(description string) error { - release, err := d.lockMutation() + release, err := d.LockMutation() if err != nil { return err } @@ -146,27 +185,40 @@ func (d *Dataset) Rename(dataRoot string, newName string) (*Dataset, error) { if err := ValidateName(newName); err != nil { return nil, err } - release, err := d.lockMutation() + release, err := d.LockMutation() + if err != nil { + return nil, err + } + // A materialize run holds its guard, whose lock file lives inside this + // directory, across copies that take no mutation lock. Windows cannot rename + // a directory with an open handle inside it, so the move has to wait for that + // run rather than fail after the manifest has already been rewritten. + releaseMat, err := d.LockMaterialize() if err != nil { + release() return nil, err } newRef := Ref{Portal: d.Ref.Portal, Name: newName} newDir := newRef.Dir(dataRoot) if _, statErr := os.Stat(newDir); statErr == nil { + releaseMat() release() return nil, fmt.Errorf("dataset %s already exists", newRef) } m, err := d.ReadManifest() if err != nil { + releaseMat() release() return nil, err } m.Name = newName if err := writeManifestFile(d.Dir, m); err != nil { + releaseMat() release() return nil, err } // Release locks before moving the folder (the lock files move with it). + releaseMat() release() if err := os.Rename(d.Dir, newDir); err != nil { return nil, err @@ -184,6 +236,14 @@ func (d *Dataset) Delete() error { if !ok { return ErrBusy } + // Same reason Rename takes it: a materialize run's guard file lives inside + // this directory and its copies hold no activity lock. + releaseMat, err := d.LockMaterialize() + if err != nil { + d.actLock.Unlock() + return err + } + releaseMat() // Release the flock handle before renaming: the lock file lives inside d.Dir, // and Windows cannot rename a directory that still has an open handle inside // it. Then tombstone-rename so the live directory vanishes atomically before @@ -256,11 +316,18 @@ func sameDownload(a, b Download) bool { // Purge deletes all downloaded artifacts and clears the manifest holdings while // keeping the dataset shell. Lock files are never removed. func (d *Dataset) Purge() error { - release, err := d.lockMutation() + release, err := d.LockMutation() if err != nil { return err } defer release() + // A materialize run's copies take no mutation lock and are written into the + // derived folder this removes, so the run has to be over first. + releaseMat, err := d.LockMaterialize() + if err != nil { + return err + } + defer releaseMat() m, err := d.ReadManifest() if err != nil { @@ -275,6 +342,7 @@ func (d *Dataset) Purge() error { m.Membership = map[string]MembershipRef{} m.Downloads = nil m.Attachments = nil + m.Materialized = map[string]Materialized{} if err := purgeCommitManifest(d.Dir, m); err != nil { return err } @@ -286,8 +354,22 @@ func (d *Dataset) Purge() error { // and prove Purge deletes nothing when the commit fails. var purgeCommitManifest = writeManifestFile -// deleteArtifacts removes stores, segments, membership files, CSVs, and the -// attachments directory, but never a lock file. +// MaterializedDir holds a dataset's Parquet query surface. +const MaterializedDir = "materialized" + +// derivedSubdirs are subfolders holding data cc-data generated from the dataset's +// own artifacts: never adopted as artifacts, never reported as orphans, removed +// wholesale by purge and delete. Reindex is deliberately not on that list, since +// the files stay readable by tools outside cc-data after cc-data stops vouching +// for them. +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] } + +// deleteArtifacts removes stores, segments, membership files, CSVs, derived +// subfolders and the attachments directory, but never a lock file. func (d *Dataset) deleteArtifacts() error { entries, err := os.ReadDir(d.Dir) if err != nil { @@ -299,7 +381,7 @@ func (d *Dataset) deleteArtifacts() error { continue } if e.IsDir() { - if name == "segments" || name == "attachments" { + if name == "segments" || name == "attachments" || IsDerivedSubdir(name) { if err := os.RemoveAll(filepath.Join(d.Dir, name)); err != nil { return err } diff --git a/internal/dataset/dataset_test.go b/internal/dataset/dataset_test.go index dab2af2..56c7684 100644 --- a/internal/dataset/dataset_test.go +++ b/internal/dataset/dataset_test.go @@ -226,3 +226,102 @@ func TestAutoName(t *testing.T) { t.Fatalf("counter name = %q", name2) } } + +func TestPurgeRemovesDerivedSubdir(t *testing.T) { + d := newDataset(t) + os.MkdirAll(d.Path(MaterializedDir), 0o700) + os.WriteFile(d.Path(MaterializedDir+"/answers.parquet"), []byte("PAR1"), 0o600) + + if err := d.Purge(); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(d.Path(MaterializedDir)); !os.IsNotExist(err) { + t.Fatalf("purge should delete %s, stat err = %v", MaterializedDir, err) + } +} + +// TestDeleteRemovesDerivedSubdir passes without production code, because Delete +// removes the whole dataset directory. It goes red if Delete is ever rewritten to +// remove known children selectively. +func TestDeleteRemovesDerivedSubdir(t *testing.T) { + d := newDataset(t) + os.MkdirAll(d.Path(MaterializedDir), 0o700) + os.WriteFile(d.Path(MaterializedDir+"/answers.parquet"), []byte("PAR1"), 0o600) + + if err := d.Delete(); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(d.Path(MaterializedDir)); !os.IsNotExist(err) { + t.Fatalf("delete should remove %s, stat err = %v", MaterializedDir, err) + } +} + +func TestPurgeClearsMaterialized(t *testing.T) { + d := newDataset(t) + m, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + m.Materialized = map[string]Materialized{ + "answers": {File: MaterializedDir + "/answers.parquet", Inputs: map[string]string{"answers.v1.jsonl": "3-7"}}, + } + if err := writeManifestFile(d.Dir, m); err != nil { + t.Fatal(err) + } + + if err := d.Purge(); err != nil { + t.Fatal(err) + } + after, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + if len(after.Materialized) != 0 { + t.Fatalf("purge left materialization entries naming files it deleted: %+v", after.Materialized) + } +} + +// TestRenameDeletePurgeRefuseDuringAMaterializeRun pins the coupling that keeps +// the lifecycle operations off a directory with an open handle inside it, which +// Windows refuses to move or remove, and which would otherwise leave the +// manifest renamed and the folder not. +func TestRenameDeletePurgeRefuseDuringAMaterializeRun(t *testing.T) { + root := t.TempDir() + ref := Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"} + d, err := Create(root, ref, "") + if err != nil { + t.Fatal(err) + } + release, err := d.LockMaterialize() + if err != nil { + t.Fatal(err) + } + + if _, err := d.Rename(root, "moved"); !errors.Is(err, ErrBusy) { + release() + t.Fatalf("rename during a materialize run should report busy, got %v", err) + } + if err := d.Delete(); !errors.Is(err, ErrBusy) { + release() + t.Fatalf("delete during a materialize run should report busy, got %v", err) + } + if err := d.Purge(); !errors.Is(err, ErrBusy) { + release() + t.Fatalf("purge during a materialize run should report busy, got %v", err) + } + // The manifest must not have been rewritten by the refused rename. + m, err := d.ReadManifest() + if err != nil { + release() + t.Fatal(err) + } + if m.Name != "ds" { + release() + t.Fatalf("a refused rename must leave the manifest name alone, got %q", m.Name) + } + + release() + if _, err := d.Rename(root, "moved"); err != nil { + t.Fatalf("rename should succeed once the run finishes: %v", err) + } +} diff --git a/internal/dataset/fingerprint.go b/internal/dataset/fingerprint.go new file mode 100644 index 0000000..b0a1b06 --- /dev/null +++ b/internal/dataset/fingerprint.go @@ -0,0 +1,72 @@ +package dataset + +import ( + "fmt" + "os" + "path/filepath" +) + +// FingerprintAbsent is recorded for a declared input that is not on disk. It +// cannot collide with a real fingerprint, which is always "-". A +// union view keeps declaring a CSV that has gone missing, so this is a normal +// state rather than an error, and recording it is what lets Fresh tell "absent +// then and absent now" from "absent then, present now". +const FingerprintAbsent = "absent" + +// 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. +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 each dataset-relative path, deduplicated, in +// the form Materialized.Inputs records and Fresh compares against. +func FingerprintInputs(dir string, files []string) map[string]string { + out := make(map[string]string, len(files)) + for _, rel := range files { + if _, seen := out[rel]; seen { + continue + } + out[rel] = Fingerprint(filepath.Join(dir, rel)) + } + return out +} + +// Fresh reports whether the Parquet may be read: it was built from the view +// definition the caller now has, the view's current inputs are exactly the +// recorded ones, and each still fingerprints the same. The caller supplies the +// current list and signature because only the engine knows either. +// +// 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. +// +// The signature is what the input fingerprints cannot see. A cc-data release +// that changes a view's SQL, or a reindex that re-stamps a zero FetchedAt and so +// reorders a dimension view's dedupe, both leave every input byte untouched +// while changing what the view returns. +func (mat Materialized) Fresh(dir string, current []string, signature string) bool { + if mat.Signature != signature { + return false + } + seen := make(map[string]bool, len(current)) + for _, rel := range current { + if seen[rel] { + continue + } + seen[rel] = true + recorded, ok := mat.Inputs[rel] + if !ok || Fingerprint(filepath.Join(dir, rel)) != recorded { + return false + } + } + return len(seen) == len(mat.Inputs) +} diff --git a/internal/dataset/fingerprint_test.go b/internal/dataset/fingerprint_test.go new file mode 100644 index 0000000..e372e2a --- /dev/null +++ b/internal/dataset/fingerprint_test.go @@ -0,0 +1,115 @@ +package dataset + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func writeInput(t *testing.T, dir, rel, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, rel), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestFingerprintChangesWithContent(t *testing.T) { + dir := t.TempDir() + writeInput(t, dir, "a.csv", "one") + first := Fingerprint(filepath.Join(dir, "a.csv")) + + writeInput(t, dir, "a.csv", "one more") + if got := Fingerprint(filepath.Join(dir, "a.csv")); got == first { + t.Fatalf("fingerprint unchanged after a rewrite: %s", got) + } + + // A same-size rewrite has to move the fingerprint too, which is what mtime + // is in it for. + writeInput(t, dir, "b.csv", "same") + before := Fingerprint(filepath.Join(dir, "b.csv")) + later := time.Now().Add(3 * time.Second) + if err := os.Chtimes(filepath.Join(dir, "b.csv"), later, later); err != nil { + t.Fatal(err) + } + if got := Fingerprint(filepath.Join(dir, "b.csv")); got == before { + t.Fatalf("fingerprint unchanged after an mtime change: %s", got) + } +} + +func TestFingerprintAbsentForMissingFile(t *testing.T) { + if got := Fingerprint(filepath.Join(t.TempDir(), "nope.csv")); got != FingerprintAbsent { + t.Fatalf("Fingerprint of a missing file = %q, want %q", got, FingerprintAbsent) + } +} + +func TestFreshTracksItsInputs(t *testing.T) { + dir := t.TempDir() + writeInput(t, dir, "one.csv", "a") + writeInput(t, dir, "two.csv", "b") + inputs := []string{"one.csv", "two.csv"} + const sig = "view-signature" + mat := Materialized{File: "materialized/reports.parquet", Inputs: FingerprintInputs(dir, inputs), Signature: sig} + + if !mat.Fresh(dir, inputs, sig) { + t.Fatal("Fresh should hold while every input is unchanged") + } + + writeInput(t, dir, "two.csv", "bb") + if mat.Fresh(dir, inputs, sig) { + t.Fatal("Fresh should fail after an input is rewritten") + } + + writeInput(t, dir, "two.csv", "b") + mat.Inputs = FingerprintInputs(dir, inputs) + if err := os.Remove(filepath.Join(dir, "two.csv")); err != nil { + t.Fatal(err) + } + if mat.Fresh(dir, inputs, sig) { + t.Fatal("Fresh should fail once a recorded input is deleted") + } + + writeInput(t, dir, "two.csv", "b") + mat.Inputs = FingerprintInputs(dir, inputs) + writeInput(t, dir, "three.csv", "c") + if mat.Fresh(dir, append(inputs, "three.csv"), sig) { + t.Fatal("Fresh should fail once the view declares an input the Parquet was not built from") + } + if !mat.Fresh(dir, inputs, sig) { + t.Fatal("Fresh should still hold for the unchanged input set") + } + + // The other direction: the view stops declaring an input the Parquet was + // built from, which leaves that run's rows in the Parquet and nowhere else. + if mat.Fresh(dir, inputs[:1], sig) { + t.Fatal("Fresh should fail once the view stops declaring a recorded input") + } + + // And the axis the fingerprints cannot see: the view's own definition moved, + // which a cc-data upgrade does without touching an input byte. + if mat.Fresh(dir, inputs, "a-different-view-definition") { + t.Fatal("Fresh should fail once the view definition the Parquet was built from has changed") + } +} + +// TestFreshRoundTripsAnAbsentInput is why the sentinel exists: a view keeps +// declaring a CSV that is not on disk, so "absent then and absent now" has to +// read as fresh while "absent then, present now" reads as stale. +func TestFreshRoundTripsAnAbsentInput(t *testing.T) { + dir := t.TempDir() + writeInput(t, dir, "one.csv", "a") + inputs := []string{"one.csv", "two.csv"} + const sig = "view-signature" + mat := Materialized{File: "materialized/reports.parquet", Inputs: FingerprintInputs(dir, inputs), Signature: sig} + if mat.Inputs["two.csv"] != FingerprintAbsent { + t.Fatalf("missing input recorded as %q, want %q", mat.Inputs["two.csv"], FingerprintAbsent) + } + if !mat.Fresh(dir, inputs, sig) { + t.Fatal("Fresh should hold while the missing input is still missing") + } + + writeInput(t, dir, "two.csv", "b") + if mat.Fresh(dir, inputs, sig) { + t.Fatal("Fresh should fail once the missing input is restored") + } +} diff --git a/internal/dataset/manifest.go b/internal/dataset/manifest.go index fa73b76..38a3817 100644 --- a/internal/dataset/manifest.go +++ b/internal/dataset/manifest.go @@ -31,6 +31,22 @@ type Manifest struct { Membership map[string]MembershipRef `json:"membership"` Downloads []Download `json:"downloads"` Attachments []AttachmentFile `json:"attachments"` + + // Materialized is keyed by view name. + Materialized map[string]Materialized `json:"materialized,omitempty"` +} + +// 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 + Inputs map[string]string `json:"inputs"` // dataset-relative path -> fingerprint + // Signature identifies the view definition the Parquet was built from, so a + // release that changes a view's projection, derived columns or ordering does + // not leave an older copy looking current over unchanged input files. + Signature string `json:"signature,omitempty"` + BuiltAt time.Time `json:"built_at"` } type Store struct { @@ -147,6 +163,9 @@ func ensureMaps(m *Manifest) { if m.Membership == nil { m.Membership = map[string]MembershipRef{} } + if m.Materialized == nil { + m.Materialized = map[string]Materialized{} + } } // writeManifestFile writes the manifest atomically; callers must hold the diff --git a/internal/dataset/reindex.go b/internal/dataset/reindex.go index 8be872b..8a76efa 100644 --- a/internal/dataset/reindex.go +++ b/internal/dataset/reindex.go @@ -23,7 +23,7 @@ var reindexCSVRe = regexp.MustCompile(`^report_(\d+)(?:_job_(\d+))?\.csv$`) // from membership files and CSVs (recovering report_type from CSV shape only // partially), rebuild the attachment index, and GC unreferenced files. func (d *Dataset) Reindex() error { - release, err := d.lockMutation() + release, err := d.LockMutation() if err != nil { return err } diff --git a/internal/dataset/reindex_test.go b/internal/dataset/reindex_test.go index a97d41a..168b8a0 100644 --- a/internal/dataset/reindex_test.go +++ b/internal/dataset/reindex_test.go @@ -411,3 +411,47 @@ func TestRecoverReportTypeDoesNotGuessLogFromAMissingStudentID(t *testing.T) { }) } } + +// TestDerivedSubdirIsInvisible holds the contract that reindex neither adopts +// files inside a derived subfolder nor reports them as orphans. Each planted +// name would be adopted at the top level, so the test fails the moment either +// walk starts descending into subdirectories. +func TestDerivedSubdirIsInvisible(t *testing.T) { + d := newDataset(t) + planted := []string{"answers.v99.jsonl", "report_4242.csv", "members_answers_5.v9.jsonl"} + os.MkdirAll(d.Path(MaterializedDir), 0o700) + for _, name := range planted { + os.WriteFile(d.Path(MaterializedDir+"/"+name), []byte("{}\n"), 0o600) + } + + if err := d.Reindex(); err != nil { + t.Fatal(err) + } + m, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + if len(m.Stores) != 0 || len(m.Membership) != 0 || len(m.Downloads) != 0 { + t.Fatalf("reindex adopted files from %s: %+v", MaterializedDir, m) + } + + // The same name at the top level is the control: it must be the only orphan + // reported, which proves the check fires at all and that the copy inside the + // derived subfolder is not what fired it. + os.WriteFile(d.Path("report_4242.csv"), []byte("a\n"), 0o600) + s := showJSON(t, d) + var orphans []string + for _, w := range s.Warnings { + if strings.HasPrefix(w, "ORPHAN_FILE") { + orphans = append(orphans, w) + } + } + if len(orphans) != 1 || !strings.Contains(orphans[0], "report_4242.csv") { + t.Fatalf("want exactly the top-level report_4242.csv orphan, got %v", orphans) + } + for _, name := range planted { + if _, err := os.Stat(d.Path(MaterializedDir + "/" + name)); err != nil { + t.Fatalf("reindex must leave %s in place: %v", name, err) + } + } +} diff --git a/internal/dataset/summary.go b/internal/dataset/summary.go index 55f57bb..c351051 100644 --- a/internal/dataset/summary.go +++ b/internal/dataset/summary.go @@ -6,6 +6,7 @@ import ( "regexp" "sort" "strconv" + "strings" "time" "github.com/concord-consortium/cc-data-cli/internal/store" @@ -20,8 +21,11 @@ type ShowJSON struct { CreatedAt time.Time `json:"created_at"` Totals map[string]int `json:"totals"` SizeBytes int64 `json:"size_bytes"` - Downloads []DownloadJSON `json:"downloads"` - Warnings []string `json:"warnings"` + // MaterializedBytes is the subset of SizeBytes under derived subfolders, + // which a researcher can reclaim for free. SizeBytes keeps its meaning. + MaterializedBytes int64 `json:"materialized_bytes"` + Downloads []DownloadJSON `json:"downloads"` + Warnings []string `json:"warnings"` } // DownloadJSON is one download in ShowJSON. @@ -55,25 +59,26 @@ type ListRowJSON struct { AgeSeconds int64 `json:"age_seconds"` Totals map[string]int `json:"totals"` SizeBytes int64 `json:"size_bytes"` + // MaterializedBytes carries the same meaning it has in ShowJSON. + MaterializedBytes int64 `json:"materialized_bytes"` } -// BuildShowJSON computes the dataset summary from the manifest only (no data-file -// scan beyond size/drift stats, no DuckDB). full adds per-file detail. -func (d *Dataset) BuildShowJSON(full bool) (*ShowJSON, error) { - m, err := d.ReadManifest() - if err != nil { - return nil, err - } - size := dirSize(d.Dir) +// BuildShowJSON computes the dataset summary from one manifest read (no data-file +// scan beyond size/drift stats, no DuckDB). full adds per-file detail. The +// caller passes the manifest so the warnings only the view set can decide are +// added from the same read; see duck.ShowJSON. +func (d *Dataset) BuildShowJSON(m *Manifest, full bool) *ShowJSON { + size, derived := dirSizes(d.Dir) out := &ShowJSON{ - Ref: d.Ref.String(), - Name: m.Name, - Description: m.Description, - Portal: d.Ref.Portal.Host(), - CreatedAt: m.CreatedAt, - Totals: manifestTotals(m), - SizeBytes: size, - Warnings: driftWarnings(d, m), + Ref: d.Ref.String(), + Name: m.Name, + Description: m.Description, + Portal: d.Ref.Portal.Host(), + CreatedAt: m.CreatedAt, + Totals: manifestTotals(m), + SizeBytes: size, + MaterializedBytes: derived, + Warnings: driftWarnings(d, m), } for _, dl := range m.Downloads { dj := DownloadJSON{ @@ -101,7 +106,7 @@ func (d *Dataset) BuildShowJSON(full bool) (*ShowJSON, error) { if out.Warnings == nil { out.Warnings = []string{} } - return out, nil + return out } // BuildListJSON enumerates every dataset across all portal folders under a data @@ -134,14 +139,16 @@ func BuildListJSON(dataRoot string) (*ListJSON, error) { if err != nil { continue } + size, derived := dirSizes(dir) out.Datasets = append(out.Datasets, ListRowJSON{ - Ref: decodePortalFolder(p.Name()) + "/" + m.Name, - Name: m.Name, - Description: m.Description, - Portal: decodePortalFolder(p.Name()), - AgeSeconds: int64(now.Sub(m.CreatedAt).Seconds()), - Totals: manifestTotals(m), - SizeBytes: dirSize(dir), + Ref: decodePortalFolder(p.Name()) + "/" + m.Name, + Name: m.Name, + Description: m.Description, + Portal: decodePortalFolder(p.Name()), + AgeSeconds: int64(now.Sub(m.CreatedAt).Seconds()), + Totals: manifestTotals(m), + SizeBytes: size, + MaterializedBytes: derived, }) } } @@ -220,6 +227,21 @@ func driftWarnings(d *Dataset, m *Manifest) []string { warnings = append(warnings, "UNKNOWN_TYPE: run "+itoa(dl.RunID)+" report_type "+dl.ReportType+" is unknown to this cc-data version and excluded from the reports view; upgrade suggested") } } + // A recorded Parquet that is gone. The folder is documented as safe to + // delete, and this is the one place that says which views that affected. + for view, mat := range m.Materialized { + if !fileOnDisk(d.Path(mat.File)) { + warnings = append(warnings, "MISSING_MATERIALIZED: view "+view+" records "+mat.File+" but it is missing on disk; queries are reading the raw artifacts, run cc-data dataset materialize "+d.Ref.String()+" to rebuild it") + } + } + // A Parquet in a derived subfolder that no manifest entry names: the state a + // reindex leaves, or a file something else put there. Deciding it needs no + // view knowledge, only the folder listing against Materialized. Materialize + // overwrites a view's own file and never removes one it does not recognize, + // so the file's fate is the researcher's call. + for _, name := range unreferencedMaterialized(d, m) { + warnings = append(warnings, "UNREFERENCED_MATERIALIZED: "+name+" is on disk but the manifest does not record it, so cc-data does not read it; if it was a view's copy, run cc-data dataset materialize "+d.Ref.String()+" to record a current one; the file can be kept, moved or deleted") + } // Orphan final-named files not referenced by the manifest. referenced := manifestReferencedFiles(m) entries, _ := os.ReadDir(d.Dir) @@ -236,6 +258,26 @@ func driftWarnings(d *Dataset, m *Manifest) []string { return warnings } +func unreferencedMaterialized(d *Dataset, m *Manifest) []string { + entries, err := os.ReadDir(filepath.Join(d.Dir, MaterializedDir)) + if err != nil { + return nil + } + recorded := map[string]bool{} + for _, mat := range m.Materialized { + recorded[filepath.Base(mat.File)] = true + } + var out []string + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".parquet") || recorded[name] { + continue + } + out = append(out, MaterializedDir+"/"+name) + } + return out +} + func manifestReferencedFiles(m *Manifest) map[string]bool { ref := map[string]bool{} for _, st := range m.Stores { @@ -259,18 +301,27 @@ func fileOnDisk(path string) bool { return err == nil } -func dirSize(dir string) int64 { - var total int64 +// dirSizes returns the dataset's total bytes and the subset under registered +// derived subfolders, in one walk so dataset list does not pay a second per +// dataset. +func dirSizes(dir string) (total, derived int64) { filepath.WalkDir(dir, func(path string, de os.DirEntry, err error) error { if err != nil || de.IsDir() { return nil } - if info, e := de.Info(); e == nil { - total += info.Size() + info, e := de.Info() + if e != nil { + return nil + } + total += info.Size() + if rel, rerr := filepath.Rel(dir, path); rerr == nil { + if top := strings.Split(filepath.ToSlash(rel), "/")[0]; IsDerivedSubdir(top) { + derived += info.Size() + } } return nil }) - return total + return total, derived } func itoa(n int) string { return strconv.Itoa(n) } diff --git a/internal/dataset/summary_test.go b/internal/dataset/summary_test.go index 01a4184..e1e5e7f 100644 --- a/internal/dataset/summary_test.go +++ b/internal/dataset/summary_test.go @@ -1,7 +1,10 @@ package dataset import ( + "bytes" "os" + "path/filepath" + "sort" "strings" "testing" @@ -13,10 +16,7 @@ func TestShowJSONSchema(t *testing.T) { seg := writeFinishedSegment(t, d, "answers", 584, [][]byte{rec("s", "e1", "q1", "", "A")}) d.MergeCompact("answers", 584, seg) - s, err := d.BuildShowJSON(false) - if err != nil { - t.Fatal(err) - } + s := showJSON(t, d) if s.Ref != "learn.concord.org/ds" || s.Portal != "learn.concord.org" { t.Fatalf("show ref/portal wrong: %+v", s) } @@ -38,7 +38,7 @@ func TestShowWarnsOnOrphanFile(t *testing.T) { // Plant an orphan store version not in the manifest. os.WriteFile(d.Path("answers.v9.jsonl"), []byte("{}\n"), 0o600) - s, _ := d.BuildShowJSON(false) + s := showJSON(t, d) found := false for _, w := range s.Warnings { if strings.HasPrefix(w, "ORPHAN_FILE:") { @@ -57,7 +57,7 @@ func TestPurgeThenShowZeroHoldings(t *testing.T) { if err := d.Purge(); err != nil { t.Fatal(err) } - s, _ := d.BuildShowJSON(false) + s := showJSON(t, d) if s.Totals["answers"] != 0 || len(s.Downloads) != 0 { t.Fatalf("purge should leave zero holdings: %+v", s) } @@ -114,7 +114,7 @@ func TestShowWarnsWhatARecoveredDownloadLost(t *testing.T) { t.Fatal(err) } - s, _ := d.BuildShowJSON(false) + s := showJSON(t, d) got := warningsWithPrefix(s.Warnings, "RECOVERED_PROVENANCE:") if len(got) != 1 { t.Fatalf("expected one recovered-provenance warning, got %v", s.Warnings) @@ -136,8 +136,116 @@ func TestShowDoesNotWarnForARecoveredStoreDownload(t *testing.T) { t.Fatal(err) } - s, _ := d.BuildShowJSON(false) + s := showJSON(t, d) if got := warningsWithPrefix(s.Warnings, "RECOVERED_PROVENANCE:"); len(got) != 0 { t.Fatalf("a store download raised a provenance warning: %v", got) } } + +func TestMaterializedBytesIsReportedSeparately(t *testing.T) { + d := newDataset(t) + os.WriteFile(d.Path("answers.v1.jsonl"), []byte("{}\n"), 0o600) + os.MkdirAll(d.Path(MaterializedDir), 0o700) + body := bytes.Repeat([]byte("x"), 500) + os.WriteFile(d.Path(MaterializedDir+"/answers.parquet"), body, 0o600) + + s := showJSON(t, d) + if s.MaterializedBytes != int64(len(body)) { + t.Fatalf("materialized_bytes = %d, want %d", s.MaterializedBytes, len(body)) + } + if s.SizeBytes <= s.MaterializedBytes { + t.Fatalf("size_bytes (%d) must still count everything, including the %d derived bytes", + s.SizeBytes, s.MaterializedBytes) + } + + list, err := BuildListJSON(filepath.Dir(filepath.Dir(filepath.Dir(d.Dir)))) + if err != nil { + t.Fatal(err) + } + if len(list.Datasets) != 1 { + t.Fatalf("want one dataset listed, got %d", len(list.Datasets)) + } + if list.Datasets[0].MaterializedBytes != int64(len(body)) { + t.Fatalf("list materialized_bytes = %d, want %d", list.Datasets[0].MaterializedBytes, len(body)) + } +} + +// showJSON builds the summary the way both surfaces do, from one manifest read. +func showJSON(t *testing.T, d *Dataset) *ShowJSON { + t.Helper() + m, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + return d.BuildShowJSON(m, false) +} + +func TestUnreferencedMaterializedWarning(t *testing.T) { + d := newDataset(t) + os.MkdirAll(d.Path(MaterializedDir), 0o700) + os.WriteFile(d.Path(MaterializedDir+"/logs.parquet"), []byte("PAR1"), 0o600) + os.WriteFile(d.Path(MaterializedDir+"/answers.parquet"), []byte("PAR1"), 0o600) + + m, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + m.Materialized["answers"] = Materialized{File: MaterializedDir + "/answers.parquet"} + if err := writeManifestFile(d.Dir, m); err != nil { + t.Fatal(err) + } + + s := showJSON(t, d) + var named []string + for _, w := range s.Warnings { + if strings.HasPrefix(w, "UNREFERENCED_MATERIALIZED") { + named = append(named, w) + } + } + if len(named) != 1 || !strings.Contains(named[0], "logs.parquet") { + t.Fatalf("want exactly the unrecorded logs.parquet named, got %v", named) + } + if !strings.Contains(named[0], "dataset materialize") { + t.Fatalf("the warning must name the command that re-records a view's copy: %s", named[0]) + } + // Materialize never removes a file it does not recognize, so the warning + // must not promise that it clears the condition. + if !strings.Contains(named[0], "kept, moved or deleted") { + t.Fatalf("the warning must leave the file's fate to the researcher: %s", named[0]) + } + if !sort.StringsAreSorted(s.Warnings) { + t.Fatalf("warnings must stay sorted: %v", s.Warnings) + } +} + +// TestMissingMaterializedWarning covers the documented action, deleting the +// folder: the entries stay recorded, so nothing else in show would name the +// views now reading their raw artifacts. +func TestMissingMaterializedWarning(t *testing.T) { + d := newDataset(t) + m, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + m.Materialized["answers"] = Materialized{File: MaterializedDir + "/answers.parquet"} + m.Materialized["logs"] = Materialized{File: MaterializedDir + "/logs.parquet"} + if err := writeManifestFile(d.Dir, m); err != nil { + t.Fatal(err) + } + os.MkdirAll(d.Path(MaterializedDir), 0o700) + os.WriteFile(d.Path(MaterializedDir+"/logs.parquet"), []byte("PAR1"), 0o600) + + s := showJSON(t, d) + var named []string + for _, w := range s.Warnings { + if strings.HasPrefix(w, "MISSING_MATERIALIZED") { + named = append(named, w) + } + } + if len(named) != 1 || !strings.Contains(named[0], "view answers") { + t.Fatalf("want exactly the missing answers copy named, got %v", named) + } + if !strings.Contains(named[0], "dataset materialize") { + t.Fatalf("the warning must name the command that rebuilds it: %s", named[0]) + } +} diff --git a/internal/duck/dimension_views_test.go b/internal/duck/dimension_views_test.go index e8fea40..178ff08 100644 --- a/internal/duck/dimension_views_test.go +++ b/internal/duck/dimension_views_test.go @@ -472,7 +472,7 @@ func TestDimensionViewsAreEmptyAfterAManifestLessReindex(t *testing.T) { t.Fatalf("the CSV itself was lost, not just its provenance: %d rows", n) } - s, err := d.BuildShowJSON(false) + s, err := ShowJSON(d, false) if err != nil { t.Fatal(err) } diff --git a/internal/duck/engine.go b/internal/duck/engine.go index 1775fcb..423345c 100644 --- a/internal/duck/engine.go +++ b/internal/duck/engine.go @@ -7,6 +7,7 @@ import ( "database/sql" "fmt" "io" + "os" "path/filepath" "strings" @@ -24,48 +25,73 @@ type DatasetSpec struct { type Engine struct { db *sql.DB conn *sql.Conn + warn io.Writer + // pinned holds the views registered from a Parquet, so a later query can + // notice the copy going stale and put the view back on its raw artifacts. + pinned []pinnedView +} + +// pinnedView is a view reading its Parquet, with the directory its freshness +// fingerprints resolve against. +type pinnedView struct { + stmt viewStmt + dir string } // Open registers the datasets' views and locks the sandbox to their folders plus // any allow-dirs. Warnings from degraded views are written to warnOut. func Open(ctx context.Context, datasets []DatasetSpec, allowDirs []string, warnOut io.Writer) (*Engine, error) { + e, _, err := open(ctx, datasets, allowDirs, warnOut, true) + return e, err +} + +// OpenRaw registers every view from its raw artifacts, ignoring any Parquet the +// manifest records, and reports the views that fell back to their typed-empty +// statement. Materialization reads through it so a stale Parquet can never be +// copied forward into a fresh-looking one. +func OpenRaw(ctx context.Context, d *dataset.Dataset, warnOut io.Writer) (*Engine, map[string]bool, error) { + return open(ctx, []DatasetSpec{{DS: d}}, nil, warnOut, false) +} + +func open(ctx context.Context, datasets []DatasetSpec, allowDirs []string, warnOut io.Writer, useMaterialized bool) (*Engine, map[string]bool, error) { + degraded := map[string]bool{} if warnOut == nil { warnOut = io.Discard } if len(datasets) == 0 { - return nil, fmt.Errorf("no datasets given") + return nil, nil, fmt.Errorf("no datasets given") } schemas, err := resolveSchemas(datasets) if err != nil { - return nil, err + return nil, nil, err } var allowed []string for _, ds := range datasets { canon, err := canonicalize(ds.DS.Dir) if err != nil { - return nil, err + return nil, nil, err } allowed = append(allowed, canon) } for _, dir := range allowDirs { canon, err := canonicalize(dir) if err != nil { - return nil, fmt.Errorf("--allow-dir %q: %w", dir, err) + return nil, nil, fmt.Errorf("--allow-dir %q: %w", dir, err) } allowed = append(allowed, canon) } db, err := sql.Open("duckdb", "") if err != nil { - return nil, err + return nil, nil, err } conn, err := db.Conn(ctx) if err != nil { db.Close() - return nil, err + return nil, nil, err } - e := &Engine{db: db, conn: conn} + e := &Engine{db: db, conn: conn, warn: warnOut} multi := len(datasets) > 1 for i, ds := range datasets { @@ -74,32 +100,94 @@ func Open(ctx context.Context, datasets []DatasetSpec, allowDirs []string, warnO prefix = sqlIdent(schemas[i]) + "." if _, err := conn.ExecContext(ctx, "CREATE SCHEMA "+sqlIdent(schemas[i])); err != nil { e.Close() - return nil, fmt.Errorf("creating schema %q: %w", schemas[i], err) + return nil, nil, fmt.Errorf("creating schema %q: %w", schemas[i], err) } } m, err := ds.DS.ReadManifest() if err != nil { e.Close() - return nil, err + return nil, nil, err } canon, _ := canonicalize(ds.DS.Dir) vs := viewSet{prefix: prefix, canonDir: canon, m: m, warn: warnOut} - for _, stmt := range vs.statements() { - if _, err := conn.ExecContext(ctx, stmt.primary); err != nil { - if _, ferr := conn.ExecContext(ctx, stmt.fallback); ferr != nil { - e.Close() - return nil, fmt.Errorf("registering view %s: %v (fallback also failed: %v)", stmt.name, err, ferr) - } - fmt.Fprintf(warnOut, "warning: view %s degraded to empty (%v); affected files: %s\n", stmt.name, err, strings.Join(stmt.files, ", ")) + stmts := vs.statements() + if useMaterialized { + stmts = vs.applyMaterialized(stmts) + } + for _, stmt := range stmts { + pinned, deg, err := e.register(ctx, stmt) + if err != nil { + e.Close() + return nil, nil, err + } + if pinned { + e.pinned = append(e.pinned, pinnedView{stmt: stmt, dir: canon}) + } + if deg { + degraded[bareViewName(stmt.name, prefix)] = true } } } if err := e.lockSandbox(ctx, allowed); err != nil { e.Close() - return nil, err + return nil, nil, err } - return e, nil + return e, degraded, nil +} + +// register creates one view, trying its forms in order: the Parquet when one is +// set, then the raw artifacts, then the typed-empty fallback. It reports whether +// the view ended pinned to its Parquet and whether it degraded to empty. A +// missing Parquet gets one short line; only a present but unreadable one +// carries the engine's error, since that is the case with something to explain. +func (e *Engine) register(ctx context.Context, st viewStmt) (pinned, degraded bool, err error) { + if st.materialized != "" { + if _, serr := os.Stat(st.parquet); serr != nil { + fmt.Fprintf(e.warn, "warning: materialized %s is missing; reading the raw artifacts instead\n", st.name) + } else if merr := e.exec(ctx, st.materialized); merr == nil { + return true, false, nil + } else { + fmt.Fprintf(e.warn, "warning: materialized %s is unreadable (%v); reading the raw artifacts instead\n", st.name, merr) + } + } + if perr := e.exec(ctx, st.primary); perr != nil { + if ferr := e.exec(ctx, st.fallback); ferr != nil { + return false, false, fmt.Errorf("registering view %s: %v (fallback also failed: %v)", st.name, perr, ferr) + } + fmt.Fprintf(e.warn, "warning: view %s degraded to empty (%v); affected files: %s\n", st.name, perr, strings.Join(st.files, ", ")) + return false, true, nil + } + return false, false, nil +} + +// refreshPinned re-registers every pinned view whose Parquet has gone stale +// from its raw artifacts. The raw statements read their files on every query, +// so without this a session that outlives a fetch would keep answering from a +// copy of the data as it stood when the session opened. +func (e *Engine) refreshPinned(ctx context.Context) error { + if len(e.pinned) == 0 { + return nil + } + var kept []pinnedView + for i, p := range e.pinned { + if p.stmt.record.Fresh(p.dir, p.stmt.files, p.stmt.signature) { + kept = append(kept, p) + continue + } + raw := p.stmt + raw.materialized = "" + err := e.exec(ctx, "DROP VIEW "+raw.name) + if err == nil { + _, _, err = e.register(ctx, raw) + } + if err != nil { + e.pinned = append(kept, e.pinned[i+1:]...) + return err + } + } + e.pinned = kept + return nil } // Resource caps applied before lock_configuration=true. DuckDB otherwise @@ -137,8 +225,19 @@ func (e *Engine) lockSandbox(ctx context.Context, allowed []string) error { return nil } -// Query runs user SQL on the pinned connection. +// exec runs a statement that returns no rows. Query cannot serve here: dropping +// its *sql.Rows leaks the connection and deadlocks Close. +func (e *Engine) exec(ctx context.Context, stmt string) error { + _, err := e.conn.ExecContext(ctx, stmt) + return err +} + +// Query runs user SQL on the pinned connection, after putting any view whose +// Parquet has gone stale back on its raw artifacts. func (e *Engine) Query(ctx context.Context, query string) (*sql.Rows, error) { + if err := e.refreshPinned(ctx); err != nil { + return nil, err + } return e.conn.QueryContext(ctx, query) } diff --git a/internal/duck/materialize.go b/internal/duck/materialize.go new file mode 100644 index 0000000..ea68199 --- /dev/null +++ b/internal/duck/materialize.go @@ -0,0 +1,620 @@ +package duck + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "sort" + "strings" + "time" + + "github.com/concord-consortium/cc-data-cli/internal/dataset" + "github.com/concord-consortium/cc-data-cli/internal/fsutil" + "github.com/concord-consortium/cc-data-cli/internal/store" +) + +// MaterializeOptions controls one materialize run. +type MaterializeOptions struct { + Force bool // rebuild a view whose recorded inputs are unchanged + AllowPartial bool // build a view from what is on disk when a declared input is missing + Progress func(view string, i, n int) +} + +// MaterializeStatus is what became of one view in a run. A view has exactly one. +type MaterializeStatus string + +const ( + // StatusWritten: copied and now recorded in the manifest. + StatusWritten MaterializeStatus = "written" + // StatusFresh: the recorded Parquet still matches its inputs, so nothing was done. + StatusFresh MaterializeStatus = "fresh" + // StatusDiscarded: copied, then dropped because its inputs moved while the + // copy ran. Nothing is wrong with the dataset and running again picks it up. + StatusDiscarded MaterializeStatus = "discarded" + // StatusRefused: deliberately not materialized, for the reason it carries. + StatusRefused MaterializeStatus = "refused" +) + +// ViewOutcome is one view's result. Reason says why a view was refused or +// discarded; on a written or fresh view it carries a caveat when there is one, +// a copy built from fewer inputs than the view declares or one that reads a +// download marked incomplete, and is empty otherwise. The caveat travels in the +// outcome rather than as a progress message because the MCP tool returns the +// outcome and may have nowhere to send progress. +type ViewOutcome struct { + View string `json:"view"` + Status MaterializeStatus `json:"status"` + Reason string `json:"reason,omitempty"` +} + +// MaterializeResult carries one outcome per materializable view, in view order. +// +// One list rather than a list per status, so a view cannot land in two of them +// or none, and so a caller rendering the result cannot quietly stop reporting a +// status that is added later: the MCP tool returns this slice rather than a +// projection of it that someone has to remember to extend. +type MaterializeResult struct { + Views []ViewOutcome `json:"views"` +} + +// Names returns the views with one status, in view order. +func (r MaterializeResult) Names(status MaterializeStatus) []string { + var out []string + for _, o := range r.Views { + if o.Status == status { + out = append(out, o.View) + } + } + return out +} + +// Written, Fresh and Discarded name the views that ended with each status. +func (r MaterializeResult) Written() []string { return r.Names(StatusWritten) } +func (r MaterializeResult) Fresh() []string { return r.Names(StatusFresh) } +func (r MaterializeResult) Discarded() []string { return r.Names(StatusDiscarded) } + +// Refused maps each refused view to its reason. +func (r MaterializeResult) Refused() map[string]string { + out := map[string]string{} + for _, o := range r.Views { + if o.Status == StatusRefused { + out[o.View] = o.Reason + } + } + return out +} + +// Refusals reports whether any view was refused, which is what makes the command +// exit non-zero. +func (r MaterializeResult) Refusals() bool { return len(r.Names(StatusRefused)) > 0 } + +// Materialize writes each materializable view to a ZSTD Parquet under the +// dataset's materialized folder. A view that cannot be copied is refused like +// any other, so one failure costs its own view rather than the whole run; only +// a cancelled context stops the run outright. +// +// The run holds the dataset's materialize guard from start to finish, so two +// runs never work the same dataset at once. The mutation locks are a different +// matter: they are held only to read the manifest at the start and repoint it at +// the end, so a concurrent get does not fail as busy for the whole run. A view +// whose inputs moved in between is discarded rather than recorded, because its +// Parquet describes a state that is already gone. The repoint waits for a get +// that is still running, since the copies are worth more than the wait. +func Materialize(ctx context.Context, d *dataset.Dataset, opts MaterializeOptions, warnOut io.Writer) (MaterializeResult, error) { + if warnOut == nil { + warnOut = io.Discard + } + var res MaterializeResult + outcomes := map[string]ViewOutcome{} + record := func(view string, status MaterializeStatus, reason string) { + outcomes[view] = ViewOutcome{View: view, Status: status, Reason: reason} + } + + releaseRun, err := d.LockMaterialize() + if err != nil { + return res, err + } + defer releaseRun() + + m, err := readManifestLocked(d) + if err != nil { + return res, err + } + declared, err := declaredViews(d.Dir, m) + if err != nil { + return res, err + } + views := make([]string, 0, len(declared)) + filesByView := map[string][]string{} + for view, decl := range declared { + views = append(views, view) + filesByView[view] = decl.files + } + sort.Strings(views) + + if err := os.MkdirAll(d.Path(dataset.MaterializedDir), 0o700); err != nil { + return res, err + } + if err := sweepStaleTemps(d); err != nil { + return res, err + } + e, degraded, err := OpenRaw(ctx, d, warnOut) + if err != nil { + return res, err + } + defer e.Close() + + built := map[string]builtView{} + defer func() { + for _, b := range built { + removeCopyArtifacts(d, b.tmp) + } + }() + + for i, view := range views { + if opts.Progress != nil { + opts.Progress(view, i+1, len(views)) + } + files := filesByView[view] + if degraded[view] { + record(view, StatusRefused, "registered from its typed-empty fallback, so every declared input is unreadable: "+ + strings.Join(files, ", ")) + continue + } + missing := missingInputs(d.Dir, files) + if len(missing) > 0 && !opts.AllowPartial { + record(view, StatusRefused, missingInputReason(d, m, files, missing)) + continue + } + caveat := viewCaveat(m, view, files, missing) + if !opts.Force { + // The recorded copy has to still be readable, not merely recorded. The + // folder is documented as safe to delete at any time, and without this + // a deleted or truncated Parquet reads as fresh forever while queries + // quietly fall back to the raw artifacts. + if rec, ok := m.Materialized[view]; ok && rec.Fresh(d.Dir, files, declared[view].signature) && + parquetReadable(ctx, e, d.Path(rec.File)) { + record(view, StatusFresh, caveat) + continue + } + } + // Fingerprinted before the copy, so the re-check at the repoint can see an + // input that moved while the copy was running. + inputs := dataset.FingerprintInputs(d.Dir, files) + tmp, err := copyViewToParquet(ctx, e, d, view) + if err != nil { + if ctx.Err() != nil { + return res, err + } + record(view, StatusRefused, err.Error()) + continue + } + if st, isStore := m.Stores[view]; isStore { + n, err := parquetRowCount(ctx, e, d.Path(tmp)) + if err != nil { + removeCopyArtifacts(d, tmp) + if ctx.Err() != nil { + return res, err + } + record(view, StatusRefused, err.Error()) + continue + } + if n != st.Count { + removeCopyArtifacts(d, tmp) + record(view, StatusRefused, fmt.Sprintf("copied %d rows but the manifest records %d in the %s store", n, st.Count, view)) + continue + } + } + built[view] = builtView{tmp: tmp, inputs: inputs, signature: declared[view].signature, caveat: caveat} + } + + testHookBeforeRepoint() + if err := repointManifest(ctx, d, built, record, warnOut); err != nil { + return res, err + } + // Assembled from the view list rather than appended as we go, so the order is + // the view order and a view that somehow got no outcome is visible as absent + // rather than as an arbitrary default. + for _, view := range views { + if o, ok := outcomes[view]; ok { + res.Views = append(res.Views, o) + } + } + return res, nil +} + +// testHookBeforeRepoint is a seam for driving the window between the copies and +// the repoint, matching the hook the merge path uses. +var testHookBeforeRepoint = func() {} + +// builtView is one finished copy waiting to be committed: its temp file, the +// input fingerprints taken just before it was made, and the caveat its outcome +// will carry. +type builtView struct { + tmp string + inputs map[string]string + signature string + caveat string +} + +func readManifestLocked(d *dataset.Dataset) (*dataset.Manifest, error) { + release, err := d.LockMutation() + if err != nil { + return nil, err + } + defer release() + return d.ReadManifest() +} + +// repointManifest takes the locks back, keeps only the copies whose inputs still +// fingerprint as they did when the copy started, and renames those into place. +// The manifest write is the commit point, so a discarded copy leaves nothing +// behind and a committed one is already on disk under its final name. +// +// The locks are waited for rather than tried once. A fetch holds the activity +// lock shared for its whole run, and failing busy here would throw away every +// finished copy because someone started a get; only the caller's context ends +// the wait. A copy whose rename fails is refused on its own, like a copy that +// failed, so the views already renamed still reach the manifest. +func repointManifest(ctx context.Context, d *dataset.Dataset, built map[string]builtView, + record func(view string, status MaterializeStatus, reason string), warnOut io.Writer) error { + if len(built) == 0 { + return nil + } + release, err := d.LockMutation() + if errors.Is(err, dataset.ErrBusy) { + fmt.Fprintln(warnOut, "waiting for another cc-data command to finish before committing the copies (interrupt to abandon them)") + release, err = d.LockMutationWait(ctx) + } + if err != nil { + return err + } + defer release() + + m, err := d.ReadManifest() + if err != nil { + return err + } + current, err := declaredViews(d.Dir, m) + if err != nil { + return err + } + now := time.Now().UTC() + for view, b := range built { + decl, stillAView := current[view] + if !stillAView || decl.signature != b.signature || + !sameFingerprints(b.inputs, dataset.FingerprintInputs(d.Dir, decl.files)) { + record(view, StatusDiscarded, "its inputs or its view definition changed while the copy was running") + continue + } + rec := dataset.Materialized{ + File: filepath.ToSlash(filepath.Join(dataset.MaterializedDir, view+".parquet")), + Inputs: b.inputs, + Signature: b.signature, + BuiltAt: now, + } + if err := fsutil.RenameAtomic(d.Path(b.tmp), d.Path(rec.File)); err != nil { + record(view, StatusRefused, err.Error()) + continue + } + delete(built, view) + m.Materialized[view] = rec + record(view, StatusWritten, b.caveat) + } + return d.WriteManifest(m) +} + +// viewDecl is what a materializable view currently declares: the files it reads +// and the signature of the statement that reads them. +type viewDecl struct { + files []string + signature string +} + +// declaredViews maps each materializable view to what it currently declares. The +// directory is canonicalized here so the signatures match the ones the engine +// computes when it decides whether to read a Parquet. +func declaredViews(dir string, m *dataset.Manifest) (map[string]viewDecl, error) { + canon, err := canonicalize(dir) + if err != nil { + return nil, err + } + vs := viewSet{canonDir: canon, m: m} + stmts := vs.statements() + eligible := map[string]bool{} + for _, view := range materializableFrom(stmts, vs.prefix) { + eligible[view] = true + } + out := map[string]viewDecl{} + for _, st := range stmts { + if bare := bareViewName(st.name, vs.prefix); eligible[bare] { + out[bare] = viewDecl{files: st.files, signature: viewSignature(st, canon, vs.prefix)} + } + } + return out, nil +} + +func sameFingerprints(a, b map[string]string) bool { + if len(a) != len(b) { + return false + } + for k, v := range a { + if b[k] != v { + return false + } + } + return true +} + +// tempParquetInfix marks an in-flight copy. The writer and the sweep share it so +// they cannot disagree about which files are unfinished work. +const tempParquetInfix = ".parquet.tmp-" + +// sweepStaleTemps removes the temp files of a run that died before its own +// cleanup could run: a kill, a crash, or a power cut. The materialize guard is +// held, so a temp file present now belongs to no living run. +func sweepStaleTemps(d *dataset.Dataset) error { + entries, err := os.ReadDir(d.Path(dataset.MaterializedDir)) + if err != nil { + return err + } + for _, entry := range entries { + if entry.IsDir() || !strings.Contains(entry.Name(), tempParquetInfix) { + continue + } + if err := os.Remove(d.Path(filepath.Join(dataset.MaterializedDir, entry.Name()))); err != nil { + return err + } + } + return nil +} + +// copyViewToParquet writes the view to a temp name in the derived folder, unique +// per call so the name cannot collide with a concurrent copy of another view. +func copyViewToParquet(ctx context.Context, e *Engine, d *dataset.Dataset, view string) (string, error) { + f, err := os.CreateTemp(d.Path(dataset.MaterializedDir), view+tempParquetInfix+"*") + if err != nil { + return "", err + } + f.Close() + tmp := filepath.ToSlash(filepath.Join(dataset.MaterializedDir, filepath.Base(f.Name()))) + stmt := fmt.Sprintf("COPY (SELECT * FROM %s) TO %s (FORMAT parquet, COMPRESSION zstd, KV_METADATA {view: %s, built_at: %s})", + sqlIdent(view), sqlStr(d.Path(tmp)), sqlStr(view), sqlStr(time.Now().UTC().Format(time.RFC3339))) + if err := e.exec(ctx, stmt); err != nil { + removeCopyArtifacts(d, tmp) + return "", fmt.Errorf("materializing %s: %w", view, err) + } + return tmp, nil +} + +// removeCopyArtifacts removes a temp copy and anything left beside it. A COPY +// that fails partway leaves its own scratch file in the target's directory, +// named after the target, so removing the target alone leaks it. Matching on the +// temp name rather than on a fixed prefix keeps this independent of how the +// engine spells its scratch file. +func removeCopyArtifacts(d *dataset.Dataset, tmp string) { + os.Remove(d.Path(tmp)) + dir := d.Path(dataset.MaterializedDir) + base := filepath.Base(tmp) + entries, err := os.ReadDir(dir) + if err != nil { + return + } + for _, entry := range entries { + if !entry.IsDir() && entry.Name() != base && strings.Contains(entry.Name(), base) { + os.Remove(filepath.Join(dir, entry.Name())) + } + } +} + +// parquetReadable reports whether the engine can open the file and read its +// footer, which is what separates a recorded copy that is still usable from one +// that has been deleted or truncated. +func parquetReadable(ctx context.Context, e *Engine, path string) bool { + if _, err := os.Stat(path); err != nil { + return false + } + _, err := parquetRowCount(ctx, e, path) + return err == nil +} + +func parquetRowCount(ctx context.Context, e *Engine, path string) (int, error) { + rows, err := e.Query(ctx, fmt.Sprintf("SELECT count(*) FROM read_parquet(%s)", sqlStr(path))) + if err != nil { + return 0, err + } + defer rows.Close() + var n int + if rows.Next() { + if err := rows.Scan(&n); err != nil { + return 0, err + } + } + return n, rows.Err() +} + +func missingInputs(dir string, files []string) []string { + var missing []string + seen := map[string]bool{} + for _, rel := range files { + if seen[rel] { + continue + } + seen[rel] = true + if dataset.Fingerprint(filepath.Join(dir, rel)) == dataset.FingerprintAbsent { + missing = append(missing, rel) + } + } + sort.Strings(missing) + return missing +} + +// missingInputReason names both remedies, because the point of refusing is to +// surface them rather than to leave a flag to be typed on every run afterwards. +func missingInputReason(d *dataset.Dataset, m *dataset.Manifest, files, missing []string) string { + var b strings.Builder + fmt.Fprintf(&b, "declares %d inputs and %d %s missing: %s", len(files), len(missing), + plural(len(missing), "is", "are"), strings.Join(missing, ", ")) + if runs := runsForFiles(m, missing); len(runs) > 0 { + fmt.Fprintf(&b, "\n re-fetch %s, or run cc-data dataset reindex %s to drop the entry so the file stops being expected", + runList(runs), d.Ref.String()) + } else { + fmt.Fprintf(&b, "\n re-fetch the run, or run cc-data dataset reindex %s to drop the entry so the file stops being expected", + d.Ref.String()) + } + b.WriteString("\n to build from what is on disk anyway, pass --allow-partial") + return b.String() +} + +func runsForFiles(m *dataset.Manifest, files []string) []int { + want := map[string]bool{} + for _, f := range files { + want[f] = true + } + seen := map[int]bool{} + var runs []int + for _, dl := range m.Downloads { + for _, f := range dl.Files { + if want[f] && !seen[dl.RunID] { + seen[dl.RunID] = true + runs = append(runs, dl.RunID) + } + } + } + sort.Ints(runs) + return runs +} + +func runList(runs []int) string { + parts := make([]string, len(runs)) + for i, r := range runs { + parts[i] = fmt.Sprintf("%d", r) + } + return plural(len(runs), "run ", "runs ") + strings.Join(parts, ", ") +} + +func plural(n int, one, many string) string { + if n == 1 { + return one + } + return many +} + +// viewCaveat is what a written or fresh view's outcome has to say about the +// copy: how many of its declared inputs it was built from when some are missing, +// and which of the runs it reads are marked incomplete. A fresh copy built with +// an input missing records that input as absent, so the same test applies to it. +func viewCaveat(m *dataset.Manifest, view string, files, missing []string) string { + var parts []string + if len(missing) > 0 { + parts = append(parts, fmt.Sprintf("built from %d of %d declared inputs (missing: %s)", + len(files)-len(missing), len(files), strings.Join(missing, ", "))) + } + if runs := incompleteRuns(m, view, files); len(runs) > 0 { + parts = append(parts, fmt.Sprintf("reads %s marked incomplete", runList(runs))) + } + return strings.Join(parts, "; ") +} + +// incompleteRuns names the runs a view reads whose download is marked +// incomplete. It is a caveat rather than a refusal: only a store fetch can set +// the flag, and it 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. +// +// The view-to-download mapping is three rules because no single rule covers it. +// Report-backed views map by file intersection. The store views map by download +// type, since a 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 caveat is about. +func incompleteRuns(m *dataset.Manifest, view string, files []string) []int { + var runs []int + seen := map[int]bool{} + add := func(run int) { + if !seen[run] { + seen[run] = true + runs = append(runs, run) + } + } + for _, dl := range m.Downloads { + if dl.Complete { + continue + } + if isStoreType(dl.Type) { + if view == dl.Type || view == "run_membership" { + add(dl.RunID) + } + continue + } + if intersects(dl.Files, files) { + add(dl.RunID) + } + } + sort.Ints(runs) + return runs +} + +func isStoreType(t string) bool { return t == store.TypeAnswers || t == store.TypeHistory } + +func intersects(a, b []string) bool { + seen := map[string]bool{} + for _, f := range a { + seen[f] = true + } + for _, f := range b { + if seen[f] { + return true + } + } + return false +} + +// 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, m *dataset.Manifest) ([]string, error) { + if len(m.Materialized) == 0 { + return nil, nil + } + current, err := declaredViews(d.Dir, m) + if err != nil { + return nil, err + } + var stale []string + for view, rec := range m.Materialized { + decl, ok := current[view] + if !ok || !rec.Fresh(d.Dir, decl.files, decl.signature) { + stale = append(stale, view) + } + } + sort.Strings(stale) + return stale, nil +} + +// ShowJSON builds a dataset's summary from one manifest read, with the warnings +// only the view set can decide appended and the whole list re-sorted. Both +// surfaces call it, so the same documented ShowJSON contract cannot carry +// different warning sets on the CLI and over MCP, and the holdings and the +// warnings cannot describe two different versions of the manifest. +func ShowJSON(d *dataset.Dataset, full bool) (*dataset.ShowJSON, error) { + m, err := d.ReadManifest() + if err != nil { + return nil, err + } + s := d.BuildShowJSON(m, full) + stale, err := staleMaterializedViews(d, m) + if err != nil { + return nil, err + } + for _, view := range stale { + s.Warnings = append(s.Warnings, fmt.Sprintf( + "STALE_MATERIALIZED: view %s was materialized from inputs that have since changed; run cc-data dataset materialize %s to refresh it", + view, d.Ref.String())) + } + sort.Strings(s.Warnings) + return s, nil +} diff --git a/internal/duck/materialize_bench_test.go b/internal/duck/materialize_bench_test.go new file mode 100644 index 0000000..e7a866a --- /dev/null +++ b/internal/duck/materialize_bench_test.go @@ -0,0 +1,175 @@ +package duck + +import ( + "bufio" + "context" + "encoding/json" + "fmt" + "io" + "math/rand" + "os" + "strings" + "testing" + "time" + + "github.com/concord-consortium/cc-data-cli/internal/config" + "github.com/concord-consortium/cc-data-cli/internal/dataset" +) + +// timingEnvVar gates the timing assertions. CI runs plain `go test ./...` with no +// -short convention, and this fixture is hundreds of megabytes of generated +// JSONL, so it is opt-in rather than skipped by default. +const timingEnvVar = "CC_DATA_MATERIALIZE_TIMING" + +// timingRows is large enough that the query cost dominates the fixed per-query +// overhead. At this size the fixture is roughly 540MB of JSONL. +const timingRows = 1_000_000 + +// TestMaterializeTiming asserts the shape of the speedup, not a single number. +// Column pruning is the mechanism: an aggregate over two narrow columns is the +// case materializing exists for, while a high-cardinality aggregate is bounded by +// the aggregation rather than the scan and is only required not to get worse. +// +// Run with: CC_DATA_MATERIALIZE_TIMING=1 go test ./internal/duck/ -run Timing -timeout 30m +func TestMaterializeTiming(t *testing.T) { + if os.Getenv(timingEnvVar) == "" { + t.Skipf("set %s=1 to run the timing assertions (they generate a multi-hundred-MB fixture)", timingEnvVar) + } + d := timingFixture(t, timingRows) + + raw := openEngine(t, []DatasetSpec{{DS: d}}, nil) + prunedRaw := timeQuery(t, raw, prunedAggregate) + wideRaw := timeQuery(t, raw, highCardinalityAggregate) + + start := time.Now() + res, err := Materialize(context.Background(), d, MaterializeOptions{}, io.Discard) + if err != nil { + t.Fatal(err) + } + if !containsString(res.Written(), "history") { + t.Fatalf("the history view was not materialized: %+v", res) + } + t.Logf("materialized %d rows in %s", timingRows, time.Since(start).Round(time.Millisecond)) + + mat := openEngine(t, []DatasetSpec{{DS: d}}, nil) + prunedMat := timeQuery(t, mat, prunedAggregate) + wideMat := timeQuery(t, mat, highCardinalityAggregate) + + t.Logf("column-pruned aggregate: raw %s, materialized %s (%.1fx)", + prunedRaw.Round(time.Millisecond), prunedMat.Round(time.Millisecond), + float64(prunedRaw)/float64(prunedMat)) + t.Logf("high-cardinality aggregate: raw %s, materialized %s (%.1fx)", + wideRaw.Round(time.Millisecond), wideMat.Round(time.Millisecond), + float64(wideRaw)/float64(wideMat)) + + // Measured at ~60x, so the threshold carries six times the headroom rather + // than being tuned until it passed. + if prunedMat*10 > prunedRaw { + t.Errorf("column-pruned aggregate should be at least 10x faster materialized: raw %s, materialized %s", + prunedRaw, prunedMat) + } + // Not-worse rather than faster: this shape is bounded by the aggregation, and + // the check exists to catch a change that made materializing a pessimization. + if wideMat > wideRaw { + t.Errorf("high-cardinality aggregate got slower materialized: raw %s, materialized %s", wideRaw, wideMat) + } +} + +const ( + prunedAggregate = `SELECT question_id, count(*) FROM history GROUP BY question_id` + highCardinalityAggregate = `SELECT question_id, count(DISTINCT remote_endpoint) FROM history GROUP BY question_id` +) + +func timeQuery(t *testing.T, e *Engine, query string) time.Duration { + t.Helper() + start := time.Now() + rows, err := e.Query(context.Background(), query) + if err != nil { + t.Fatalf("query %q: %v", query, err) + } + defer rows.Close() + for rows.Next() { + } + if err := rows.Err(); err != nil { + t.Fatal(err) + } + return time.Since(start) +} + +// timingFixture writes a history store directly, bypassing the segment merge, +// because the merge's identity pass is not what is being measured. +// +// The payloads have to vary per row. Given a constant blob, ZSTD dictionary +// encoding compresses the column away and the cost moves entirely into the +// aggregation, so the measurement becomes one of compression rather than of +// materialization. +func timingFixture(t *testing.T, rows int) *dataset.Dataset { + t.Helper() + root := t.TempDir() + d, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "timing"}, "") + if err != nil { + t.Fatal(err) + } + file := "history.v1.jsonl" + f, err := os.Create(d.Path(file)) + if err != nil { + t.Fatal(err) + } + rng := rand.New(rand.NewSource(1)) + w := bufio.NewWriterSize(f, 1<<20) + for i := 0; i < rows; i++ { + rec := map[string]any{ + "source_key": fmt.Sprintf("activity-%d", i%50), + "remote_endpoint": fmt.Sprintf("https://portal/dataservice/external_activity_data/%d", i%20000), + "question_id": fmt.Sprintf("q%d", i%400), + "history_id": fmt.Sprintf("h%d", i), + "state": randomState(rng, i), + "_fetched_at": "2026-09-01T00:00:00Z", + "_run_id": 584, + } + line, err := json.Marshal(rec) + if err != nil { + t.Fatal(err) + } + w.Write(line) + w.Write([]byte("\n")) + } + if err := w.Flush(); err != nil { + t.Fatal(err) + } + f.Close() + if fi, err := os.Stat(d.Path(file)); err == nil { + t.Logf("fixture: %d rows, %.0f MB of JSONL", rows, float64(fi.Size())/(1<<20)) + } + + m, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + m.Stores["history"] = dataset.Store{ + File: file, Version: 1, Count: rows, + Columns: map[string]string{ + "source_key": "VARCHAR", "remote_endpoint": "VARCHAR", "question_id": "VARCHAR", + "history_id": "VARCHAR", "state": "VARCHAR", "_fetched_at": "TIMESTAMP", "_run_id": "BIGINT", + }, + } + if err := writeManifestUnderLock(d, m); err != nil { + t.Fatal(err) + } + return d +} + +// randomState builds a state blob whose values differ per row, so the Parquet +// cannot dictionary-encode the column away. +func randomState(rng *rand.Rand, i int) string { + var b strings.Builder + b.WriteString(`{"version":1,"cells":[`) + for j := 0; j < 5; j++ { + if j > 0 { + b.WriteString(",") + } + fmt.Fprintf(&b, `{"id":"c%d-%d","v":%0.6f,"label":"%x"}`, i%977, j, rng.Float64(), rng.Uint64()) + } + b.WriteString(`]}`) + return b.String() +} diff --git a/internal/duck/materialize_test.go b/internal/duck/materialize_test.go new file mode 100644 index 0000000..fd17c09 --- /dev/null +++ b/internal/duck/materialize_test.go @@ -0,0 +1,882 @@ +package duck + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "reflect" + "sort" + "strings" + "testing" + "time" + + "github.com/concord-consortium/cc-data-cli/internal/config" + "github.com/concord-consortium/cc-data-cli/internal/dataset" +) + +// fullFixture holds a store, a log CSV and a mapping CSV, so the materializable +// set spans store-backed, union and dimension views. +func fullFixture(t *testing.T) *dataset.Dataset { + t.Helper() + root := t.TempDir() + d, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}, "") + if err != nil { + t.Fatal(err) + } + buildStore(t, d, 584, [][]byte{answerRec("s", "e1", "q1", "hi"), answerRec("s", "e2", "q2", "yo")}) + addReportCSV(t, d, 584, "answers", "student_id,res_1_q1_answer\nPrompt,What?\nCorrect answer,42\n1,hello\n2,world\n") + addDimensionCSV(t, d, dimFixture{run: 700, slug: "student-id-mapping", fetchedAt: time.Unix(1, 0).UTC(), + csv: mappingCSV(mappingRow(1, 30, endpointAAA))}) + // A log run puts the logs view in the set, which is the one carrying JSON, + // TIMESTAMP and LIST columns derived per query rather than read from the CSV. + addLogCSV(t, d, logFixture{run: 900, csv: clueCSV()}) + return d +} + +func materialize(t *testing.T, d *dataset.Dataset, opts MaterializeOptions) (MaterializeResult, string) { + t.Helper() + var warn bytes.Buffer + res, err := Materialize(context.Background(), d, opts, &warn) + if err != nil { + t.Fatalf("Materialize: %v", err) + } + return res, warn.String() +} + +func columnTypes(t *testing.T, e *Engine, view string) map[string]string { + t.Helper() + rows, err := e.Query(context.Background(), + fmt.Sprintf("SELECT column_name, column_type FROM (DESCRIBE SELECT * FROM %s)", view)) + if err != nil { + t.Fatalf("describe %s: %v", view, err) + } + defer rows.Close() + out := map[string]string{} + for rows.Next() { + var name, typ string + if err := rows.Scan(&name, &typ); err != nil { + t.Fatal(err) + } + out[name] = typ + } + return out +} + +func viewRows(t *testing.T, e *Engine, view string) []string { + t.Helper() + rows, err := e.Query(context.Background(), fmt.Sprintf("SELECT * FROM %s ORDER BY ALL", view)) + if err != nil { + t.Fatalf("select %s: %v", view, err) + } + defer rows.Close() + cols, err := rows.Columns() + if err != nil { + t.Fatal(err) + } + var out []string + for rows.Next() { + cells := make([]any, len(cols)) + ptrs := make([]any, len(cols)) + for i := range cells { + ptrs[i] = &cells[i] + } + if err := rows.Scan(ptrs...); err != nil { + t.Fatal(err) + } + out = append(out, fmt.Sprintf("%v", cells)) + } + return out +} + +func tempFiles(t *testing.T, d *dataset.Dataset) []string { + t.Helper() + entries, err := os.ReadDir(d.Path(dataset.MaterializedDir)) + if err != nil { + if os.IsNotExist(err) { + return nil + } + t.Fatal(err) + } + var out []string + for _, e := range entries { + if strings.Contains(e.Name(), ".tmp-") { + out = append(out, e.Name()) + } + } + return out +} + +// TestMaterializeReturnsIdenticalResults iterates the materializable views rather +// than a hand-written list, so a view added later cannot go uncompared. +func TestMaterializeReturnsIdenticalResults(t *testing.T) { + d := fullFixture(t) + views := materializableViews(mustManifest(t, d)) + if len(views) == 0 { + t.Fatal("fixture materializes nothing, so the comparison below asserts nothing") + } + + raw := openEngine(t, []DatasetSpec{{DS: d}}, nil) + before := map[string][]string{} + beforeTypes := map[string]map[string]string{} + for _, v := range views { + before[v] = viewRows(t, raw, sqlIdent(v)) + beforeTypes[v] = columnTypes(t, raw, sqlIdent(v)) + } + + res, _ := materialize(t, d, MaterializeOptions{}) + sort.Strings(views) + if !reflect.DeepEqual(res.Written(), views) { + t.Fatalf("written = %v, want every materializable view %v", res.Written(), views) + } + + after := openEngine(t, []DatasetSpec{{DS: d}}, nil) + for _, v := range views { + if got := viewRows(t, after, sqlIdent(v)); !reflect.DeepEqual(got, before[v]) { + t.Fatalf("%s rows differ after materializing:\n raw = %v\n mat = %v", v, before[v], got) + } + if got := columnTypes(t, after, sqlIdent(v)); !reflect.DeepEqual(got, beforeTypes[v]) { + t.Fatalf("%s column types differ after materializing:\n raw = %v\n mat = %v", v, beforeTypes[v], got) + } + } + if f := tempFiles(t, d); len(f) > 0 { + t.Fatalf("temp files survived a clean run: %v", f) + } + // Named individually because these are computed per query, so their types are + // what the round trip could lose. + logTypes := columnTypes(t, after, `"logs"`) + for col, want := range map[string]string{ + "parameters_json": "JSON", "extras_json": "JSON", + "event_time": "TIMESTAMP", "received_time": "TIMESTAMP", + } { + if logTypes[col] != want { + t.Errorf("materialized logs.%s is %q, want %q", col, logTypes[col], want) + } + } +} + +// TestMaterializedParquetCarriesItsProvenance covers the footer metadata. Nothing +// in cc-data reads it back; it is there so a Parquet found in a backup or a +// copied folder can say what it is. +func TestMaterializedParquetCarriesItsProvenance(t *testing.T) { + d := fullFixture(t) + materialize(t, d, MaterializeOptions{}) + + e := openEngine(t, []DatasetSpec{{DS: d}}, nil) + path := d.Path(filepath.Join(dataset.MaterializedDir, "answers.parquet")) + rows, err := e.Query(context.Background(), + fmt.Sprintf("SELECT key, value FROM parquet_kv_metadata(%s)", sqlStr(path))) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + kv := map[string]string{} + for rows.Next() { + var k, v []byte + if err := rows.Scan(&k, &v); err != nil { + t.Fatal(err) + } + kv[string(k)] = string(v) + } + if kv["view"] != "answers" { + t.Errorf("footer view = %q, want answers", kv["view"]) + } + if _, err := time.Parse(time.RFC3339, kv["built_at"]); err != nil { + t.Errorf("footer built_at %q is not a timestamp: %v", kv["built_at"], err) + } +} + +func TestMaterializeSkipsFreshViewsUnlessForced(t *testing.T) { + d := fullFixture(t) + first, _ := materialize(t, d, MaterializeOptions{}) + + second, _ := materialize(t, d, MaterializeOptions{}) + if len(second.Written()) != 0 { + t.Fatalf("second run wrote %v, want nothing: every view is fresh", second.Written()) + } + if !reflect.DeepEqual(second.Fresh(), first.Written()) { + t.Fatalf("skipped = %v, want the views the first run wrote %v", second.Fresh(), first.Written()) + } + + forced, _ := materialize(t, d, MaterializeOptions{Force: true}) + if !reflect.DeepEqual(forced.Written(), first.Written()) { + t.Fatalf("--force wrote %v, want %v", forced.Written(), first.Written()) + } +} + +func TestMaterializeRefusesAMissingInputPerView(t *testing.T) { + d := fullFixture(t) + if err := os.Remove(d.Path("report_700.csv")); err != nil { + t.Fatal(err) + } + + res, _ := materialize(t, d, MaterializeOptions{}) + // reports and student_id_mapping both read the deleted CSV. + for _, view := range []string{"reports", "student_id_mapping"} { + reason, ok := res.Refused()[view] + if !ok { + t.Fatalf("%s should be refused; refused = %v", view, res.Refused()) + } + if !strings.Contains(reason, "report_700.csv") { + t.Fatalf("%s reason does not name the missing file: %s", view, reason) + } + if !strings.Contains(reason, "re-fetch run 700") || !strings.Contains(reason, "dataset reindex") { + t.Fatalf("%s reason must name both remedies: %s", view, reason) + } + if !strings.Contains(reason, "--allow-partial") { + t.Fatalf("%s reason must name the override: %s", view, reason) + } + } + if !containsAll(res.Written(), "answers", "run_membership") { + t.Fatalf("clean views should still be written, got %v", res.Written()) + } + if !res.Refusals() { + t.Fatal("a refusal must be reportable, so the command can exit non-zero") + } + + // --force is not a second way past the refusal; that is --allow-partial's job. + forced, _ := materialize(t, d, MaterializeOptions{Force: true}) + if _, ok := forced.Refused()["reports"]; !ok { + t.Fatalf("--force must not override a missing input, refused = %v", forced.Refused()) + } + + partial, _ := materialize(t, d, MaterializeOptions{AllowPartial: true}) + if len(partial.Refused()) != 0 { + t.Fatalf("--allow-partial should clear the refusals, got %v", partial.Refused()) + } + if !containsAll(partial.Written(), "reports", "student_id_mapping") { + t.Fatalf("--allow-partial should write the short views, got %v", partial.Written()) + } + // The shortfall rides on the outcome, which is all an MCP caller gets back. + if r := outcomeReason(partial, "reports"); !strings.Contains(r, "built from 2 of 3 declared inputs") || !strings.Contains(r, "report_700.csv") { + t.Fatalf("a partial copy must report the shortfall as a count and name the file, got %q", r) + } + if r := outcomeReason(partial, "answers"); r != "" { + t.Fatalf("a complete copy carries no caveat, got %q", r) + } + if f := tempFiles(t, d); len(f) > 0 { + t.Fatalf("temp files survived: %v", f) + } + + // A second run finds the short copy fresh, since the missing input is still + // missing, and the caveat has to survive that. + again, _ := materialize(t, d, MaterializeOptions{AllowPartial: true}) + if !containsString(again.Fresh(), "reports") { + t.Fatalf("the short copy should be fresh on the next run, got %+v", again.Views) + } + if r := outcomeReason(again, "reports"); !strings.Contains(r, "built from 2 of 3 declared inputs") { + t.Fatalf("a fresh short copy must still carry its caveat, got %q", r) + } +} + +func outcomeReason(res MaterializeResult, view string) string { + for _, o := range res.Views { + if o.View == view { + return o.Reason + } + } + return "" +} + +// TestMaterializeRefusesADegradedViewEvenWithAllowPartial covers total loss: the +// view registered from its typed-empty fallback, so there is nothing to publish. +func TestMaterializeRefusesADegradedViewEvenWithAllowPartial(t *testing.T) { + d := fullFixture(t) + storeFile := mustManifest(t, d).Stores["answers"].File + if err := os.Remove(d.Path(storeFile)); err != nil { + t.Fatal(err) + } + + res, _ := materialize(t, d, MaterializeOptions{AllowPartial: true}) + reason, ok := res.Refused()["answers"] + if !ok { + t.Fatalf("a degraded view must be refused with --allow-partial given; refused = %v", res.Refused()) + } + if !strings.Contains(reason, "typed-empty") { + t.Fatalf("reason should say the view degraded: %s", reason) + } + for _, w := range res.Written() { + if w == "answers" { + t.Fatal("a degraded view must never be published") + } + } +} + +// TestMaterializeRefusesAViewItCannotCopy holds the per-view rule for a failing +// copy: that view is refused, the others are still written, and the run itself +// succeeds. A store whose bytes will not parse is the shape that reaches the +// copy, because read_json with an explicit column map validates no content at +// CREATE VIEW, so the view registers without complaint and fails at query time. +func TestMaterializeRefusesAViewItCannotCopy(t *testing.T) { + d := fullFixture(t) + storeFile := mustManifest(t, d).Stores["answers"].File + if err := os.WriteFile(d.Path(storeFile), []byte("{not json at all\n"), 0o600); err != nil { + t.Fatal(err) + } + + res, err := Materialize(context.Background(), d, MaterializeOptions{}, io.Discard) + if err != nil { + t.Fatalf("one view's failed copy must not fail the run: %v", err) + } + reason, ok := res.Refused()["answers"] + if !ok { + t.Fatalf("the view whose copy failed should be refused, got %+v", res.Views) + } + if !strings.Contains(reason, "answers") { + t.Fatalf("the reason should carry the failing copy's error: %s", reason) + } + if len(res.Written()) == 0 { + t.Fatalf("the other views should still be written, got %+v", res.Views) + } + if f := tempFiles(t, d); len(f) > 0 { + t.Fatalf("a failed copy must leave no temp file: %v", f) + } +} + +func TestMaterializeRefusesAStoreViewShortOfItsCount(t *testing.T) { + d := fullFixture(t) + m := mustManifest(t, d) + st := m.Stores["answers"] + st.Count = 99 + m.Stores["answers"] = st + if err := writeManifestUnderLock(d, m); err != nil { + t.Fatal(err) + } + + res, _ := materialize(t, d, MaterializeOptions{}) + reason, ok := res.Refused()["answers"] + if !ok { + t.Fatalf("a short store copy must be refused; refused = %v", res.Refused()) + } + if !strings.Contains(reason, "99") { + t.Fatalf("reason should name the manifest's count: %s", reason) + } + if _, err := os.Stat(d.Path(filepath.Join(dataset.MaterializedDir, "answers.parquet"))); !os.IsNotExist(err) { + t.Fatal("a refused copy must not be left on disk under its final name") + } +} + +func TestMaterializeReportsAnIncompleteDownloadWithoutBlocking(t *testing.T) { + d := fullFixture(t) + m := mustManifest(t, d) + // Two incomplete store downloads for one run: run_membership maps by type, so + // both reach it and the run must still be named once. + m.Downloads = append(m.Downloads, + dataset.Download{Type: "answers", RunID: 901, Complete: false}, + dataset.Download{Type: "history", RunID: 901, Complete: false}) + if err := writeManifestUnderLock(d, m); err != nil { + t.Fatal(err) + } + + res, _ := materialize(t, d, MaterializeOptions{}) + if len(res.Refused()) != 0 { + t.Fatalf("an incomplete download must block nothing, refused = %v", res.Refused()) + } + if !containsAll(res.Written(), "answers", "run_membership") { + t.Fatalf("the store views should still be written, got %v", res.Written()) + } + for _, view := range []string{"answers", "run_membership"} { + r := outcomeReason(res, view) + if !strings.Contains(r, "run 901") { + t.Fatalf("%s reads an incomplete download and its outcome must say so: %q", view, r) + } + if strings.Contains(r, "901, 901") { + t.Fatalf("a run reached through two downloads must be named once: %q", r) + } + } + if r := outcomeReason(res, "logs"); r != "" { + t.Fatalf("a view that reads no incomplete download carries no caveat, got %q", r) + } +} + +// TestMaterializeDiscardsACopyWhoseInputsMoved drives the window between the copy +// and the repoint, which is the one place a Parquet can describe a state that is +// already gone. +func TestMaterializeDiscardsACopyWhoseInputsMoved(t *testing.T) { + d := fullFixture(t) + res, err := materializeWithRace(t, d, func() { + buildStore(t, d, 585, [][]byte{answerRec("s", "e9", "q9", "later")}) + }) + if err != nil { + t.Fatal(err) + } + if containsString(res.Written(), "answers") { + t.Fatal("a copy whose store moved under it must not be recorded as written") + } + // The distinction the status exists for: a discarded view is not materialized + // and running again picks it up, where a fresh one needs nothing. + if containsString(res.Fresh(), "answers") { + t.Fatal("a discarded copy must not be reported as already fresh; nothing about it is current") + } + if !containsString(res.Discarded(), "answers") { + t.Fatalf("the discarded view should carry the discarded status, got %+v", res.Views) + } + for _, o := range res.Views { + if o.View == "answers" && o.Reason == "" { + t.Fatal("a discarded view must say why, or the caller has nothing to render") + } + } + if _, ok := mustManifest(t, d).Materialized["answers"]; ok { + t.Fatal("a discarded copy must leave no manifest entry") + } + if f := tempFiles(t, d); len(f) > 0 { + t.Fatalf("a discarded copy must leave no temp file: %v", f) + } +} + +// TestMaterializeReportsEveryViewExactlyOnce is the invariant the outcome list +// exists to make structural: a view cannot land in two statuses or in none. +func TestMaterializeReportsEveryViewExactlyOnce(t *testing.T) { + d := fullFixture(t) + // One view refused and one already fresh, so the run spans several statuses. + materialize(t, d, MaterializeOptions{}) + if err := os.Remove(d.Path("report_700.csv")); err != nil { + t.Fatal(err) + } + res, _ := materialize(t, d, MaterializeOptions{}) + + want := materializableViews(mustManifest(t, d)) + seen := map[string]int{} + for _, o := range res.Views { + seen[o.View]++ + if o.Status == "" { + t.Fatalf("view %s has no status", o.View) + } + } + if len(seen) != len(want) { + t.Fatalf("outcomes cover %d views, want the %d materializable ones: %+v", len(seen), len(want), res.Views) + } + for _, view := range want { + if seen[view] != 1 { + t.Fatalf("view %s appears %d times, want exactly once: %+v", view, seen[view], res.Views) + } + } + // And the order is the view order, so output is stable between runs. + var got []string + for _, o := range res.Views { + got = append(got, o.View) + } + sort.Strings(want) + if !reflect.DeepEqual(got, want) { + t.Fatalf("outcome order = %v, want view order %v", got, want) + } +} + +// TestMaterializeDiscardsACopyWhoseDefinitionMoved drives the manifest-only race: +// a change that rewrites no input byte but changes what the view returns. A +// dimension view embeds its download's fetch time to break ties between runs, so +// re-stamping that time reorders the dedupe under a copy already taken. +func TestMaterializeDiscardsACopyWhoseDefinitionMoved(t *testing.T) { + d := fullFixture(t) + res, err := materializeWithRace(t, d, func() { + m := mustManifest(t, d) + for i := range m.Downloads { + if m.Downloads[i].Slug == "student-id-mapping" { + m.Downloads[i].FetchedAt = m.Downloads[i].FetchedAt.Add(time.Hour) + } + } + if err := writeManifestUnderLock(d, m); err != nil { + t.Error(err) + } + }) + if err != nil { + t.Fatal(err) + } + if containsString(res.Written(), "student_id_mapping") { + t.Fatalf("a copy whose view definition moved under it must not be recorded: %+v", res.Views) + } + if !containsString(res.Discarded(), "student_id_mapping") { + t.Fatalf("it should be discarded, got %+v", res.Views) + } + if _, ok := mustManifest(t, d).Materialized["student_id_mapping"]; ok { + t.Fatal("a discarded copy must leave no manifest entry") + } +} + +// materializeWithRace runs between() in the window after every copy is made and +// before the manifest is repointed, which is when the mutation locks are free. +func materializeWithRace(t *testing.T, d *dataset.Dataset, between func()) (MaterializeResult, error) { + t.Helper() + return materializeWithRaceCtx(t, context.Background(), d, between) +} + +func materializeWithRaceCtx(t *testing.T, ctx context.Context, d *dataset.Dataset, between func()) (MaterializeResult, error) { + t.Helper() + orig := testHookBeforeRepoint + testHookBeforeRepoint = func() { + testHookBeforeRepoint = orig + between() + } + t.Cleanup(func() { testHookBeforeRepoint = orig }) + return Materialize(ctx, d, MaterializeOptions{}, io.Discard) +} + +// TestMaterializeWaitsForAFetchBeforeCommitting holds the activity lock shared, +// as a fetch does for its whole run, across the repoint. Failing busy there would +// delete every finished copy because someone started a get. +func TestMaterializeWaitsForAFetchBeforeCommitting(t *testing.T) { + d := fullFixture(t) + res, err := materializeWithRace(t, d, func() { + ok, err := d.Activity().TryRLock() + if err != nil || !ok { + t.Fatalf("taking the activity lock shared: ok=%v err=%v", ok, err) + } + go func() { + time.Sleep(300 * time.Millisecond) + d.Activity().RUnlock() + }() + }) + if err != nil { + t.Fatalf("the run must wait for the fetch rather than fail: %v", err) + } + if !containsAll(res.Written(), "answers", "reports", "logs") { + t.Fatalf("the copies made before the fetch must still be committed, got %v", res.Written()) + } +} + +// TestMaterializeAbandonsTheWaitWhenCancelled is the way out of that wait: a +// paged fetch can run for a long time, and the caller's context is what ends it. +func TestMaterializeAbandonsTheWaitWhenCancelled(t *testing.T) { + d := fullFixture(t) + ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond) + defer cancel() + _, err := materializeWithRaceCtx(t, ctx, d, func() { + ok, err := d.Activity().TryRLock() + if err != nil || !ok { + t.Fatalf("taking the activity lock shared: ok=%v err=%v", ok, err) + } + t.Cleanup(d.Activity().RUnlock) + }) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("a cancelled wait should surface the context error, got %v", err) + } + if f := tempFiles(t, d); len(f) > 0 { + t.Fatalf("an abandoned run must leave no temp file: %v", f) + } + if n := len(mustManifest(t, d).Materialized); n != 0 { + t.Fatalf("an abandoned run must record nothing, got %d entries", n) + } +} + +// TestMaterializeRefusesOnlyTheViewWhoseRenameFails pins the per-view rule at +// the commit: one rename failing must not lose the views renamed before it, nor +// leave them on disk under their final names with no manifest entry. +func TestMaterializeRefusesOnlyTheViewWhoseRenameFails(t *testing.T) { + d := fullFixture(t) + // A directory squatting on the final name makes that one rename fail. + if err := os.MkdirAll(d.Path(filepath.Join(dataset.MaterializedDir, "answers.parquet")), 0o700); err != nil { + t.Fatal(err) + } + + res, _ := materialize(t, d, MaterializeOptions{}) + if _, ok := res.Refused()["answers"]; !ok { + t.Fatalf("the view whose rename failed should be refused, got %+v", res.Views) + } + if !containsAll(res.Written(), "reports", "logs", "run_membership", "student_id_mapping") { + t.Fatalf("the other views must still be written, got %v", res.Written()) + } + m := mustManifest(t, d) + for _, view := range res.Written() { + rec, ok := m.Materialized[view] + if !ok { + t.Fatalf("%s reported written but not recorded", view) + } + if _, err := os.Stat(d.Path(rec.File)); err != nil { + t.Fatalf("%s recorded but not on disk: %v", view, err) + } + } + if _, ok := m.Materialized["answers"]; ok { + t.Fatal("a refused view must not be recorded") + } + if f := tempFiles(t, d); len(f) > 0 { + t.Fatalf("the failed copy must be cleaned up: %v", f) + } +} + +func containsString(all []string, want string) bool { + for _, s := range all { + if s == want { + return true + } + } + return false +} + +func containsAll(all []string, want ...string) bool { + for _, w := range want { + if !containsString(all, w) { + return false + } + } + return true +} + +func TestStaleMaterializedViewsTracksTheInputs(t *testing.T) { + d := fullFixture(t) + materialize(t, d, MaterializeOptions{}) + + stale, err := staleMaterializedViews(d, mustManifest(t, d)) + if err != nil { + t.Fatal(err) + } + if len(stale) != 0 { + t.Fatalf("nothing should be stale right after materializing, got %v", stale) + } + + buildStore(t, d, 585, [][]byte{answerRec("s", "e9", "q9", "later")}) + stale, err = staleMaterializedViews(d, mustManifest(t, d)) + if err != nil { + t.Fatal(err) + } + if !containsAll(stale, "answers", "run_membership") { + t.Fatalf("a get that moved the store should make its views stale, got %v", stale) + } +} + +func TestShowJSONAddsTheStaleWarning(t *testing.T) { + d := fullFixture(t) + materialize(t, d, MaterializeOptions{}) + buildStore(t, d, 585, [][]byte{answerRec("s", "e9", "q9", "later")}) + + raw := d.BuildShowJSON(mustManifest(t, d), false) + for _, w := range raw.Warnings { + if strings.HasPrefix(w, "STALE_MATERIALIZED") { + t.Fatal("the raw show contract cannot decide staleness; only the view set can") + } + } + s, err := ShowJSON(d, false) + if err != nil { + t.Fatal(err) + } + var named []string + for _, w := range s.Warnings { + if strings.HasPrefix(w, "STALE_MATERIALIZED") { + named = append(named, w) + } + } + if len(named) == 0 { + t.Fatalf("want a stale warning, got %v", s.Warnings) + } + if !strings.Contains(named[0], "dataset materialize") { + t.Fatalf("the warning must name the refresh command: %s", named[0]) + } + if !sort.StringsAreSorted(s.Warnings) { + t.Fatalf("ShowJSON must re-sort after appending: %v", s.Warnings) + } +} + +// TestReindexDropsTheEntriesAndKeepsTheFiles pins three things at once: reindex +// drops the manifest entries, leaves the Parquet files on disk, and dataset show +// then names them as unreferenced. The middle one is what would catch a later +// tidy-up making reindex delete the folder. +func TestReindexDropsTheEntriesAndKeepsTheFiles(t *testing.T) { + d := fullFixture(t) + res, _ := materialize(t, d, MaterializeOptions{}) + if len(res.Written()) == 0 { + t.Fatal("fixture materialized nothing") + } + + if err := d.Reindex(); err != nil { + t.Fatal(err) + } + if got := mustManifest(t, d).Materialized; len(got) != 0 { + t.Fatalf("reindex must drop the materialization entries, got %v", got) + } + for _, view := range res.Written() { + if _, err := os.Stat(d.Path(filepath.Join(dataset.MaterializedDir, view+".parquet"))); err != nil { + t.Fatalf("reindex must leave %s.parquet on disk for external readers: %v", view, err) + } + } + + s, err := ShowJSON(d, false) + if err != nil { + t.Fatal(err) + } + var named int + for _, w := range s.Warnings { + if strings.HasPrefix(w, "UNREFERENCED_MATERIALIZED") { + named++ + } + } + if named != len(res.Written()) { + t.Fatalf("want every leftover Parquet named, got %d of %d: %v", named, len(res.Written()), s.Warnings) + } +} + +// TestMaterializeSweepsATempFromADeadRun covers the state a hard interrupt +// leaves: nothing installs a signal handler, so a killed run's deferred cleanup +// never runs and its temp file survives. The next run clears it, which it can do +// unconditionally because it holds the materialize guard and the kernel drops +// that guard when its holder dies. +func TestMaterializeSweepsATempFromADeadRun(t *testing.T) { + d := fullFixture(t) + if err := os.MkdirAll(d.Path(dataset.MaterializedDir), 0o700); err != nil { + t.Fatal(err) + } + stale := filepath.Join(dataset.MaterializedDir, "answers"+tempParquetInfix+"99") + if err := os.WriteFile(d.Path(stale), []byte("half a parquet"), 0o600); err != nil { + t.Fatal(err) + } + + materialize(t, d, MaterializeOptions{}) + + if _, err := os.Stat(d.Path(stale)); !os.IsNotExist(err) { + t.Fatalf("a dead run's temp file should be swept, stat err = %v", err) + } + if f := tempFiles(t, d); len(f) > 0 { + t.Fatalf("no temp file should survive: %v", f) + } +} + +// TestMaterializeRefusesASecondRun is what makes the sweep safe: while one run +// holds the guard, a second cannot be inside the window where its temp files +// exist, so there is never a live owner for the sweep to rob. +func TestMaterializeRefusesASecondRun(t *testing.T) { + d := fullFixture(t) + release, err := d.LockMaterialize() + if err != nil { + t.Fatal(err) + } + defer release() + + _, err = Materialize(context.Background(), d, MaterializeOptions{}, io.Discard) + if !errors.Is(err, dataset.ErrBusy) { + t.Fatalf("a second run should report the dataset busy, got %v", err) + } +} + +// TestMaterializeHoldsItsGuardForTheWholeRun pins the property the sweep rests +// on. The check runs in the window where the temp files exist, so a run that +// took the guard and released it straight away fails here: at that point a +// second run could start and sweep away this run's work. +func TestMaterializeHoldsItsGuardForTheWholeRun(t *testing.T) { + d := fullFixture(t) + var heldWhileTempsExist bool + orig := testHookBeforeRepoint + testHookBeforeRepoint = func() { + testHookBeforeRepoint = orig + if f := tempFiles(t, d); len(f) == 0 { + t.Error("the hook must run while temp files exist, or this asserts nothing") + } + release, err := d.LockMaterialize() + heldWhileTempsExist = err != nil + if err == nil { + release() + } + } + t.Cleanup(func() { testHookBeforeRepoint = orig }) + + materialize(t, d, MaterializeOptions{}) + if !heldWhileTempsExist { + t.Fatal("the guard must stay held for the whole run; released early, a second run could sweep this run's temp files") + } +} + +// TestMaterializeReleasesItsGuard keeps the guard from leaking across runs, which +// would make every later run on the dataset fail as busy. +func TestMaterializeReleasesItsGuard(t *testing.T) { + d := fullFixture(t) + materialize(t, d, MaterializeOptions{}) + + release, err := d.LockMaterialize() + if err != nil { + t.Fatalf("the guard should be free once a run finishes: %v", err) + } + release() +} + +// TestMaterializeRebuildsAViewWhoseParquetIsGone covers the state a researcher +// creates by following the documentation: the folder is safe to delete at any +// time. The recorded inputs are untouched, so the entry still reads as fresh, +// and without a readability check the view would be skipped forever while +// queries quietly fell back to the raw artifacts. +func TestMaterializeRebuildsAViewWhoseParquetIsGone(t *testing.T) { + for _, tc := range []struct { + name string + damage func(t *testing.T, path string) + }{ + {"deleted", func(t *testing.T, path string) { + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + }}, + {"truncated", func(t *testing.T, path string) { + if err := os.WriteFile(path, []byte("PAR1"), 0o600); err != nil { + t.Fatal(err) + } + }}, + } { + t.Run(tc.name, func(t *testing.T) { + d := fullFixture(t) + materialize(t, d, MaterializeOptions{}) + path := d.Path(filepath.Join(dataset.MaterializedDir, "answers.parquet")) + tc.damage(t, path) + + res, _ := materialize(t, d, MaterializeOptions{}) + if containsString(res.Fresh(), "answers") { + t.Fatalf("a view whose Parquet is unusable must not read as fresh: %+v", res.Views) + } + if !containsString(res.Written(), "answers") { + t.Fatalf("it should be rebuilt without --force, got %+v", res.Views) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("the Parquet should be back: %v", err) + } + }) + } +} + +// TestMaterializeRebuildsWhenTheViewDefinitionChanges is the axis the input +// fingerprints cannot see. A cc-data release that changes a view's SQL leaves +// every input byte untouched, so without the signature the older copy would keep +// being read, and queries would return the old view's columns indefinitely. +func TestMaterializeRebuildsWhenTheViewDefinitionChanges(t *testing.T) { + d := fullFixture(t) + materialize(t, d, MaterializeOptions{}) + + m := mustManifest(t, d) + rec := m.Materialized["answers"] + if rec.Signature == "" { + t.Fatal("a materialized view must record the definition it was built from") + } + rec.Signature = "what an older release produced" + m.Materialized["answers"] = rec + if err := writeManifestUnderLock(d, m); err != nil { + t.Fatal(err) + } + + stale, err := staleMaterializedViews(d, mustManifest(t, d)) + if err != nil { + t.Fatal(err) + } + if !containsString(stale, "answers") { + t.Fatalf("a copy built from a different view definition is stale, got %v", stale) + } + res, _ := materialize(t, d, MaterializeOptions{}) + if !containsString(res.Written(), "answers") { + t.Fatalf("it should be rebuilt, got %+v", res.Views) + } +} + +// TestMaterializedViewIsIgnoredWhenTheDefinitionMoved is the read-path half: the +// engine must not serve a Parquet built from a definition it no longer uses. +func TestMaterializedViewIsIgnoredWhenTheDefinitionMoved(t *testing.T) { + d, _, storeFile := answersFixture(t, "ds") + plantAnswersSentinel(t, d, storeFile) + + m := mustManifest(t, d) + rec := m.Materialized["answers"] + rec.Signature = "what an older release produced" + m.Materialized["answers"] = rec + if err := writeManifestUnderLock(d, m); err != nil { + t.Fatal(err) + } + + e := openEngine(t, []DatasetSpec{{DS: d}}, nil) + if n := sentinelCount(t, e, "answers"); n != 0 { + t.Fatalf("sentinel rows = %d, want 0: a Parquet from an older view definition must not be read", n) + } +} diff --git a/internal/duck/materialized_test.go b/internal/duck/materialized_test.go new file mode 100644 index 0000000..91a6b91 --- /dev/null +++ b/internal/duck/materialized_test.go @@ -0,0 +1,320 @@ +package duck + +import ( + "bytes" + "context" + "database/sql" + "fmt" + "os" + "path/filepath" + "reflect" + "sort" + "testing" + "time" + + "github.com/concord-consortium/cc-data-cli/internal/config" + "github.com/concord-consortium/cc-data-cli/internal/dataset" +) + +// plantParquet writes a Parquet for one view and records it in the manifest. The +// select runs against the dataset's raw views on a scratch connection, so the +// Parquet carries the view's own column set and types. +func plantParquet(t *testing.T, d *dataset.Dataset, view, selectSQL string, inputs []string) { + t.Helper() + if err := os.MkdirAll(d.Path(dataset.MaterializedDir), 0o700); err != nil { + t.Fatal(err) + } + rel := filepath.ToSlash(filepath.Join(dataset.MaterializedDir, view+".parquet")) + + db, err := sql.Open("duckdb", "") + if err != nil { + t.Fatal(err) + } + defer db.Close() + canon, err := canonicalize(d.Dir) + if err != nil { + t.Fatal(err) + } + vs := viewSet{canonDir: canon, m: mustManifest(t, d)} + ctx := context.Background() + var signature string + for _, st := range vs.statements() { + if bareViewName(st.name, vs.prefix) == view { + signature = viewSignature(st, canon, vs.prefix) + } + if _, err := db.ExecContext(ctx, st.primary); err != nil { + if _, ferr := db.ExecContext(ctx, st.fallback); ferr != nil { + t.Fatalf("registering %s: %v", st.name, err) + } + } + } + if signature == "" { + t.Fatalf("no statement for view %s, so the planted entry could never be read", view) + } + copySQL := fmt.Sprintf("COPY (%s) TO %s (FORMAT parquet, COMPRESSION zstd)", selectSQL, sqlStr(d.Path(rel))) + if _, err := db.ExecContext(ctx, copySQL); err != nil { + t.Fatalf("planting %s: %v", rel, err) + } + + m := mustManifest(t, d) + m.Materialized[view] = dataset.Materialized{File: rel, Inputs: dataset.FingerprintInputs(d.Dir, inputs), Signature: signature} + if err := writeManifestUnderLock(d, m); err != nil { + t.Fatal(err) + } +} + +func writeManifestUnderLock(d *dataset.Dataset, m *dataset.Manifest) error { + ok, err := d.Lock().TryLock() + if err != nil { + return err + } + if !ok { + return fmt.Errorf("dataset lock busy") + } + defer d.Lock().Unlock() + return d.WriteManifest(m) +} + +func mustManifest(t *testing.T, d *dataset.Dataset) *dataset.Manifest { + t.Helper() + m, err := d.ReadManifest() + if err != nil { + t.Fatal(err) + } + return m +} + +// answersFixture is a two-record answers store, returned with the data root so a +// caller can rename it. +func answersFixture(t *testing.T, name string) (*dataset.Dataset, string, string) { + t.Helper() + root := t.TempDir() + d, err := dataset.Create(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: name}, "") + if err != nil { + t.Fatal(err) + } + buildStore(t, d, 584, [][]byte{answerRec("s", "e1", "q1", "hi"), answerRec("s", "e2", "q2", "yo")}) + return d, root, mustManifest(t, d).Stores["answers"].File +} + +// plantAnswersSentinel materializes the answers view with one extra row the raw +// store does not have, so a query that returns it can only have read the Parquet. +func plantAnswersSentinel(t *testing.T, d *dataset.Dataset, storeFile string) { + t.Helper() + sel := `SELECT * FROM "answers" UNION ALL (SELECT * REPLACE ('SENTINEL' AS question_id) FROM "answers" LIMIT 1)` + plantParquet(t, d, "answers", sel, []string{storeFile}) +} + +func sentinelCount(t *testing.T, e *Engine, view string) int { + t.Helper() + return queryInt(t, e, fmt.Sprintf("SELECT count(*) FROM %s WHERE question_id = 'SENTINEL'", view)) +} + +func TestMaterializedViewIsPreferredWhileFresh(t *testing.T) { + d, _, storeFile := answersFixture(t, "ds") + plantAnswersSentinel(t, d, storeFile) + + e := openEngine(t, []DatasetSpec{{DS: d}}, nil) + if n := sentinelCount(t, e, "answers"); n != 1 { + t.Fatalf("sentinel rows = %d, want 1: the fresh Parquet should be the source", n) + } +} + +func TestMaterializedViewIsIgnoredWhenStale(t *testing.T) { + d, _, storeFile := answersFixture(t, "ds") + plantAnswersSentinel(t, d, storeFile) + // A second run rewrites the store, moving the recorded fingerprint. + buildStore(t, d, 585, [][]byte{answerRec("s", "e3", "q3", "new")}) + + var warn bytes.Buffer + e, err := Open(context.Background(), []DatasetSpec{{DS: d}}, nil, &warn) + if err != nil { + t.Fatal(err) + } + defer e.Close() + if n := sentinelCount(t, e, "answers"); n != 0 { + t.Fatalf("sentinel rows = %d, want 0: a stale Parquet must not be read", n) + } + if warn.Len() != 0 { + t.Fatalf("going stale must be silent, got %q", warn.String()) + } +} + +// TestOpenSessionStopsReadingAParquetThatWentStale is the repl's case: one +// engine outlives a fetch. Freshness decided once at open would leave the +// session answering from the copy while a fresh session reads the raw files. +func TestOpenSessionStopsReadingAParquetThatWentStale(t *testing.T) { + d, _, storeFile := answersFixture(t, "ds") + plantAnswersSentinel(t, d, storeFile) + + var warn bytes.Buffer + e, err := Open(context.Background(), []DatasetSpec{{DS: d}}, nil, &warn) + if err != nil { + t.Fatal(err) + } + defer e.Close() + if n := sentinelCount(t, e, "answers"); n != 1 { + t.Fatalf("sentinel rows before the change = %d, want 1", n) + } + + // Changed in place, as a re-fetched report CSV is, so a session that never + // used the Parquet would see the new row on its next query. + f, err := os.OpenFile(d.Path(storeFile), os.O_APPEND|os.O_WRONLY, 0o600) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write(append(answerRec("s", "e3", "q3", "new"), '\n')); err != nil { + t.Fatal(err) + } + f.Close() + if n := sentinelCount(t, e, "answers"); n != 0 { + t.Fatalf("sentinel rows after the store changed = %d, want 0: the open session kept reading the stale Parquet", n) + } + if n := queryInt(t, e, "SELECT count(*) FROM answers"); n != 3 { + t.Fatalf("answers count = %d, want the 3 raw rows", n) + } + if warn.Len() != 0 { + t.Fatalf("going stale must be silent in a session too, got %q", warn.String()) + } +} + +func TestMaterializedViewFallsBackWhenUnreadable(t *testing.T) { + for _, tc := range []struct { + name string + want string + damage func(t *testing.T, path string) + }{ + {"deleted", "is missing", func(t *testing.T, path string) { + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + }}, + {"corrupt", "is unreadable", func(t *testing.T, path string) { + if err := os.WriteFile(path, []byte("not a parquet file"), 0o600); err != nil { + t.Fatal(err) + } + }}, + {"truncated", "is unreadable", func(t *testing.T, path string) { + body, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, body[:len(body)/2], 0o600); err != nil { + t.Fatal(err) + } + }}, + } { + t.Run(tc.name, func(t *testing.T) { + d, _, storeFile := answersFixture(t, "ds") + plantAnswersSentinel(t, d, storeFile) + path := d.Path(filepath.Join(dataset.MaterializedDir, "answers.parquet")) + // The fingerprint covers the inputs, not the Parquet, so the entry + // stays fresh and the damage has to be caught at registration. + tc.damage(t, path) + + var warn bytes.Buffer + e, err := Open(context.Background(), []DatasetSpec{{DS: d}}, nil, &warn) + if err != nil { + t.Fatal(err) + } + defer e.Close() + if n := queryInt(t, e, "SELECT count(*) FROM answers"); n != 2 { + t.Fatalf("answers count = %d, want the 2 raw rows", n) + } + if !bytes.Contains(warn.Bytes(), []byte(tc.want)) { + t.Fatalf("want a warning saying %q, got %q", tc.want, warn.String()) + } + // A deleted folder is the documented action, so its warning is one + // line per view rather than the engine's error with a path and SQL. + if tc.name == "deleted" && bytes.Count(warn.Bytes(), []byte("\n")) != 1 { + t.Fatalf("a missing Parquet should warn in one line, got %q", warn.String()) + } + }) + } +} + +// TestMaterializedViewSurvivesRename pins the property whose absence disqualified +// a persistent DuckDB: the manifest records dataset-relative paths, so the folder +// moves with the dataset. +func TestMaterializedViewSurvivesRename(t *testing.T) { + d, root, storeFile := answersFixture(t, "ds") + plantAnswersSentinel(t, d, storeFile) + + renamed, err := d.Rename(root, "moved") + if err != nil { + t.Fatal(err) + } + e := openEngine(t, []DatasetSpec{{DS: renamed}}, nil) + if n := sentinelCount(t, e, "answers"); n != 1 { + t.Fatalf("sentinel rows after rename = %d, want 1", n) + } +} + +func TestMaterializedViewResolvesUnderItsSchema(t *testing.T) { + alpha, _, storeFile := answersFixture(t, "alpha") + plantAnswersSentinel(t, alpha, storeFile) + beta, _, _ := answersFixture(t, "beta") + + e := openEngine(t, []DatasetSpec{{DS: alpha}, {DS: beta}}, nil) + if n := sentinelCount(t, e, `"alpha"."answers"`); n != 1 { + t.Fatalf("alpha sentinel rows = %d, want 1", n) + } + if n := sentinelCount(t, e, `"beta"."answers"`); n != 0 { + t.Fatalf("beta sentinel rows = %d, want 0: only alpha is materialized", n) + } +} + +func TestMaterializableViews(t *testing.T) { + d := newDS(t, "ds") + buildStore(t, d, 584, [][]byte{answerRec("s", "e1", "q1", "hi")}) + addReportCSV(t, d, 584, "answers", "student_id,res_1_q1_answer\nPrompt,What?\nCorrect answer,42\n1,hello\n") + addDimensionCSV(t, d, dimFixture{run: 700, slug: "student-id-mapping", fetchedAt: time.Unix(1, 0).UTC(), + csv: mappingCSV(mappingRow(1, 30, endpointAAA))}) + + got := materializableViews(mustManifest(t, d)) + sort.Strings(got) + want := []string{"answers", "report_prompts", "reports", "run_membership", "student_id_mapping"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("materializableViews = %v, want %v", got, want) + } +} + +// TestMaterializableViewsExcludeTheAttachmentViews pins the exclusion to the +// names. The attachment views declare the files they read like any other view, +// so nothing about their construction keeps them out of the set. +func TestMaterializableViewsExcludeTheAttachmentViews(t *testing.T) { + d := newDS(t, "att") + addAttachment(t, d, dataset.AttachmentFile{ + ID12: "aaa", Name: "file.json", Source: "activity-player.concord.org", + PublicPath: "ia/f1/s1/file.json", ContentType: "application/json", Size: 7, + File: "attachments/aaa_file.json", State: true, + }, []byte(`{"v":1}`)) + + vs := viewSet{m: mustManifest(t, d)} + declared := map[string]bool{} + for _, st := range vs.statements() { + if len(st.files) > 0 { + declared[bareViewName(st.name, vs.prefix)] = true + } + } + for _, view := range []string{"attachment_states", "attachment_content"} { + if !declared[view] { + t.Fatalf("%s declares no files, so this test cannot tell the name exclusion from an accident", view) + } + } + got := materializableViews(mustManifest(t, d)) + for _, view := range got { + if neverMaterialized[view] { + t.Fatalf("%s must never be materializable, got %v", view, got) + } + } +} + +// TestMaterializableViewsExcludesEmptyDeclarations covers the other half of the +// predicate: over an empty manifest every builder returns its stand-in, which +// scans nothing, so nothing is materializable. +func TestMaterializableViewsExcludesEmptyDeclarations(t *testing.T) { + if got := materializableViews(&dataset.Manifest{}); len(got) != 0 { + t.Fatalf("materializableViews over an empty manifest = %v, want none", got) + } +} diff --git a/internal/duck/views.go b/internal/duck/views.go index 9eef8f7..f7e85bc 100644 --- a/internal/duck/views.go +++ b/internal/duck/views.go @@ -1,6 +1,8 @@ package duck import ( + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "io" @@ -33,10 +35,18 @@ var membershipColumns = map[string]string{ // viewStmt is one view with its primary SQL and a typed-empty fallback. type viewStmt struct { - name string - primary string - fallback string - files []string // files the primary reads (for the degradation warning) + name string + materialized string // optional read_parquet form, tried before primary + primary string + fallback string + files []string // files the primary reads (for the degradation warning) + + // Set alongside materialized: the file it reads, the manifest entry that + // names it, and the signature of the definition the entry was checked + // against, so a session can re-check the copy's freshness after it opens. + parquet string + record dataset.Materialized + signature string } // viewSet builds the view statements for one dataset under a schema prefix. @@ -392,7 +402,7 @@ func (vs viewSet) attachmentFilesView() viewStmt { // attachmentStatesView exposes offloaded state files as raw text plus TRY_CAST JSON. func (vs viewSet) attachmentStatesView() viewStmt { name := vs.prefix + `"attachment_states"` - var members []string + var members, files []string for _, af := range vs.m.Attachments { if !af.State { continue @@ -400,6 +410,7 @@ func (vs viewSet) attachmentStatesView() viewStmt { members = append(members, fmt.Sprintf( "SELECT %s AS id12, %s AS name, filename, content FROM read_text(%s)", sqlStr(af.ID12), sqlStr(af.Name), vs.file(af.File))) + files = append(files, af.File) } fallback := fmt.Sprintf("CREATE VIEW %s AS SELECT CAST(NULL AS VARCHAR) AS filename, CAST(NULL AS VARCHAR) AS id12, CAST(NULL AS VARCHAR) AS name, CAST(NULL AS VARCHAR) AS content, CAST(NULL AS JSON) AS state WHERE false", name) if len(members) == 0 { @@ -407,7 +418,7 @@ func (vs viewSet) attachmentStatesView() viewStmt { } inner := strings.Join(members, "\nUNION ALL BY NAME\n") primary := fmt.Sprintf("CREATE VIEW %s AS SELECT filename, id12, name, content, TRY_CAST(content AS JSON) AS state FROM (%s)", name, inner) - return viewStmt{name: name, primary: primary, fallback: fallback} + return viewStmt{name: name, primary: primary, fallback: fallback, files: files} } // attachmentContentView exposes the text content of every downloaded attachment @@ -417,7 +428,7 @@ func (vs viewSet) attachmentStatesView() viewStmt { // attachment_states stays as the narrow current-answer view for back-compat. func (vs viewSet) attachmentContentView() viewStmt { name := vs.prefix + `"attachment_content"` - var members []string + var members, files []string for _, af := range vs.m.Attachments { // read_text requires valid UTF-8, so binary attachments (audio, images) // cannot be exposed as text; they remain downloadable via attachment_files. @@ -428,6 +439,7 @@ func (vs viewSet) attachmentContentView() viewStmt { members = append(members, fmt.Sprintf( "SELECT %s AS id12, %s AS name, %s AS source, %s AS public_path, filename, content FROM read_text(%s)", sqlStr(af.ID12), sqlStr(af.Name), sqlStr(af.Source), sqlStr(af.PublicPath), vs.file(af.File))) + files = append(files, af.File) } fallback := fmt.Sprintf("CREATE VIEW %s AS SELECT CAST(NULL AS VARCHAR) AS id12, CAST(NULL AS VARCHAR) AS name, CAST(NULL AS VARCHAR) AS source, CAST(NULL AS VARCHAR) AS public_path, CAST(NULL AS VARCHAR) AS filename, CAST(NULL AS VARCHAR) AS content, CAST(NULL AS JSON) AS state WHERE false", name) if len(members) == 0 { @@ -435,7 +447,7 @@ func (vs viewSet) attachmentContentView() viewStmt { } inner := strings.Join(members, "\nUNION ALL BY NAME\n") primary := fmt.Sprintf("CREATE VIEW %s AS SELECT id12, name, source, public_path, filename, content, TRY_CAST(content AS JSON) AS state FROM (%s)", name, inner) - return viewStmt{name: name, primary: primary, fallback: fallback} + return viewStmt{name: name, primary: primary, fallback: fallback, files: files} } // perDownloadViews builds report_, answers_, history_, and @@ -591,6 +603,100 @@ func StaticViewNames() []string { return names } +// applyMaterialized points a view at its Parquet when the manifest records one +// that is fresh. A stale entry is silent by contract; a missing or unreadable +// one is not, and the registration loop is what tells those apart. +func (vs viewSet) applyMaterialized(stmts []viewStmt) []viewStmt { + if len(vs.m.Materialized) == 0 { + return stmts + } + 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] + signature := viewSignature(st, vs.canonDir, vs.prefix) + if !ok || !mat.Fresh(vs.canonDir, st.files, signature) { + continue + } + stmts[i].materialized = fmt.Sprintf("CREATE VIEW %s AS SELECT * FROM read_parquet(%s)", + st.name, vs.file(mat.File)) + stmts[i].parquet = filepath.Join(vs.canonDir, mat.File) + stmts[i].record = mat + stmts[i].signature = signature + } + return stmts +} + +// materializableViews returns the views of this dataset that can be +// materialized: those that scan files, minus the per-run views and the two +// attachment views. The first two 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) +} + +// neverMaterialized names the views kept out of the set by hand. The attachment +// views read_text every attachment, so materializing them would copy the +// attachment corpus a second time for the two views with the largest output. +var neverMaterialized = map[string]bool{"attachment_states": true, "attachment_content": true} + +// 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] && !neverMaterialized[bare] { + out = append(out, bare) + } + } + return out +} + +// viewSignature identifies the definition a Parquet was built from. Two things +// are neutralized first, because neither changes what the view returns: the +// dataset directory, since the statement embeds absolute paths and a +// materialized view has to survive a rename, and the schema prefix, since the +// same dataset registers unprefixed alone and prefixed in a multi-dataset +// session. +func viewSignature(st viewStmt, canonDir, prefix string) string { + sql := st.primary + if prefix != "" { + sql = strings.ReplaceAll(sql, prefix, "") + } + if canonDir != "" { + sql = strings.ReplaceAll(sql, canonDir, "") + } + sum := sha256.Sum256([]byte(sql)) + return hex.EncodeToString(sum[:]) +} + +// 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 goes through it. +func bareViewName(name, prefix string) string { + return strings.Trim(strings.TrimPrefix(name, prefix), `"`) +} + // IdentityColumnNames returns the columns that identify a record across the stores, derived // from the membership key so the drift guard's inventory side stays code-derived. Deliberately // not contractStoreColumns, which also carries the internal _fetched_at/_run_id bookkeeping. diff --git a/internal/guidance/src/core.md b/internal/guidance/src/core.md index a69d246..1e3244f 100644 --- a/internal/guidance/src/core.md +++ b/internal/guidance/src/core.md @@ -186,6 +186,31 @@ its own schema. There are no implicit cross-dataset unions — write them explicitly with provenance, e.g. `SELECT * FROM fall_2026.answers UNION ALL BY NAME SELECT * FROM spring_2027.answers`. +## Materializing a dataset + +Every query re-reads the raw JSONL and CSV a fetch produced, which is right for a +few thousand answers and slow once a dataset reaches gigabytes. Materializing +writes each view to a Parquet file that queries read instead, while it is fresh. + +- It is a speed decision, never a correctness one. A materialized view returns + exactly what the raw artifacts return, and a view whose inputs have changed + since the copy falls back to them silently, with no error and nothing for the + user to do. +- It is worth doing once a dataset is large and the same questions are being + asked repeatedly, and it is worth redoing after a fetch that changes the data + a view reads. It buys most on aggregates over a few columns and least on + queries that read a wide JSON blob or take an unfiltered `LIMIT`. +- The Parquet lands at a fixed path inside the dataset folder, so analysis code + outside cc-data can open it directly rather than going through a query. +- The folder is safe for the researcher to delete at any time, and cc-data itself + removes a finished Parquet only when purging or deleting the whole dataset, so + an external script may rely on the path. The one thing a materialize run does + clear is the half-written file a previous run left behind if it was + interrupted. +- A dataset with only some of its views materialized is an ordinary state, not a + fault: freshness is per view, and a view can also be refused when a file it + reads is missing from disk. + ## Sensitive data Datasets hold sensitive student data. You may auto-read a dataset's summary; do diff --git a/internal/guidance/src/skill_header.md b/internal/guidance/src/skill_header.md index dc7888e..8f8d019 100644 --- a/internal/guidance/src/skill_header.md +++ b/internal/guidance/src/skill_header.md @@ -21,6 +21,12 @@ guessing flags. - Delete a dataset's data with `cc-data dataset purge `. - `--dataset` is repeatable on `query`/`repl`, which is how a query spans more than one dataset. +- Speed up a large dataset's queries with `cc-data dataset materialize `, + which writes each view to `materialized/.parquet` inside the dataset + folder. `--force` rebuilds an unchanged view; `--allow-partial` builds a view + whose declared file is missing from disk. It exits non-zero if any view was + refused, and `cc-data dataset show ` names a view whose copy has gone + stale. ## Auth diff --git a/internal/guidance/src/tools.md b/internal/guidance/src/tools.md index 1c37fcd..a49a259 100644 --- a/internal/guidance/src/tools.md +++ b/internal/guidance/src/tools.md @@ -30,5 +30,9 @@ - `dataset_purge` — a dataset's downloaded data, keeping the dataset itself. - `dataset_reindex` — rebuilds a dataset's manifest from what is on disk, for a dataset whose summary looks wrong. +- `dataset_materialize` — writes each of a dataset's file-backed views to a + Parquet file that queries read instead of the raw JSONL and CSV while the + copy is current. It never changes an answer, so it is a speed decision, not a + correctness one. - `query` — SQL over one or more datasets, which is how every question about the data gets answered. diff --git a/internal/mcpserver/server_test.go b/internal/mcpserver/server_test.go index 98a669d..43da7a5 100644 --- a/internal/mcpserver/server_test.go +++ b/internal/mcpserver/server_test.go @@ -108,7 +108,7 @@ func TestMCPToolSurface(t *testing.T) { "reports_filter_options", "reports_create", "reports_duplicate", "get_report", "get_answers", "get_history", "get_attachments", "dataset_create", "dataset_list", "dataset_show", "dataset_rename", "dataset_edit", "dataset_delete", "dataset_purge", - "dataset_reindex", "query"} + "dataset_reindex", "dataset_materialize", "query"} for _, n := range want { if _, ok := names[n]; !ok { t.Fatalf("missing tool %q", n) @@ -488,9 +488,9 @@ func TestMCPDatasetShowParity(t *testing.T) { _, out := callJSON(t, cs, "dataset_show", map[string]any{"ref": "learn.concord.org/ds"}) - // The tool payload must equal the CLI's BuildShowJSON for the same dataset. + // The tool payload must equal the CLI's ShowJSON for the same dataset. d := dataset.Open(root, dataset.Ref{Portal: config.MustPortal("learn.concord.org"), Name: "ds"}) - cliJSON, _ := d.BuildShowJSON(false) + cliJSON, _ := duck.ShowJSON(d, false) cliBytes, _ := json.Marshal(cliJSON) toolBytes, _ := json.Marshal(out) // Re-normalize both through generic maps for a stable compare. diff --git a/internal/mcpserver/tools.go b/internal/mcpserver/tools.go index 422c399..d501b95 100644 --- a/internal/mcpserver/tools.go +++ b/internal/mcpserver/tools.go @@ -190,7 +190,7 @@ func registerTools(s *mcp.Server, opts Options) { if err != nil { return nil, dataset.ShowJSON{}, err } - s, err := d.BuildShowJSON(in.Full) + s, err := duck.ShowJSON(d, in.Full) if err != nil { return nil, dataset.ShowJSON{}, err } @@ -269,6 +269,26 @@ func registerTools(s *mcp.Server, opts Options) { return nil, mapOut{"ref": d.Ref.String(), "reindexed": true}, nil }) + // No hints, so a client applies the MCP default and treats it as + // destructive, which is right: an allow_partial run replaces a complete + // Parquet with a shorter one at a path scripts outside cc-data read. + addTool(s, &mcp.Tool{Name: "dataset_materialize", Description: "Write each of a dataset's file-backed views to " + dataset.MaterializedDir + "/.parquet, so queries over a large dataset stop re-parsing the raw JSONL and CSV. Queries read a view's Parquet while it is fresh and fall back to the raw artifacts otherwise, so this never changes an answer. Set force=true to rebuild a view whose inputs are unchanged, and allow_partial=true to build a view even though a file it declares is missing. Returns one outcome per view: written, fresh (its copy was already current), discarded (a concurrent change moved its inputs, so running again picks it up), or refused, which carries the reason. A written or fresh view also carries a reason when its copy was built from fewer inputs than it declares or reads a download marked incomplete. Views are refused rather than published when an input is missing or the view could not be read at all."}, + func(ctx context.Context, req *mcp.CallToolRequest, in datasetMaterializeIn) (*mcp.CallToolResult, mapOut, error) { + d, _, err := openDataset(in.Ref) + if err != nil { + return nil, nil, err + } + opts := duck.MaterializeOptions{Force: in.Force, AllowPartial: in.AllowPartial} + res, err := duck.Materialize(ctx, d, opts, newProgress(ctx, req)) + if err != nil { + return nil, nil, err + } + // The outcome list goes back as it stands rather than as a set of + // per-status keys assembled here, which would have to be extended by + // hand every time a status is added. + return nil, mapOut{"ref": d.Ref.String(), "views": res.Views}, nil + }) + addTool(s, &mcp.Tool{Name: "query", Description: queryDescription(), Annotations: readOnly}, queryHandler(opts)) } diff --git a/internal/mcpserver/types.go b/internal/mcpserver/types.go index fcb34a8..fdf290f 100644 --- a/internal/mcpserver/types.go +++ b/internal/mcpserver/types.go @@ -101,6 +101,12 @@ type datasetRefIn struct { Ref string `json:"ref"` } +type datasetMaterializeIn struct { + Ref string `json:"ref"` + Force bool `json:"force,omitempty"` + AllowPartial bool `json:"allow_partial,omitempty"` +} + type confirmRefIn struct { Ref string `json:"ref"` Confirm bool `json:"confirm,omitempty"` diff --git a/internal/store/lock.go b/internal/store/lock.go index 83c9c6f..aa6f1f0 100644 --- a/internal/store/lock.go +++ b/internal/store/lock.go @@ -3,19 +3,43 @@ package store import ( + "context" "os" "path/filepath" "sync" + "time" "github.com/gofrs/flock" ) +// lockRetry is how often a waiting acquisition re-tries its guard. +const lockRetry = 100 * time.Millisecond + +// waitFor re-tries a non-blocking acquisition until it succeeds or the context +// ends. Both guards wait this way rather than through the flock's own blocking +// call so that a wait can be abandoned, and so the in-process bookkeeping each +// TryLock does is the only bookkeeping there is. +func waitFor(ctx context.Context, try func() (bool, error)) error { + for { + ok, err := try() + if err != nil || ok { + return err + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(lockRetry): + } + } +} + // Lock file names within a dataset directory. None is ever renamed or unlinked, // because unlinking a held flock file detaches it from its inode and a rename // detaches held flocks onto a dead inode. const ( - DatasetLockFile = ".dataset.lock" - ActivityLockFile = ".activity.lock" + DatasetLockFile = ".dataset.lock" + ActivityLockFile = ".activity.lock" + MaterializeLockFile = ".materialize.lock" ) // Each lock is a process-wide guard per path: a sync primitive for goroutine @@ -27,6 +51,7 @@ var ( dsGuards = map[string]*DatasetLock{} actGuards = map[string]*ActivityLock{} dlGuards = map[string]*DownloadLock{} + matGuards = map[string]*DatasetLock{} ) // DatasetLockFor returns the process-wide per-dataset guard for a dataset dir. @@ -56,6 +81,24 @@ func ActivityLockFor(datasetDir string) *ActivityLock { return g } +// MaterializeLockFor returns the process-wide guard a materialize run holds for +// its whole duration. It is separate from the mutation guards because a +// materialize deliberately copies outside those, so that a fetch is never +// blocked by one; holding this guard for the whole run is what makes a temp file +// found at the start provably the work of a run that is no longer alive, since +// the kernel drops an flock when its holder dies however abruptly. +func MaterializeLockFor(datasetDir string) *DatasetLock { + key := filepath.Clean(datasetDir) + registryMu.Lock() + defer registryMu.Unlock() + if g, ok := matGuards[key]; ok { + return g + } + g := &DatasetLock{flock: flock.New(filepath.Join(key, MaterializeLockFile))} + matGuards[key] = g + return g +} + // DownloadLockFor returns the process-wide per-download guard for a lock file // (seg__.lock and friends) under a dataset's segments directory. // Unlike the dataset-keyed guards, download guards are keyed per-run, so the @@ -148,6 +191,9 @@ func (l *DatasetLock) Lock() error { return nil } +// LockContext waits for the guard until the context ends. +func (l *DatasetLock) LockContext(ctx context.Context) error { return waitFor(ctx, l.TryLock) } + // TryLock acquires the guard without blocking; the bool reports success. func (l *DatasetLock) TryLock() (bool, error) { if !l.mu.TryLock() { @@ -215,6 +261,9 @@ func (l *ActivityLock) RUnlock() { } } +// LockContext waits for the exclusive lock until the context ends. +func (l *ActivityLock) LockContext(ctx context.Context) error { return waitFor(ctx, l.TryLock) } + // TryLock acquires the exclusive lock without blocking. func (l *ActivityLock) TryLock() (bool, error) { l.mu.Lock() diff --git a/specs/REPORT-113-dataset-materialization.md b/specs/REPORT-113-dataset-materialization.md new file mode 100644 index 0000000..accb69f --- /dev/null +++ b/specs/REPORT-113-dataset-materialization.md @@ -0,0 +1,455 @@ +# Dataset materialization (Parquet query surface) + +**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-113 + +**Status**: **Closed** + +## 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. + +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 the study code owns: the Dataflow study maintained 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 did not offer one. + +## 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 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 exclude the manifest-derived VALUES tables (`downloads`, `attachment_files`) and the `read_text` attachment views (`attachment_states`, `attachment_content`). +- 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. When overridden, the run reports how many of the declared inputs it built from. +- 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. A discarded view 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. 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. +- A download marked `!Complete` is a caveat on the view's outcome naming the runs, not a refusal, and `--allow-partial` does not gate it. A copy built with `--allow-partial` carries the same kind of caveat, saying how many of its declared inputs it used. Both ride on the outcome rather than on progress output because the MCP tool returns the outcome and may have nowhere to send progress. +- The view-to-download mapping the caveat needs is three rules, because no single rule covers it. Report-backed views map by file intersection. `answers` and `history` map by download `Type`, since the store is one merged file across every run. `run_membership` maps by `Type` too, since an incomplete run has no membership file for a per-run mapping to find. +- The refusal names both remedies for a missing input: 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. +- 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. The union views get no such check: 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. 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 signature of the view definition it was built from, and the view's Parquet is used only while every input still matches and the definition still agrees. Otherwise the view is registered from its raw artifacts, with no error and no user action required. +- The signature covers what the input fingerprints cannot see. A cc-data release that changes a view's projection, derived columns or ordering leaves every input byte untouched, as does a reindex that re-stamps a zero `FetchedAt` and so reorders a dimension view's dedupe. It is taken over the view's statement with the dataset directory and the schema prefix neutralized, because a materialized view has to survive a rename and has to resolve the same alone as in a multi-dataset session. +- A recorded view is only skipped as fresh when its Parquet is still readable. The folder is documented as safe to delete at any time, so without that check a deleted or truncated copy reads as fresh forever while queries quietly fall back to the raw artifacts, repairable only through `--force`. +- 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 compares like any other fingerprint, so an input absent at build time and absent still reads as fresh, while one that has since appeared or gone missing reads as stale. +- A Parquet the manifest names but that is absent on disk falls back to the raw artifacts, with a one-line warning per view; only a Parquet that is present but unreadable carries the engine's error. `dataset show` names the views whose recorded Parquet is missing. +- A session that outlives a fetch re-checks each Parquet-backed view's freshness on every query and puts a stale one back on its raw artifacts, so a repl answers the same as a session opened after the fetch would. +- 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. The repoint waits for a `get` that is still running rather than failing busy and deleting every finished copy; the wait ends only with the caller's context, which the CLI cancels on interrupt. It is the only waiting acquisition, since rename, delete and purge take the materialize guard while holding the mutation locks and would deadlock against it if they waited too. +- A copy whose rename into place fails is refused on its own; the views renamed before it still reach the manifest. +- 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 fails as busy. `get` never takes this guard. `dataset rename`, `dataset delete` and `dataset purge` do take it, because the guard's lock file and the run's open copies live inside the directory they move or remove and Windows cannot rename or remove a directory with an open handle inside it; without that coupling a rename would fail after having already rewritten the manifest name. Because the kernel drops an `flock` when its holder dies however abruptly, any temp file present 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, which is the whole answer to an interrupted run. +- `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/`, and `Purge` clears `m.Materialized` alongside the four maps it already resets. +- `dataset reindex` drops the manifest's materialization entries rather than carrying them forward, and never deletes `materialized/`. Nothing except purge and delete removes it. The derived-subfolder contract is "never adopted, never reported as orphans, removed wholesale by purge and delete". +- `dataset show` names any Parquet in `materialized/` that no manifest entry references, which is the state a reindex leaves. The message states what cc-data can and cannot vouch for rather than telling the researcher to clear it. +- 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. +- 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. +- A materialized view survives `dataset rename`, and resolves correctly in a multi-dataset session under its schema prefix. Both asserted by tests. +- 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. +- `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 a finished Parquet only through `dataset purge` and `dataset delete`. The two sentences sit together, because the first alone reads like a warning that the tool might delete it. *(Amended during implementation: the sweep clears a half-written file from an interrupted run, so the promise is stated for a finished Parquet rather than for everything in the folder.)* +- The researcher guide documents the `materialized/.parquet` layout as a stable path external tools may read directly. +- `materialize` has no view-selection flag. `materialized/` is a deterministic mirror of the materializable set, which is what lets an external script treat a path as a contract. +- The researcher guide states the two measured duplication costs, so the folder size is expected rather than discovered. +- 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 a missing input 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 typed-empty view; the store row-count assertion; `Fresh` returning false once a recorded input is deleted; `--force` rebuilding an unchanged view; reindex dropping the entries with the files 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. 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`, reporting `v1.5.4`), since the sandbox's `enable_external_access = false` made it an open question whether any of this was reachable: + +- `COPY (