getBuildTargets reports a source it cannot compile instead of failing the plan

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.
 

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,78 @@ 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 is planned', () => {
+    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.code).toBe('runtime-persist-given-no-value');
+    expect(unbuildable[0].problem.message).toContain('extend-block `where:`');
+  });
+
+  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 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'
+    );
+  });
+});

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<string>, 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<string, string>,
-  buildIdOpts: BuildIdOptions
+  buildIdOpts: BuildIdOptions,
+  unbuildable: UnbuildableSource[]
 ): ConnectionBuild[] {
   const targets = new Map<string, PartialTarget>();
   // 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);
 

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,24 +1821,18 @@ 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.
     // The build-time key (makeBuildId over this SQL) must equal the serve-time
     // 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 ?? {}
+    );
   }
 
   /**

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';
 

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};
   }
 }
 

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[];
 }

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,23 @@ 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.
+
+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 `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.
+
+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