From 550831d0885d7504602cdebb09b2a014d02e213b Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 08:47:41 -0600 Subject: [PATCH 1/4] getBuildTargets reports a source it cannot compile instead of failing the plan A persist source whose SQL raises a MalloyCompileError (a given with no default, a virtual source with no map entry), and anything that inlines it, comes back in BuildTargets.unbuildable with the compiler's problem; the rest of the model is planned. Any other error still throws. Signed-off-by: James Estes --- packages/malloy/src/api/CONTEXT.md | 2 +- .../src/api/foundation/build-targets.spec.ts | 103 +++++++++++++++++- .../src/api/foundation/build_targets.ts | 39 ++++++- packages/malloy/src/api/foundation/index.ts | 1 + packages/malloy/src/api/foundation/runtime.ts | 15 ++- packages/malloy/src/api/foundation/types.ts | 18 +++ packages/malloy/src/doc/persist/api.md | 16 ++- packages/malloy/src/doc/persist/internal.md | 10 +- packages/malloy/src/index.ts | 1 + scripts/simple_builder/build.ts | 14 ++- 10 files changed, 194 insertions(+), 25 deletions(-) diff --git a/packages/malloy/src/api/CONTEXT.md b/packages/malloy/src/api/CONTEXT.md index 6fae35c58..aecfc1022 100644 --- a/packages/malloy/src/api/CONTEXT.md +++ b/packages/malloy/src/api/CONTEXT.md @@ -261,7 +261,7 @@ API-level documentation lives in the JSDoc on `ModelMaterializer.loadRestrictedQ `getPreparedResult()` / `getSQL()` / `run()` still throw `MalloyError` on failure; the thrown `.problems` is the same array. A `validate()` followed by a no-options consumer reuses one cached compile via `QueryMaterializer._compileAttempt`. -SQL-compile errors that are user-actionable throw `MalloyCompileError` (`packages/malloy/src/model/malloy_compile_error.ts`) at the throw site, carrying `{message, code, at?}`. The materializer's `_compileAndCollect` helper translates it to a `LogMessage`. Invariant violations stay as bare `Error` and surface with `code: 'compiler-bug'`. +SQL-compile errors that are user-actionable throw `MalloyCompileError` (`packages/malloy/src/model/malloy_compile_error.ts`) at the throw site, carrying `{message, code, at?}`. The materializer's `_compileAndCollect` helper translates it to a `LogMessage`, and `mkBuildTargets` does the same per source, reporting the source in `BuildTargets.unbuildable`. Invariant violations stay as bare `Error` and surface with `code: 'compiler-bug'`. API-level documentation lives in the JSDoc on the two `validate` methods. diff --git a/packages/malloy/src/api/foundation/build-targets.spec.ts b/packages/malloy/src/api/foundation/build-targets.spec.ts index 81b1c7fe9..3b264c10d 100644 --- a/packages/malloy/src/api/foundation/build-targets.spec.ts +++ b/packages/malloy/src/api/foundation/build-targets.spec.ts @@ -3,9 +3,10 @@ * SPDX-License-Identifier: MIT */ +import type {PersistSource} from './core'; import {Model} from './core'; import {mkBuildTargets, resolvePersistWalk} from './build_targets'; -import type {BuildTarget, ConnectionBuild} from './types'; +import type {BuildTarget, ConnectionBuild, UnbuildableSource} from './types'; import type {LogMessage} from '../../lang'; import {TestTranslator} from '../../lang/test/test-translator'; @@ -18,8 +19,10 @@ const DIGESTS = { // up once the SQL has been hashed. These tests take the long way — a real // translate, a real getSQL() — because hand-built input could not tell the // difference between two sources that share a table and two that don't. -function modelOf(src: string): Model { - const tt = new TestTranslator(`##! experimental.persistence\n${src}`); +function modelOf(src: string, compilerFlags: string[] = []): Model { + const tt = new TestTranslator(`##! experimental.persistence\n${src}`, { + compilerFlags, + }); const compiled = tt.translate(); if (!compiled.modelDef) { const problems = (compiled.problems ?? []).map(p => p.message).join('\n'); @@ -29,9 +32,23 @@ function modelOf(src: string): Model { } function connectionsOf(src: string): ConnectionBuild[] { - const model = modelOf(src); + return planOf(src).connections; +} + +function planOf( + src: string, + compilerFlags: string[] = [] +): {connections: ConnectionBuild[]; unbuildable: UnbuildableSource[]} { + const model = modelOf(src, compilerFlags); const log: LogMessage[] = []; - return mkBuildTargets(resolvePersistWalk(model, log), DIGESTS, {}); + const unbuildable: UnbuildableSource[] = []; + const connections = mkBuildTargets( + resolvePersistWalk(model, log), + DIGESTS, + {}, + unbuildable + ); + return {connections, unbuildable}; } function connectionNamed( @@ -295,7 +312,7 @@ describe('what a target carries', () => { const model = modelOf(`${ROLLUP}\nrun: rollup -> { select: * }`); const walk = resolvePersistWalk(model, []); - expect(() => mkBuildTargets(walk, {}, {})).toThrow('_db_'); + expect(() => mkBuildTargets(walk, {}, {}, [])).toThrow('_db_'); }); test('a model with no persist sources has no targets', () => { @@ -307,3 +324,77 @@ describe('what a target carries', () => { expect(targets).toEqual([]); }); }); + +describe('a source whose SQL does not compile', () => { + // A given with no default has no value at plan time, so the SQL that keys + // the table cannot be written. That is a fact about one source, not about + // the model. + const SCOPED = ` + given: TENANT :: string + #@ persist name=scoped + source: scoped is _db_.table('aTable') -> { + where: astr = $TENANT + group_by: astr + } + `; + const GIVENS = ['experimental.givens']; + + test('is reported, and the rest of the model still builds', () => { + const {connections, unbuildable} = planOf( + ` + ${ROLLUP} + ${SCOPED} + run: rollup -> { select: * } + run: scoped -> { select: * } + `, + GIVENS + ); + + expect(connections.flatMap(c => c.targets).map(names)).toEqual([ + ['rollup'], + ]); + expect(unbuildable.map(u => u.source.name)).toEqual(['scoped']); + expect(unbuildable[0].problem.severity).toBe('error'); + expect(unbuildable[0].problem.message).toContain('TENANT'); + }); + + test('makes what reads it unbuildable too', () => { + // A dependent's SQL inlines it, so cannot be written either. + const {connections, unbuildable} = planOf( + ` + ${SCOPED} + #@ persist name=top + source: top is scoped -> { group_by: astr } + run: top -> { select: * } + `, + GIVENS + ); + + expect(connections).toEqual([]); + expect(unbuildable.map(u => u.source.name)).toEqual(['scoped', 'top']); + }); + + test('a compiler bug is still thrown', () => { + // Only a MalloyCompileError is a fact about the source. Anything else is + // the compiler's own failure, and hiding it as an unbuildable source would + // hide the bug. + const broken = { + sourceID: 'broken@test', + name: 'broken', + connectionName: '_db_', + getSQL: () => { + throw new Error('invariant violated'); + }, + } as unknown as PersistSource; + const walk = [ + { + node: {sourceID: 'broken@test', persistent: true, dependsOn: []}, + source: broken, + }, + ]; + + expect(() => mkBuildTargets(walk, DIGESTS, {}, [])).toThrow( + 'invariant violated' + ); + }); +}); diff --git a/packages/malloy/src/api/foundation/build_targets.ts b/packages/malloy/src/api/foundation/build_targets.ts index 62c6067be..1a3cdf38a 100644 --- a/packages/malloy/src/api/foundation/build_targets.ts +++ b/packages/malloy/src/api/foundation/build_targets.ts @@ -4,10 +4,11 @@ */ import type {BuildIdOptions, SourceID} from '../../model'; +import {MalloyCompileError} from '../../model'; import type {LogMessage} from '../../lang'; import type {PersistNode} from '../../model/persist_utils'; import type {Model, PersistSource} from './core'; -import type {BuildTarget, ConnectionBuild} from './types'; +import type {BuildTarget, ConnectionBuild, UnbuildableSource} from './types'; /** * Read a key this function's own construction guarantees is present. @@ -60,6 +61,30 @@ export function resolvePersistWalk( return resolved; } +/** + * The SQL that keys `source`'s table, or undefined when it does not compile on + * its own — the source is then reported in `unbuildable` instead. + * + * Only a `MalloyCompileError` is about the source; anything else is the + * compiler's own failure and must not pass for one. + */ +function buildIdSQL( + source: PersistSource, + buildIdOpts: BuildIdOptions, + unbuildable: UnbuildableSource[] +): string | undefined { + try { + return source.getSQL(buildIdOpts); + } catch (e) { + if (!(e instanceof MalloyCompileError)) throw e; + unbuildable.push({ + source, + problem: {message: e.message, severity: 'error', code: e.code, at: e.at}, + }); + return undefined; + } +} + /** * A target under construction. `dependsOn` is held as keys until every target * exists, then rewritten as references. @@ -115,11 +140,14 @@ function addUnique(into: string[], seen: Set, keys: string[]): void { * recomputes the same key at serve time through `buildIdOptions()`; supply a * different `virtualMap` here and the table is built under a key nothing * looks up. + * @param unbuildable Receives each source whose SQL does not compile, which + * becomes no target. See `UnbuildableSource`. */ export function mkBuildTargets( walk: ResolvedNode[], connectionDigests: Record, - buildIdOpts: BuildIdOptions + buildIdOpts: BuildIdOptions, + unbuildable: UnbuildableSource[] ): ConnectionBuild[] { const targets = new Map(); // What each source hands to whoever referenced it: its own target if it is @@ -150,7 +178,12 @@ export function mkBuildTargets( ); } - const sql = source.getSQL(buildIdOpts); + const sql = buildIdSQL(source, buildIdOpts, unbuildable); + if (sql === undefined) { + // Not a table either. Whatever reads it inlines it, so is unbuildable too. + contributes.set(node.sourceID, childKeys); + continue; + } const buildId = source.makeBuildId(digest, sql); const key = targetKey(connectionName, buildId); diff --git a/packages/malloy/src/api/foundation/index.ts b/packages/malloy/src/api/foundation/index.ts index cfcb64ea3..a02574152 100644 --- a/packages/malloy/src/api/foundation/index.ts +++ b/packages/malloy/src/api/foundation/index.ts @@ -15,6 +15,7 @@ export type { BuildTarget, BuildTargets, ConnectionBuild, + UnbuildableSource, } from './types'; export {EMPTY_BUILD_MANIFEST} from './types'; diff --git a/packages/malloy/src/api/foundation/runtime.ts b/packages/malloy/src/api/foundation/runtime.ts index 462605232..11c637246 100644 --- a/packages/malloy/src/api/foundation/runtime.ts +++ b/packages/malloy/src/api/foundation/runtime.ts @@ -32,6 +32,7 @@ import type { CompileOptions, CompileQueryOptions, BuildTargets, + UnbuildableSource, } from './types'; import {mkBuildTargets, resolvePersistWalk} from './build_targets'; import type {PreparedResult, Explore} from './core'; @@ -678,12 +679,14 @@ export class Runtime { } } - return { - connections: mkBuildTargets(walk, connectionDigests, { - virtualMap: this.virtualMap, - }), - tagParseLog, - }; + const unbuildable: UnbuildableSource[] = []; + const connections = mkBuildTargets( + walk, + connectionDigests, + {virtualMap: this.virtualMap}, + unbuildable + ); + return {connections, tagParseLog, unbuildable}; } } diff --git a/packages/malloy/src/api/foundation/types.ts b/packages/malloy/src/api/foundation/types.ts index 5b5979763..11912def1 100644 --- a/packages/malloy/src/api/foundation/types.ts +++ b/packages/malloy/src/api/foundation/types.ts @@ -180,6 +180,22 @@ export interface ConnectionBuild { targets: BuildTarget[]; } +/** + * A persist source no table can be built for, because its SQL does not compile + * on its own — for example it reads a `given:` with no default, or a virtual + * source the runtime has no `virtualMap` entry for. + * + * Reported rather than thrown, so one such source does not cost the rest of + * the model its build. A source that reads one inlines it, so cannot compile + * either, and is reported here too. + */ +export interface UnbuildableSource { + /** The source, as a `BuildTarget` would have carried it */ + source: PersistSource; + /** Why its SQL did not compile, as a query would report it */ + problem: LogMessage; +} + /** * The build schedule for a model. * @@ -193,4 +209,6 @@ export interface BuildTargets { connections: ConnectionBuild[]; /** Errors and warnings from parsing `#@` annotations on persistable sources */ tagParseLog: LogMessage[]; + /** Persist sources with no table to build, each with the reason */ + unbuildable: UnbuildableSource[]; } diff --git a/packages/malloy/src/doc/persist/api.md b/packages/malloy/src/doc/persist/api.md index 8e573527f..d1d07e836 100644 --- a/packages/malloy/src/doc/persist/api.md +++ b/packages/malloy/src/doc/persist/api.md @@ -197,7 +197,8 @@ this section. ```typescript const model = await runtime.loadModel(modelURL).getModel(); -const {connections, tagParseLog} = await runtime.getBuildTargets(model); +const {connections, tagParseLog, unbuildable} = + await runtime.getBuildTargets(model); // throws without ##! experimental.persistence ``` @@ -226,9 +227,16 @@ batching by depth would. `ConnectionBuild` in `types.ts` carries both loops. malformed annotation is a build problem. It compiles every persist source in the model, because a BuildID is a hash of -SQL. One whose dialect cannot express it throws — from the dialect, with its -own message — and takes the whole call rather than reporting one unbuildable -target among many. +SQL. A source whose SQL does not compile on its own — for example it reads a +`given:` with no default, or a virtual source the runtime has no `virtualMap` +entry for — is not a target. It comes back in `unbuildable`, with the compiler's `problem`, and +so does every source that reads it, since that source's SQL inlines it. The rest +of the model is planned as usual. Report them; whether an unbuildable source is +an error is the builder's call. + +One whose dialect cannot express it throws — from the dialect, with its own +message — and takes the whole call, as does any other internal compiler error. +Only a `MalloyCompileError` is a fact about one source. **One table, several sources** is the normal case, not an edge case — `#@ persist` is inherited and `extend` does not change the SQL a source diff --git a/packages/malloy/src/doc/persist/internal.md b/packages/malloy/src/doc/persist/internal.md index 5002b0be7..0e27a9b5e 100644 --- a/packages/malloy/src/doc/persist/internal.md +++ b/packages/malloy/src/doc/persist/internal.md @@ -238,8 +238,8 @@ the first one to arrive. `Runtime.getBuildTargets(model)` in [`api/foundation/runtime.ts`](../../api/foundation/runtime.ts) is what a builder consumes. `Model._walkPersistSources()` supplies the stream — every named source -and query plus the unnamed queries, as one walk — and `mkBuildTargets(nodes, -model, connectionDigests)` in +and query plus the unnamed queries, as one walk — and `mkBuildTargets(walk, +connectionDigests, buildIdOpts, unbuildable)` in [`api/foundation/build_targets.ts`](../../api/foundation/build_targets.ts) folds it. That split is where the async/sync boundary falls: fetching a digest per connection is async, the fold is not, so the Runtime materializes the walk, @@ -255,7 +255,11 @@ them, because the walk is in dependency order: immediately — minus any that landed on its own key (an extension depending on its base is one table, not a dependency), and intersected with what the other sources on this target recorded (see the cycle note below); -3. a route contributes its children's keys upward instead of a key of its own; +3. a route contributes its children's keys upward instead of a key of its own, + and so does a persistent source whose `getSQL()` throws a + `MalloyCompileError`, which goes to `unbuildable` instead of a target. Its + dependents inline its SQL, so they throw and land there too. Any other error + is the compiler's own and propagates; 4. finally `place()` emits each target after everything it depends on — a depth-first post-order walk of the target graph — filing it under its connection. diff --git a/packages/malloy/src/index.ts b/packages/malloy/src/index.ts index 88d79551a..70ed93721 100644 --- a/packages/malloy/src/index.ts +++ b/packages/malloy/src/index.ts @@ -282,6 +282,7 @@ export type { BuildTarget, BuildTargets, ConnectionBuild, + UnbuildableSource, } from './api/foundation'; export {PersistSource, EMPTY_BUILD_MANIFEST} from './api/foundation'; export {Reference} from './api/foundation'; diff --git a/scripts/simple_builder/build.ts b/scripts/simple_builder/build.ts index ea3be849b..0d2fc2380 100644 --- a/scripts/simple_builder/build.ts +++ b/scripts/simple_builder/build.ts @@ -69,7 +69,8 @@ * order, so it never includes substituted table names. * `getBuildTargets` has already computed both. A model with virtual * sources needs a `virtualMap` on the Runtime for this to compile, and - * the same map in the build SQL below. + * the same map in the build SQL below; without it, those sources come + * back in `unbuildable` instead of as targets. * * - **Build SQL** — `source.getSQL({buildManifest, connectionDigests})`. * Dependencies that are already in the manifest are replaced with @@ -298,13 +299,22 @@ async function runBuild( // This lives on the Runtime rather than the Model because a BuildID is a // hash of the SQL *and the connection's digest* — the answer can't be // finished without a connection. - const {connections, tagParseLog} = await runtime.getBuildTargets(model); + const {connections, tagParseLog, unbuildable} = + await runtime.getBuildTargets(model); for (const msg of tagParseLog) { const loc = msg.at ? ` (${msg.at.url}:${msg.at.range.start.line + 1})` : ''; console.warn(`WARNING: ${msg.message}${loc}`); } + // Whether an unbuildable source fails the run is the builder's call. This + // one reports it and builds the rest. + for (const {source, problem} of unbuildable) { + const at = problem.at ?? source.location; + const loc = at ? ` (${at.url}:${at.range.start.line + 1})` : ''; + console.warn(`SKIPPED ${source.name}: ${problem.message}${loc}`); + } + const targetCount = connections.flatMap(c => c.targets).length; if (targetCount === 0) { console.log('No #@ persist sources found in model'); From b481911d26be640889d6a954216d0d5873e1b9ae Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 14:21:02 -0600 Subject: [PATCH 2/4] A persisted source that reads a given with no default says why it cannot be persisted The builder and the manifest lookup both compile a persisted source's SQL without resolving givens, so a given with no default has no value there. Both now raise persist-given-no-value, naming the source and the extend-block where: that filters per caller, instead of asking for a value the builder cannot supply and the query already did. Fixes #3041. Signed-off-by: James Estes --- .../src/api/foundation/build-targets.spec.ts | 3 +- packages/malloy/src/api/foundation/core.ts | 18 ++--- packages/malloy/src/doc/persist/api.md | 7 ++ packages/malloy/src/doc/persist/internal.md | 5 +- packages/malloy/src/model/index.ts | 6 +- packages/malloy/src/model/sql_compiled.ts | 61 ++++++++++++--- test/src/core/persist.spec.ts | 76 +++++++++++++++++++ 7 files changed, 150 insertions(+), 26 deletions(-) diff --git a/packages/malloy/src/api/foundation/build-targets.spec.ts b/packages/malloy/src/api/foundation/build-targets.spec.ts index 3b264c10d..5f0fb52bc 100644 --- a/packages/malloy/src/api/foundation/build-targets.spec.ts +++ b/packages/malloy/src/api/foundation/build-targets.spec.ts @@ -355,7 +355,8 @@ describe('a source whose SQL does not compile', () => { ]); expect(unbuildable.map(u => u.source.name)).toEqual(['scoped']); expect(unbuildable[0].problem.severity).toBe('error'); - expect(unbuildable[0].problem.message).toContain('TENANT'); + expect(unbuildable[0].problem.code).toBe('persist-given-no-value'); + expect(unbuildable[0].problem.message).toContain('extend-block `where:`'); }); test('makes what reads it unbuildable too', () => { diff --git a/packages/malloy/src/api/foundation/core.ts b/packages/malloy/src/api/foundation/core.ts index c4c745424..015be9fbf 100644 --- a/packages/malloy/src/api/foundation/core.ts +++ b/packages/malloy/src/api/foundation/core.ts @@ -54,8 +54,8 @@ import { isJoined, isRecordOrRepeatedRecord, isPersistableSourceDef, - getCompiledSQL, getModelAnnotations, + persistedSourceSQL, safeRecordGet, } from '../../model'; import {mkModelDef} from '../../model/utils'; @@ -1821,7 +1821,6 @@ export class PersistSource implements Taggable { * @return The SQL string for this source. */ getSQL(options?: CompileQueryOptions): string { - const sd = this.persistableDef; const queryModel = this.model.queryModel; // Compile with finalize=false so this SQL is the bare source SELECT. @@ -1829,16 +1828,11 @@ export class PersistSource implements Taggable { // manifest lookup key, which persistedTableFor recomputes from the same // unfinalized SELECT; finalizing would diverge the two on dialects with a // final stage (Postgres) and mis-materialize the table. - if (sd.type === 'sql_select') { - return getCompiledSQL( - sd, - options ?? {}, - (query, opts) => queryModel.compileQuery(query, opts, false).sql - ); - } else { - const compiled = queryModel.compileQuery(sd.query, options, false); - return compiled.sql; - } + return persistedSourceSQL( + this.persistableDef, + (query, opts) => queryModel.compileQuery(query, opts, false).sql, + options ?? {} + ); } /** diff --git a/packages/malloy/src/doc/persist/api.md b/packages/malloy/src/doc/persist/api.md index d1d07e836..c482d1e6b 100644 --- a/packages/malloy/src/doc/persist/api.md +++ b/packages/malloy/src/doc/persist/api.md @@ -234,6 +234,13 @@ so does every source that reads it, since that source's SQL inlines it. The rest of the model is planned as usual. Report them; whether an unbuildable source is an error is the builder's call. +A source that reads a `given:` with no default is the case to know. Its table +would have to be one table for every caller, so it cannot be persisted: its +problem is `persist-given-no-value`, and a query over it fails with the same +error once a manifest is in play, whatever givens the query supplies. Filter on +the given in an extend-block `where:` instead. That is not part of the persisted +relation, so the table holds every row and each query reads its own slice. + One whose dialect cannot express it throws — from the dialect, with its own message — and takes the whole call, as does any other internal compiler error. Only a `MalloyCompileError` is a fact about one source. diff --git a/packages/malloy/src/doc/persist/internal.md b/packages/malloy/src/doc/persist/internal.md index 0e27a9b5e..197ddb9b3 100644 --- a/packages/malloy/src/doc/persist/internal.md +++ b/packages/malloy/src/doc/persist/internal.md @@ -370,7 +370,10 @@ a type narrow enough that it cannot carry a manifest, so the BuildID cannot depend on what is already materialized. It keeps `virtualMap`, which decides what table a virtual source reads. It drops `resolvedGivens`, which also changes the SQL but which only the compiler can produce; resolving on one side -only makes the keys disagree, so neither side resolves givens (see #3041). +only makes the keys disagree, so neither side resolves givens. +`persistedSourceSQL()` compiles a persisted source's SQL for both sides, and +turns the resulting `compiler-given-no-value` into `persist-given-no-value`, +which names the source and the way out (#3041). `mkBuildTargets` takes the same type. A hit returns `entry.tableName` unquoted; manifest names are canonical. A miss under `strict` throws `runtime-manifest-strict-miss` at the source's location, appending `loadError` if present. It names the source's diff --git a/packages/malloy/src/model/index.ts b/packages/malloy/src/model/index.ts index 1efe3fcd1..06888d579 100644 --- a/packages/malloy/src/model/index.ts +++ b/packages/malloy/src/model/index.ts @@ -48,7 +48,11 @@ export { } from './utils'; export {getModelAnnotations} from './annotation_utils'; export {constantExprToSQL} from './constant_expression_compiler'; -export {getCompiledSQL, buildIdOptions} from './sql_compiled'; +export { + getCompiledSQL, + buildIdOptions, + persistedSourceSQL, +} from './sql_compiled'; export type {BuildIdOptions} from './sql_compiled'; export {MalloyCompileError} from './malloy_compile_error'; export { diff --git a/packages/malloy/src/model/sql_compiled.ts b/packages/malloy/src/model/sql_compiled.ts index 42b7d8cf9..4ee1a2b18 100644 --- a/packages/malloy/src/model/sql_compiled.ts +++ b/packages/malloy/src/model/sql_compiled.ts @@ -104,9 +104,9 @@ export interface BuildIdOptions { * givens and `PersistSource.getSQL` never resolves them — so including it makes * the two sides disagree: the compiler folds `$IS_ADMIN` to a literal while the * builder re-derives it from the declaration and emits `'admin'='admin'`. Both - * sides therefore compile givens the same way, by not resolving them. The - * consequence — a source whose SQL depends on a supplied given cannot be - * persisted, and says so badly — is #3041. + * sides therefore compile givens the same way, by not resolving them, so a + * source whose SQL depends on a supplied given cannot be persisted; see + * {@link persistedSourceSQL}. */ export function buildIdOptions(opts: PrepareResultOptions): BuildIdOptions { return {virtualMap: opts.virtualMap}; @@ -139,7 +139,7 @@ export function persistedTableFor( const buildId = mkBuildID( connDigest, - getSourceSQL(source, compileQuery, buildIdOptions(opts)) + persistedSourceSQL(source, compileQuery, buildIdOptions(opts)) ); const entry = buildManifest.entries[buildId]; if (entry) { @@ -147,14 +147,8 @@ export function persistedTableFor( } if (buildManifest.strict) { - // `sourceID` is deleted, not inherited, when a source is modified, so it - // either names this source or is absent. `as` and `extends` survive a - // modification and would name the base, so neither stands in for it. - const named = source.sourceID - ? `Persisted source '${source.sourceID}'` - : 'Persisted source'; const base = - `${named} not found in manifest (buildId: ${buildId}); ` + + `${persistedSourceLabel(source)} not found in manifest (buildId: ${buildId}); ` + 'strict manifest mode forbids fallback to live compilation.'; throw new MalloyCompileError( buildManifest.loadError ? `${base}\n ${buildManifest.loadError}` : base, @@ -198,6 +192,51 @@ function expandQuery( return `(${sql})`; } +/** + * How an error names a persisted source. `sourceID` is deleted, not inherited, + * when a source is modified, so it either names this source or is absent. `as` + * and `extends` survive a modification and would name the base, so neither + * stands in for it. + */ +function persistedSourceLabel(source: PersistableSourceDef): string { + return source.sourceID + ? `Persisted source '${source.sourceID}'` + : 'Persisted source'; +} + +/** + * The SQL a persisted source's table is built from, compiled as both the + * builder and the manifest lookup compile it: with givens unresolved (see + * {@link buildIdOptions}). A given with no default therefore has no value here, + * and the source has no one table to build. The error says that, rather than + * asking for a value that no builder can supply and that a query already did. + */ +export function persistedSourceSQL( + source: PersistableSourceDef, + compileQuery: CompileQueryCallback, + opts: PrepareResultOptions +): string { + try { + return getSourceSQL(source, compileQuery, opts); + } catch (e) { + if ( + !(e instanceof MalloyCompileError) || + e.code !== 'compiler-given-no-value' + ) { + throw e; + } + throw new MalloyCompileError( + `${persistedSourceLabel(source)} reads a given with no default. A ` + + 'persisted source is built once, as one table for every caller, so ' + + 'its SQL cannot depend on a value each caller supplies. Filter on the ' + + 'given in an extend-block `where:`, which applies per caller when the ' + + 'table is read, or give the given a default.', + 'persist-given-no-value', + e.at ?? source.location + ); + } +} + /** * Get the SQL for a PersistableSourceDef. * diff --git a/test/src/core/persist.spec.ts b/test/src/core/persist.spec.ts index 2705cc5d6..d54178e3a 100644 --- a/test/src/core/persist.spec.ts +++ b/test/src/core/persist.spec.ts @@ -2802,4 +2802,80 @@ describe('source persistence', () => { expect(sql).toContain('LIMIT 7'); }); }); + + describe('a persisted source reading a given with no default', () => { + // Its table would have to be one table for every caller, so it cannot be + // persisted. The builder and the manifest lookup compile it without + // resolving givens, and both say so. + const MODEL = ` + ##! experimental { persistence givens } + given: CARRIER :: string + ${FLIGHTS_SOURCE} + + #@ persist + source: carrier_stats is flights -> { + group_by: carrier + aggregate: flight_count is count() + } + + #@ persist + source: in_relation is flights -> { + where: carrier = $CARRIER + group_by: carrier + aggregate: flight_count is count() + } + + #@ persist + source: at_read is flights -> { + group_by: carrier + aggregate: flight_count is count() + } extend { where: carrier = $CARRIER } + + query: q_in_relation is in_relation -> { select: * } + query: q_at_read is at_read -> { select: * } + `; + + it('fails a query with a manifest, naming the cause, though the query supplied the given', async () => { + const model = await tstRuntime.loadModel(MODEL).getModel(); + const targets = await tstRuntime.getBuildTargets(model); + const stats = targets.connections + .flatMap(c => c.targets) + .find(t => t.sources.some(s => s.name === 'carrier_stats'))!; + const manifest = createManifest(); + addManifestEntry(manifest, stats.buildId, 'carrier_stats_table'); + + await expect( + runtimeWithManifest(manifest) + .loadModel(MODEL) + .loadQueryByName('q_in_relation') + .run({givens: {CARRIER: 'WN'}}) + ).rejects.toThrow(/Persisted source '[^']*in_relation@.*no default/); + }); + + it('persists when the given filters at read, per caller', async () => { + // The advice the error gives: an extend-block `where:` is not in the + // persisted relation, so the table holds every carrier and each query + // reads its own slice of it. + const model = await tstRuntime.loadModel(MODEL).getModel(); + const targets = await tstRuntime.getBuildTargets(model); + const atRead = targets.connections + .flatMap(c => c.targets) + .find(t => t.sources.some(s => s.name === 'at_read'))!; + await tstRuntime.connection.runSQL( + `CREATE OR REPLACE TEMP VIEW at_read_table AS ${atRead.sql}` + ); + const manifest = createManifest(); + addManifestEntry(manifest, atRead.buildId, 'at_read_table'); + + const result = await runtimeWithManifest(manifest) + .loadModel(MODEL) + .loadQueryByName('q_at_read') + .run({givens: {CARRIER: 'WN'}}); + + expect(result.sql).toContain('at_read_table'); + expect( + new Set(result.data.toObject().map(row => row['carrier'])) + ).toEqual(new Set(['WN'])); + }); + }); }); From 678e841532e50b6aab27538e0581ed4848d9bcc8 Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 14:31:23 -0600 Subject: [PATCH 3/4] Name the persisted-given error in the runtime- family, and keep test names in the present Signed-off-by: James Estes --- packages/malloy/src/api/foundation/build-targets.spec.ts | 6 +++--- packages/malloy/src/doc/persist/api.md | 2 +- packages/malloy/src/doc/persist/internal.md | 2 +- packages/malloy/src/model/sql_compiled.ts | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/malloy/src/api/foundation/build-targets.spec.ts b/packages/malloy/src/api/foundation/build-targets.spec.ts index 5f0fb52bc..4c1385de4 100644 --- a/packages/malloy/src/api/foundation/build-targets.spec.ts +++ b/packages/malloy/src/api/foundation/build-targets.spec.ts @@ -339,7 +339,7 @@ describe('a source whose SQL does not compile', () => { `; const GIVENS = ['experimental.givens']; - test('is reported, and the rest of the model still builds', () => { + test('is reported, and the rest of the model is planned', () => { const {connections, unbuildable} = planOf( ` ${ROLLUP} @@ -355,7 +355,7 @@ describe('a source whose SQL does not compile', () => { ]); expect(unbuildable.map(u => u.source.name)).toEqual(['scoped']); expect(unbuildable[0].problem.severity).toBe('error'); - expect(unbuildable[0].problem.code).toBe('persist-given-no-value'); + expect(unbuildable[0].problem.code).toBe('runtime-persist-given-no-value'); expect(unbuildable[0].problem.message).toContain('extend-block `where:`'); }); @@ -375,7 +375,7 @@ describe('a source whose SQL does not compile', () => { expect(unbuildable.map(u => u.source.name)).toEqual(['scoped', 'top']); }); - test('a compiler bug is still thrown', () => { + test('a compiler bug is thrown', () => { // Only a MalloyCompileError is a fact about the source. Anything else is // the compiler's own failure, and hiding it as an unbuildable source would // hide the bug. diff --git a/packages/malloy/src/doc/persist/api.md b/packages/malloy/src/doc/persist/api.md index c482d1e6b..217519508 100644 --- a/packages/malloy/src/doc/persist/api.md +++ b/packages/malloy/src/doc/persist/api.md @@ -236,7 +236,7 @@ an error is the builder's call. A source that reads a `given:` with no default is the case to know. Its table would have to be one table for every caller, so it cannot be persisted: its -problem is `persist-given-no-value`, and a query over it fails with the same +problem is `runtime-persist-given-no-value`, and a query over it fails with the same error once a manifest is in play, whatever givens the query supplies. Filter on the given in an extend-block `where:` instead. That is not part of the persisted relation, so the table holds every row and each query reads its own slice. diff --git a/packages/malloy/src/doc/persist/internal.md b/packages/malloy/src/doc/persist/internal.md index 197ddb9b3..936995d24 100644 --- a/packages/malloy/src/doc/persist/internal.md +++ b/packages/malloy/src/doc/persist/internal.md @@ -372,7 +372,7 @@ what table a virtual source reads. It drops `resolvedGivens`, which also changes the SQL but which only the compiler can produce; resolving on one side only makes the keys disagree, so neither side resolves givens. `persistedSourceSQL()` compiles a persisted source's SQL for both sides, and -turns the resulting `compiler-given-no-value` into `persist-given-no-value`, +turns the resulting `compiler-given-no-value` into `runtime-persist-given-no-value`, which names the source and the way out (#3041). `mkBuildTargets` takes the same type. A hit returns `entry.tableName` unquoted; manifest names are canonical. A miss under `strict` throws `runtime-manifest-strict-miss` at the diff --git a/packages/malloy/src/model/sql_compiled.ts b/packages/malloy/src/model/sql_compiled.ts index 4ee1a2b18..3f0cb1e64 100644 --- a/packages/malloy/src/model/sql_compiled.ts +++ b/packages/malloy/src/model/sql_compiled.ts @@ -231,7 +231,7 @@ export function persistedSourceSQL( 'its SQL cannot depend on a value each caller supplies. Filter on the ' + 'given in an extend-block `where:`, which applies per caller when the ' + 'table is read, or give the given a default.', - 'persist-given-no-value', + 'runtime-persist-given-no-value', e.at ?? source.location ); } From 50e121cae2e7621ae41a6adcd9fbe2c736f7486f Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 14:34:47 -0600 Subject: [PATCH 4/4] Keep only the test that pins the persisted-given error, and state its function's invariant Signed-off-by: James Estes --- packages/malloy/src/model/sql_compiled.ts | 9 +++---- test/src/core/persist.spec.ts | 33 ----------------------- 2 files changed, 4 insertions(+), 38 deletions(-) diff --git a/packages/malloy/src/model/sql_compiled.ts b/packages/malloy/src/model/sql_compiled.ts index 3f0cb1e64..468882c12 100644 --- a/packages/malloy/src/model/sql_compiled.ts +++ b/packages/malloy/src/model/sql_compiled.ts @@ -205,11 +205,10 @@ function persistedSourceLabel(source: PersistableSourceDef): string { } /** - * The SQL a persisted source's table is built from, compiled as both the - * builder and the manifest lookup compile it: with givens unresolved (see - * {@link buildIdOptions}). A given with no default therefore has no value here, - * and the source has no one table to build. The error says that, rather than - * asking for a value that no builder can supply and that a query already did. + * The SQL a persisted source's table is built from, for the builder and the + * manifest lookup alike. Both compile it with givens unresolved (see + * {@link buildIdOptions}), so a given with no default has no value here: the + * source cannot be persisted, and the error names that cause. */ export function persistedSourceSQL( source: PersistableSourceDef, diff --git a/test/src/core/persist.spec.ts b/test/src/core/persist.spec.ts index d54178e3a..29ad9003f 100644 --- a/test/src/core/persist.spec.ts +++ b/test/src/core/persist.spec.ts @@ -2825,14 +2825,7 @@ describe('source persistence', () => { aggregate: flight_count is count() } - #@ persist - source: at_read is flights -> { - group_by: carrier - aggregate: flight_count is count() - } extend { where: carrier = $CARRIER } - query: q_in_relation is in_relation -> { select: * } - query: q_at_read is at_read -> { select: * } `; it('fails a query with a manifest, naming the cause, though the query supplied the given', async () => { @@ -2851,31 +2844,5 @@ describe('source persistence', () => { .run({givens: {CARRIER: 'WN'}}) ).rejects.toThrow(/Persisted source '[^']*in_relation@.*no default/); }); - - it('persists when the given filters at read, per caller', async () => { - // The advice the error gives: an extend-block `where:` is not in the - // persisted relation, so the table holds every carrier and each query - // reads its own slice of it. - const model = await tstRuntime.loadModel(MODEL).getModel(); - const targets = await tstRuntime.getBuildTargets(model); - const atRead = targets.connections - .flatMap(c => c.targets) - .find(t => t.sources.some(s => s.name === 'at_read'))!; - await tstRuntime.connection.runSQL( - `CREATE OR REPLACE TEMP VIEW at_read_table AS ${atRead.sql}` - ); - const manifest = createManifest(); - addManifestEntry(manifest, atRead.buildId, 'at_read_table'); - - const result = await runtimeWithManifest(manifest) - .loadModel(MODEL) - .loadQueryByName('q_at_read') - .run({givens: {CARRIER: 'WN'}}); - - expect(result.sql).toContain('at_read_table'); - expect( - new Set(result.data.toObject().map(row => row['carrier'])) - ).toEqual(new Set(['WN'])); - }); }); });