diff --git a/CONTEXT.md b/CONTEXT.md index 28b17a9..9407172 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -64,7 +64,7 @@ fields are preserved. ## Persistence (`build` command) Core gives three primitives — annotation (`#@ persist`), build plan -(`model.getBuildPlan()`), and compile-time substitution via `BuildManifest`. The CLI is one +(`runtime.getBuildTargets()`), and compile-time substitution via `BuildManifest`. The CLI is one opinionated implementation; the VS Code extension is the other (it consumes manifests the CLI writes). When changing behavior here, align with the primitives — don't reinvent dependency tracking or caching in the CLI layer. @@ -73,8 +73,26 @@ CLI-specific conventions, in order of how-likely-to-bite-you: - **`#@ persist name=schema.table` silently fails** — `.` is a path separator in the tag parser. Dotted names must be quoted: `name="schema.table"`. Parse errors come back - through `getBuildPlan().tagParseLog`; the CLI prints them. Easy to miss in review. -- **`name=` is required.** The CLI uses it as the destination table name. Missing → error. + through `getBuildTargets().tagParseLog`; the CLI prints them. Easy to miss in review. +- **A target is a table, not a source.** `#@ persist` is inherited and `extend` doesn't + change a source's SQL, so several sources routinely share one `buildId` — core merges + them into one `BuildTarget` with all of them in `target.sources`. Don't recompute the + BuildID, dedupe, or sort: the target carries its own id and arrives in dependency order. +- **`name=` is required, and BuildID ↔ table is a bijection for the whole run.** The CLI + uses `name=` as the destination table name; missing → error. `TableClaims` enforces both + directions and both are cross-file, since within one model core would have merged them: + one BuildID under two names → error (else the second file reads the first's manifest + entry as "up to date" and never builds its own name), and two BuildIDs under one name → + error (else both build and one silently overwrites the other, leaving two manifest + entries pointing at a table holding one of the two computations). +- **Build and record the canonical name.** `dialect.sqlValidateTableName()` returns it; + it's the input verbatim for most dialects but not DuckDB's file-path form. Core's + `Manifest.update` re-checks, so a mismatch surfaces as a confusing post-CREATE failure + rather than a bad entry — canonicalize once, up front, and both uses agree. +- **`--refresh` keys on the table name, not the BuildID.** It's a user-facing flag and + users type names. A key matching nothing is reported, and a renamed source accepts + either its old or new name, because the manifest entry keeps the old one until the SQL + changes. - **DDL is `DROP TABLE IF EXISTS … ; CREATE TABLE … AS …`.** Fails when the user has CREATE but not DELETE — affects Trino/Presto via BigQuery proxy. See `createTableFromSelect`. Known limitation, no fix queued. diff --git a/package-lock.json b/package-lock.json index 9653bc8..1116f4e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -22,14 +22,14 @@ "@commander-js/extra-typings": "^12.1.0", "@duckdb/node-api": "1.5.3-r.2", "@duckdb/node-bindings": "1.5.3-r.2", - "@malloydata/db-bigquery": "0.0.424", - "@malloydata/db-duckdb": "0.0.424", - "@malloydata/db-postgres": "0.0.424", - "@malloydata/db-snowflake": "0.0.424", - "@malloydata/db-trino": "0.0.424", - "@malloydata/malloy": "0.0.424", - "@malloydata/malloy-connections": "0.0.424", - "@malloydata/malloy-sql": "0.0.424", + "@malloydata/db-bigquery": "0.0.429", + "@malloydata/db-duckdb": "0.0.429", + "@malloydata/db-postgres": "0.0.429", + "@malloydata/db-snowflake": "0.0.429", + "@malloydata/db-trino": "0.0.429", + "@malloydata/malloy": "0.0.429", + "@malloydata/malloy-connections": "0.0.429", + "@malloydata/malloy-sql": "0.0.429", "@types/jest": "^30.0.0", "@types/node": "^24.13.1", "@yao-pkg/pkg": "^6.20.0", @@ -3574,16 +3574,16 @@ } }, "node_modules/@malloydata/db-bigquery": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/db-bigquery/-/db-bigquery-0.0.424.tgz", - "integrity": "sha512-ZJiLuXOIqxjBbHowJM7uhsV8/RIEzD27JhAQArA30tnR7k9yqqwsHXnPEm3Vxy1diD/4NHoel9n32Sm3qIu3nw==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/db-bigquery/-/db-bigquery-0.0.429.tgz", + "integrity": "sha512-IoB/j2+FYF9KrgtfY1JGqEUnggEKMvn9R8pmnNnc5m0UkrqIHnmO3d45Fht8IWXKMLICgvFG4nBIqT/f9P2N1A==", "dev": true, "license": "MIT", "dependencies": { "@google-cloud/bigquery": "7.9.4", "@google-cloud/common": "5.0.2", "@google-cloud/paginator": "5.0.2", - "@malloydata/malloy": "0.0.424", + "@malloydata/malloy": "0.0.429", "gaxios": "^4.2.0" }, "engines": { @@ -3591,14 +3591,14 @@ } }, "node_modules/@malloydata/db-databricks": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/db-databricks/-/db-databricks-0.0.424.tgz", - "integrity": "sha512-W5Yskwks8Tks4Bch4Tg9SmhU6MhpMZNR9Me31RZr3rTZawAW/2LW++GoN5v3DWteAHmL+icLZYuKEfa0y/xgDw==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/db-databricks/-/db-databricks-0.0.429.tgz", + "integrity": "sha512-cK0Hn9hE7KAsIBjqZmqv+MavDEt58STbe4Jji64HkxnzYGG0cn1VgtKL6PMPIg9F8E5S0PaMnnJ3A/g/dexO0g==", "dev": true, "license": "MIT", "dependencies": { "@databricks/sql": "1.15.0", - "@malloydata/malloy": "0.0.424", + "@malloydata/malloy": "0.0.429", "@types/node": "^22.20.0" }, "engines": { @@ -3616,15 +3616,15 @@ } }, "node_modules/@malloydata/db-duckdb": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/db-duckdb/-/db-duckdb-0.0.424.tgz", - "integrity": "sha512-p5HxuO3s0NexyEKahu7gTrs/oQF32jII4IKj6Zx0QhbsWCV4+EYmIwome7gUGxMUYb/5uFsVF9oFuyHHfOFiRQ==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/db-duckdb/-/db-duckdb-0.0.429.tgz", + "integrity": "sha512-18CKOIhf7t3jlQMn4M7tSjxHxfbI0DcOjjm/P2QxLiFJnAmjQhajmUta1T3lV7hubEWVxLluIaeG5V2rmDyoRQ==", "dev": true, "license": "MIT", "dependencies": { "@duckdb/duckdb-wasm": "1.33.1-dev45.0", "@duckdb/node-api": "1.5.3-r.2", - "@malloydata/malloy": "0.0.424", + "@malloydata/malloy": "0.0.429", "@motherduck/wasm-client": "0.6.6", "apache-arrow": "^17.0.0", "web-worker": "^1.5.0" @@ -3634,13 +3634,13 @@ } }, "node_modules/@malloydata/db-mysql": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/db-mysql/-/db-mysql-0.0.424.tgz", - "integrity": "sha512-8w/DbK6GAKv0PTFFKhY553kyUW3V/Glcyz1vWH4ePy0p663II+gNBNzXBa4RFv9mAzQChdy6oCzf6VRx9qmPQQ==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/db-mysql/-/db-mysql-0.0.429.tgz", + "integrity": "sha512-WVuO6r99Xby3cifHA00j/ntv5WMYNQAO2AqftlsG8jvQ1Xhl+mg1igpmAOe7cm50XK3PPNx8+FS5qYXL/4CqCQ==", "dev": true, "license": "MIT", "dependencies": { - "@malloydata/malloy": "0.0.424", + "@malloydata/malloy": "0.0.429", "@types/node": "^22.20.0", "fastestsmallesttextencoderdecoder": "^1.0.22", "luxon": "^3.7.2", @@ -3661,13 +3661,13 @@ } }, "node_modules/@malloydata/db-postgres": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/db-postgres/-/db-postgres-0.0.424.tgz", - "integrity": "sha512-XVUSzMNdHT/SJ2rvfX5d7NmKCWXPlkM+9gkFaRNGB5gp95bokrRlIorPlI4xJnxVvQGRyUGgKnNmRL/fuSXPyQ==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/db-postgres/-/db-postgres-0.0.429.tgz", + "integrity": "sha512-XA+WuM4O2RPm+8jl8wVCd/q6PapJ28a0c3uBXycvrQa9iuKhCi0PtUZrOSphY0YjS5uhCbrr8JTGBSnmfH0KKA==", "dev": true, "license": "MIT", "dependencies": { - "@malloydata/malloy": "0.0.424", + "@malloydata/malloy": "0.0.429", "@types/pg": "^8.6.1", "pg": "8.7.3", "pg-query-stream": "4.2.3" @@ -3704,13 +3704,13 @@ } }, "node_modules/@malloydata/db-snowflake": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/db-snowflake/-/db-snowflake-0.0.424.tgz", - "integrity": "sha512-I844xHANuDOUS1+YaR9235QxlizaDNKnhsZ5Acnlkv/5bfm6X7IBU3F0fYRVTcD0NPAh6mbS5xQhE+gduaMO2g==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/db-snowflake/-/db-snowflake-0.0.429.tgz", + "integrity": "sha512-jkK4Vs2NK6aFDsm50Y2oPbl23rmQus/atiJ3aO0zE0PaVJjhe3HT53cG9NIR14C/LP2ENEVG994n2UEKndglcQ==", "dev": true, "license": "MIT", "dependencies": { - "@malloydata/malloy": "0.0.424", + "@malloydata/malloy": "0.0.429", "generic-pool": "^3.9.0", "snowflake-sdk": "2.3.1", "toml": "^3.0.0" @@ -3720,13 +3720,13 @@ } }, "node_modules/@malloydata/db-trino": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/db-trino/-/db-trino-0.0.424.tgz", - "integrity": "sha512-rOjHywp1pvzQzJ+xT34ClBL50DTbcFc5vFHywDs4AmwKKyw1ROCPLftPkqLYsWC0/5qy9ahd6Fsr0RC34YUFEA==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/db-trino/-/db-trino-0.0.429.tgz", + "integrity": "sha512-m1MtZKYT7jWVwQD5Egw09bx3hc+wJLKG166+XyjSxGMwC1hE9+xgbBrZmyoLSflDcNecbQ/z964rIDhZ9kVo2Q==", "dev": true, "license": "MIT", "dependencies": { - "@malloydata/malloy": "0.0.424", + "@malloydata/malloy": "0.0.429", "@prestodb/presto-js-client": "^1.1.0", "gaxios": "^4.2.0", "luxon": "^3.7.2", @@ -3737,15 +3737,15 @@ } }, "node_modules/@malloydata/malloy": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/malloy/-/malloy-0.0.424.tgz", - "integrity": "sha512-57Y0AhDc30XvvAejhvS0ktNxCp+OublBp4dMkx2qIChf/W/ADR4s2Mvv6wFCAaVwRk2xl03T0tn/j5hWYVYRKg==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/malloy/-/malloy-0.0.429.tgz", + "integrity": "sha512-TSb/AJibK5G0N4vqA+JUuE9pNpS/ir+tur2x/+SA4qVBWcKY5x32OUAuziM7JAgOxcdvRGnG1Lue7UDwWE9cSA==", "dev": true, "license": "MIT", "dependencies": { - "@malloydata/malloy-filter": "0.0.424", - "@malloydata/malloy-interfaces": "0.0.424", - "@malloydata/malloy-tag": "0.0.424", + "@malloydata/malloy-filter": "0.0.429", + "@malloydata/malloy-interfaces": "0.0.429", + "@malloydata/malloy-tag": "0.0.429", "@noble/hashes": "^1.8.0", "antlr4ts": "^0.5.0-alpha.4", "assert": "^2.0.0", @@ -3759,29 +3759,29 @@ } }, "node_modules/@malloydata/malloy-connections": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/malloy-connections/-/malloy-connections-0.0.424.tgz", - "integrity": "sha512-KrzEbBPQSI4iu72BxgNeqBW4fs6ebAwHQ1P23lVsi56mDumkfkS/8lRtSpY9n2XWjOFEIjty0wam9DZIIby6EA==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/malloy-connections/-/malloy-connections-0.0.429.tgz", + "integrity": "sha512-0l4zeLrU9kYjdWUjdu18UIM9bRbjdCv4reSsPTNiCyOWslv7eZQo3AV4ehA+waQsI/HUtUEn3uM2q7rzQbMTFg==", "dev": true, "license": "MIT", "dependencies": { - "@malloydata/db-bigquery": "0.0.424", - "@malloydata/db-databricks": "0.0.424", - "@malloydata/db-duckdb": "0.0.424", - "@malloydata/db-mysql": "0.0.424", - "@malloydata/db-postgres": "0.0.424", - "@malloydata/db-snowflake": "0.0.424", - "@malloydata/db-trino": "0.0.424", - "@malloydata/malloy": "0.0.424" + "@malloydata/db-bigquery": "0.0.429", + "@malloydata/db-databricks": "0.0.429", + "@malloydata/db-duckdb": "0.0.429", + "@malloydata/db-mysql": "0.0.429", + "@malloydata/db-postgres": "0.0.429", + "@malloydata/db-snowflake": "0.0.429", + "@malloydata/db-trino": "0.0.429", + "@malloydata/malloy": "0.0.429" }, "engines": { "node": ">=20" } }, "node_modules/@malloydata/malloy-filter": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/malloy-filter/-/malloy-filter-0.0.424.tgz", - "integrity": "sha512-sa7PwgBxRooVy5gBhAFN9FnfenJY1nVRyNvWB3XrWjGXAXBEGF4bgYM5YvY/Eoej0sOqS/5MODLd3EnzUzKviA==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/malloy-filter/-/malloy-filter-0.0.429.tgz", + "integrity": "sha512-8K1VE5uiQzxlodr5Vy0HWAeM2xsrJiLTKdvX2rBn6yu/VLIDTseG2kyEthEjEa0Kc3Uq94tHdyVl7Qt137ZFzQ==", "dev": true, "license": "MIT", "dependencies": { @@ -3792,9 +3792,9 @@ } }, "node_modules/@malloydata/malloy-interfaces": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/malloy-interfaces/-/malloy-interfaces-0.0.424.tgz", - "integrity": "sha512-RC4yLmEponF1rzbk/6me4TIIJU3DFQrZ16wtV4Ko3jVW0gC8JomGB7GpZOGsi3ldYa8eTM2K9ByVy/4lpGwIQA==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/malloy-interfaces/-/malloy-interfaces-0.0.429.tgz", + "integrity": "sha512-HVfAg0GZgtVAbuUkYzfnI4ozKTG+Zqa6YLEQEt6/1OSF/mUbk3XWpHK+RrXBDFisSJLiqZms1UEASJ38eT2OwA==", "dev": true, "license": "MIT", "dependencies": { @@ -3805,22 +3805,22 @@ } }, "node_modules/@malloydata/malloy-sql": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/malloy-sql/-/malloy-sql-0.0.424.tgz", - "integrity": "sha512-Rn+q7pq6doQi4w5h2gui2oHdpDmYpB56uiX4rPDJBSDDmiEe+Gjzg9mTaj8Mdrs3tCbDSA4UnC+qE9nr6wbftA==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/malloy-sql/-/malloy-sql-0.0.429.tgz", + "integrity": "sha512-nXr/kqNSQS0cZ6eTBYAl1nrbxIz/q2LKkzXnmiXJf8wRsOhqy9dp7lLpwbbiUkYHyaa1y3ZwBzOluwFedPkt+w==", "dev": true, "license": "MIT", "dependencies": { - "@malloydata/malloy": "0.0.424" + "@malloydata/malloy": "0.0.429" }, "engines": { "node": ">=20" } }, "node_modules/@malloydata/malloy-tag": { - "version": "0.0.424", - "resolved": "https://registry.npmjs.org/@malloydata/malloy-tag/-/malloy-tag-0.0.424.tgz", - "integrity": "sha512-pGnH4Ca/pIURRt07K02Q7D9gW4Df8MdiXW9CCU27KIGbtzEOkcY7rYhq2ZSDNkq/TKdtFMSHm22PTH/aAE+obA==", + "version": "0.0.429", + "resolved": "https://registry.npmjs.org/@malloydata/malloy-tag/-/malloy-tag-0.0.429.tgz", + "integrity": "sha512-xit0/hOP+Fj2I1fvQ2taxecYi2tMBcQJ99lR18/ABfefkvg9e4SjLm4bErBASZTX6WQKGLXgZm/NXKuBN2nlpQ==", "dev": true, "license": "MIT", "dependencies": { @@ -4561,9 +4561,9 @@ "license": "MIT" }, "node_modules/@types/lodash": { - "version": "4.17.24", - "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.17.24.tgz", - "integrity": "sha512-gIW7lQLZbue7lRSWEFql49QJJWThrTFFeIMJdp3eH4tKoxm1OvEPg02rm4wCCSHS0cL3/Fizimb35b7k8atwsQ==", + "version": "4.17.25", + "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.17.25.tgz", + "integrity": "sha512-+K1NIO8I+F9/wNulfVvu23QYd0Pe9/OCqRrim4NoYIf1VoEDL90Ve4ClzpyqBLc7NpGGWRvYNCKZ1BE/Jpf8dQ==", "dev": true, "license": "MIT" }, @@ -7504,16 +7504,6 @@ "node": ">=0.4.0" } }, - "node_modules/denque": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/denque/-/denque-2.1.0.tgz", - "integrity": "sha512-HVQE3AAb/pxF8fQAoiqpvg9i3evqug3hoiwakOyZAwJm+6vZehbkYXZ0l4JxS+I3QxM97v5aaRNhj8v5oBhekw==", - "dev": true, - "license": "Apache-2.0", - "engines": { - "node": ">=0.10" - } - }, "node_modules/depd": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/depd/-/depd-2.0.0.tgz", @@ -9704,9 +9694,9 @@ } }, "node_modules/iconv-lite": { - "version": "0.7.2", - "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.7.2.tgz", - "integrity": "sha512-im9DjEDQ55s9fL4EYzOAv0yMqmMBSZp6G0VvFyTMPKWxiSBHUj9NW/qqLmXUwXrrM7AvqSlTCfvqRb0cM8yYqw==", + "version": "0.7.3", + "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.7.3.tgz", + "integrity": "sha512-IKXpvIzjnC9XTAUbVBcMfGS0EPaIXtW6v+zr+RRp+hqULEpo0owZax6wyRwPOJbWbzjYspQwusTsfVr0ifh4uQ==", "license": "MIT", "dependencies": { "safer-buffer": ">= 2.1.2 < 3.0.0" @@ -12185,16 +12175,15 @@ "license": "ISC" }, "node_modules/mysql2": { - "version": "3.23.0", - "resolved": "https://registry.npmjs.org/mysql2/-/mysql2-3.23.0.tgz", - "integrity": "sha512-ZwyGLoG9BRlL7hKNDcIy7q18hA6WTWGEpLAerGHXj5Xahemia4msZdwjIOUIzesjK3B5z5gWLktf+2tYSLAyTA==", + "version": "3.23.3", + "resolved": "https://registry.npmjs.org/mysql2/-/mysql2-3.23.3.tgz", + "integrity": "sha512-ehp9HEKr4wVJaBOUVxNFa+CNrsCCCZ6363/jbGhb7WpEmSRNIXjHBjFs5K2s2cXn7j/RhDieejsQJ6nfUwD6vQ==", "dev": true, "license": "MIT", "dependencies": { "aws-ssl-profiles": "^1.1.2", - "denque": "^2.1.0", "generate-function": "^2.3.1", - "iconv-lite": "^0.7.2", + "iconv-lite": "^0.7.3", "long": "^5.3.2", "lru.min": "^1.1.4", "named-placeholders": "^1.1.6", diff --git a/package.json b/package.json index a6aa3d6..417fafb 100644 --- a/package.json +++ b/package.json @@ -85,14 +85,14 @@ "@commander-js/extra-typings": "^12.1.0", "@duckdb/node-api": "1.5.3-r.2", "@duckdb/node-bindings": "1.5.3-r.2", - "@malloydata/db-bigquery": "0.0.424", - "@malloydata/db-duckdb": "0.0.424", - "@malloydata/db-postgres": "0.0.424", - "@malloydata/db-snowflake": "0.0.424", - "@malloydata/db-trino": "0.0.424", - "@malloydata/malloy": "0.0.424", - "@malloydata/malloy-connections": "0.0.424", - "@malloydata/malloy-sql": "0.0.424", + "@malloydata/db-bigquery": "0.0.429", + "@malloydata/db-duckdb": "0.0.429", + "@malloydata/db-postgres": "0.0.429", + "@malloydata/db-snowflake": "0.0.429", + "@malloydata/db-trino": "0.0.429", + "@malloydata/malloy": "0.0.429", + "@malloydata/malloy-connections": "0.0.429", + "@malloydata/malloy-sql": "0.0.429", "@types/jest": "^30.0.0", "@types/node": "^24.13.1", "@yao-pkg/pkg": "^6.20.0", diff --git a/src/malloy/build.ts b/src/malloy/build.ts index b247623..41516a8 100644 --- a/src/malloy/build.ts +++ b/src/malloy/build.ts @@ -4,7 +4,13 @@ import fs from 'fs'; import path from 'path'; import url from 'url'; import chalk from 'chalk'; -import {Runtime, Connection, PersistSource, Manifest} from '@malloydata/malloy'; +import { + Runtime, + Connection, + PersistSource, + BuildTarget, + Manifest, +} from '@malloydata/malloy'; import {malloyConfig, urlReader} from '../config'; import {out} from '../log'; import { @@ -12,28 +18,21 @@ import { createDirectoryOrError, withDuckdbLockRetry, } from '../util'; -import {flattenBuildNodes} from './build_graph'; /** - * Create a table from a SELECT statement, validating the user-supplied - * name through the dialect to get a SQL-safe canonical form. Uses - * DROP+CREATE for cross-dialect safety. + * Create a table from a SELECT statement. `tableName` must already be the + * dialect's canonical form (see `canonicalTableName`). Uses DROP+CREATE for + * cross-dialect safety. * * TODO: Move to core once this stabilizes. */ async function createTableFromSelect( conn: Connection, - source: PersistSource, tableName: string, selectSQL: string ): Promise { - const result = source.dialect.sqlValidateTableName(tableName); - if (!result.ok) { - throw new Error(`Invalid persist name '${tableName}': ${result.error}`); - } - const t = result.canonical; - await conn.runSQL(`DROP TABLE IF EXISTS ${t}`); - await conn.runSQL(`CREATE TABLE ${t} AS ${selectSQL}`); + await conn.runSQL(`DROP TABLE IF EXISTS ${tableName}`); + await conn.runSQL(`CREATE TABLE ${tableName} AS ${selectSQL}`); } /** @@ -60,6 +59,135 @@ async function manifestTableStillUsable( } } +/** Where a source was declared, as something a person can act on. */ +function declaredAt(source: PersistSource): string { + const at = source.location; + if (at === undefined) { + return source.name; + } + const where = at.url.startsWith('file://') + ? path.relative(process.cwd(), url.fileURLToPath(at.url)) + : at.url; + return `${where}:${at.range.start.line + 1}`; +} + +/** Every source that maps onto one table, for reporting. */ +function targetLabel(target: BuildTarget): string { + return target.sources.map(s => s.name).join(', '); +} + +type NameResult = {name: string} | {error: string}; + +/** + * A manifest entry has to hold a canonical table path for its dialect, so the + * CREATE and the entry must both use the form `sqlValidateTableName` returns. + * For most dialects that is the input verbatim; DuckDB's file-path branch + * quotes it. + */ +function canonicalTableName( + source: PersistSource, + requested: string +): NameResult { + const result = source.dialect.sqlValidateTableName(requested); + return result.ok + ? {name: result.canonical} + : {error: `invalid persist name '${requested}': ${result.error}`}; +} + +/** A name somebody asked for, and where they asked for it. */ +interface NameClaim { + tableName: string; + sites: string[]; +} + +function askedFor(claim: NameClaim): string { + return `'${claim.tableName}' at ${claim.sites.join(', ')}`; +} + +/** + * More than one name for one BuildID. Only one can be honored, and honoring + * it silently is how a request for a second table gets lost. + */ +function twoNamesError(claims: NameClaim[]): string { + return ( + `one table, two names: ${claims.map(askedFor).join(' and ')} — these ` + + 'sources compile to the same SQL, so they share a build and can only ' + + 'produce one table. Give them one name, or make them different ' + + 'computations.' + ); +} + +/** + * The table name for a target, from the `#@ persist name=` its sources carry. + * + * A target is one table and several sources routinely name it — `persist` is + * inherited and `extend` doesn't change the SQL — so they can disagree. + */ +function requestedName(target: BuildTarget): NameResult { + const asked = new Map(); + for (const source of target.sources) { + const name = source.annotations.parseAsTag('@').tag.text('name'); + if (name === undefined) continue; + const askers = asked.get(name) ?? []; + askers.push(declaredAt(source)); + asked.set(name, askers); + } + if (asked.size === 0) { + return { + error: '#@ persist requires a name (e.g. #@ persist name=my_table)', + }; + } + if (asked.size > 1) { + return { + error: twoNamesError( + [...asked].map(([tableName, sites]) => ({tableName, sites})) + ), + }; + } + return {name: [...asked.keys()][0]}; +} + +/** + * Who has asked for what, for the length of a run. + * + * Within a run a BuildID names one table and a table is built by one BuildID, + * but `buildFiles` plans each file separately, so two files that violate + * either direction never appear in one `BuildTargets` result. Unchecked, the + * first way round leaves the second file reading the first's manifest entry + * as up to date so its own `name=` is never built; the second way round + * builds both, one silently overwriting the other under a name the manifest + * still points two entries at. + */ +class TableClaims { + private readonly byBuildId = new Map(); + private readonly byTable = new Map(); + + /** Record a claim, or say why it can't be honored. */ + claim( + buildId: string, + connectionName: string, + claim: NameClaim + ): string | undefined { + const sameBuild = this.byBuildId.get(buildId); + if (sameBuild && sameBuild.tableName !== claim.tableName) { + return twoNamesError([sameBuild, claim]); + } + const tableKey = `${connectionName}:${claim.tableName}`; + const sameTable = this.byTable.get(tableKey); + if (sameTable && sameTable.buildId !== buildId) { + return ( + `one name, two tables: ${askedFor(claim)} and at ` + + `${sameTable.sites.join(', ')} — those sources compile to different ` + + 'SQL, so building both would leave one overwriting the other. Give ' + + 'them different names.' + ); + } + this.byBuildId.set(buildId, claim); + this.byTable.set(tableKey, {...claim, buildId}); + return undefined; + } +} + export interface BuildOptions { refresh: Set; // "connection:tableName" pairs dryRun: boolean; @@ -148,6 +276,8 @@ export async function buildFiles( const buildManifest = manifest.buildManifest; const connectionDigests: Record = {}; + const claims = new TableClaims(); + const refreshMatched = new Set(); let totalBuilt = 0; let totalUpToDate = 0; let totalErrors = 0; @@ -178,15 +308,42 @@ export async function buildFiles( continue; } + // getBuildTargets throws without the flag; ask first so a real planning + // failure isn't mistaken for "this file doesn't use persistence". + if ( + !model.modelAnnotations + .parseAsTag('!') + .tag.has('experimental', 'persistence') + ) { + continue; + } + let plan; try { - plan = model.getBuildPlan(); - } catch { - // No ##! experimental.persistence — skip this file + plan = await runtime.getBuildTargets(model); + for (const {connectionName} of plan.connections) { + if (!(connectionName in connectionDigests)) { + const connection: Connection = + await malloyConfig.connections.lookupConnection(connectionName); + connectionDigests[connectionName] = connection.getDigest(); + } + } + } catch (e) { + out(`\n${chalk.bold(displayPath)}`); + out( + ` ${chalk.red('✗')} ${chalk.red( + `failed to plan build: ${e instanceof Error ? e.message : e}` + )}` + ); + totalErrors++; continue; } - if (plan.tagParseLog.length === 0 && plan.graphs.length === 0) { + const targetCount = plan.connections.reduce( + (n, c) => n + c.targets.length, + 0 + ); + if (plan.tagParseLog.length === 0 && targetCount === 0) { continue; } @@ -199,74 +356,53 @@ export async function buildFiles( totalErrors++; } - if (plan.graphs.length === 0) { - continue; - } - - for (const graph of plan.graphs) { - const connName = graph.connectionName; - - // Get or cache the connection and its digest - if (!(connName in connectionDigests)) { - let connection: Connection; - try { - connection = - await malloyConfig.connections.lookupConnection(connName); - } catch (e) { + // Connections are independent; within one, targets arrive in dependency + // order, so a serial walk is correct with no scheduling of any kind. + for (const {connectionName: connName, targets} of plan.connections) { + for (const target of targets) { + const label = targetLabel(target); + const asked = requestedName(target); + const named = + 'error' in asked + ? asked + : canonicalTableName(target.sources[0], asked.name); + + if ('error' in named) { out( - ` ${chalk.red('✗')} ${chalk.red( - `connection "${connName}" not found: ${ - e instanceof Error ? e.message : e - }` - )}` + ` ${chalk.red('✗')} ${label} ${chalk.dim( + `(${connName})` + )} — ${chalk.red(named.error)}` ); totalErrors++; continue; } - connectionDigests[connName] = connection.getDigest(); - } - - // Flatten into dependency order - const allNodes = graph.nodes.flatMap(level => - level.flatMap(node => flattenBuildNodes([node])) - ); - const seenIds = new Set(); - const uniqueNodes = allNodes.filter(node => { - if (seenIds.has(node.sourceID)) return false; - seenIds.add(node.sourceID); - return true; - }); + const tableName = named.name; - for (const node of uniqueNodes) { - const source = plan.sources[node.sourceID]; - if (!source) continue; - - const parsed = source.annotations.parseAsTag('@'); - const tableName = parsed.tag.text('name'); - - if (!tableName) { + const conflict = claims.claim(target.buildId, connName, { + tableName, + sites: target.sources.map(declaredAt), + }); + if (conflict) { out( - ` ${chalk.red('✗')} ${source.name} ${chalk.dim( + ` ${chalk.red('✗')} ${label} ${chalk.dim( `(${connName})` - )} — ${chalk.red( - '#@ persist requires a name (e.g. #@ persist name=my_table)' - )}` + )} — ${chalk.red(conflict)}` ); totalErrors++; continue; } - const refreshKey = `${connName}:${tableName}`; - const forceRefresh = options.refresh.has(refreshKey); - - const sql = source.getSQL({ - buildManifest, - connectionDigests, - }); - const buildId = source.makeBuildId( - connectionDigests[connName], - source.getSQL() - ); + const existingEntry = buildManifest.entries[target.buildId]; + // A rename leaves the entry under its old name until the SQL changes, + // so accept either name for --refresh: the one asked for now and the + // one the table was actually built under. + const refreshKeys = [`${connName}:${tableName}`]; + if (existingEntry && existingEntry.tableName !== tableName) { + refreshKeys.push(`${connName}:${existingEntry.tableName}`); + } + const matched = refreshKeys.filter(k => options.refresh.has(k)); + matched.forEach(k => refreshMatched.add(k)); + const forceRefresh = matched.length > 0; // Already built and not in refresh list — skip, but only if the // table the manifest points to is still usable. The manifest can @@ -275,7 +411,6 @@ export async function buildFiles( // trusting it blindly produced "build complete" with no data on // disk. We probe via a Malloy compile against the same connection // so this matches what query compilation will see. - const existingEntry = buildManifest.entries[buildId]; if (existingEntry && !forceRefresh) { const usable = await manifestTableStillUsable( runtime, @@ -283,9 +418,9 @@ export async function buildFiles( existingEntry.tableName ); if (usable) { - manifest.touch(buildId); + manifest.touch(target.buildId); out( - ` ${chalk.green('✓')} ${source.name} ${chalk.dim( + ` ${chalk.green('✓')} ${label} ${chalk.dim( `(${connName})` )} — ${chalk.dim('up to date')}` ); @@ -293,7 +428,7 @@ export async function buildFiles( continue; } out( - ` ${chalk.yellow('…')} ${source.name} ${chalk.dim( + ` ${chalk.yellow('…')} ${label} ${chalk.dim( `(${connName})` )} — ${chalk.yellow( `manifest entry stale (${existingEntry.tableName} missing), rebuilding` @@ -304,7 +439,7 @@ export async function buildFiles( if (options.dryRun) { const reason = forceRefresh ? 'refresh' : 'new'; out( - ` ${chalk.yellow('○')} ${source.name} ${chalk.dim( + ` ${chalk.yellow('○')} ${label} ${chalk.dim( `(${connName})` )} — ${chalk.yellow(`would build (${reason})`)} → ${tableName}` ); @@ -312,19 +447,25 @@ export async function buildFiles( continue; } - // Build the table + // Any of target.sources will do — they share the SQL. This is the + // build SQL, not target.sql: dependencies built earlier in this run + // are already table references, because buildManifest is the live + // object manifest.update() mutates. + const source = target.sources[0]; + const sql = source.getSQL({buildManifest, connectionDigests}); + const startTime = Date.now(); try { await withDuckdbLockRetry(async () => { const connection = await malloyConfig.connections.lookupConnection(connName); - await createTableFromSelect(connection, source, tableName, sql); + await createTableFromSelect(connection, tableName, sql); }); const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); - manifest.update(buildId, {tableName}); + manifest.update(target.buildId, {tableName}); out( - ` ${chalk.green('✓')} ${source.name} ${chalk.dim( + ` ${chalk.green('✓')} ${label} ${chalk.dim( `(${connName})` )} — ${chalk.green('built')} ${chalk.dim( `(${elapsed}s)` @@ -333,7 +474,7 @@ export async function buildFiles( totalBuilt++; } catch (e) { out( - ` ${chalk.red('✗')} ${source.name} ${chalk.dim( + ` ${chalk.red('✗')} ${label} ${chalk.dim( `(${connName})` )} — ${chalk.red( `build failed: ${e instanceof Error ? e.message : e}` @@ -345,6 +486,18 @@ export async function buildFiles( } } + // A --refresh that names nothing built is almost always a typo or a name + // that has since changed, and the run otherwise reports "up to date" and + // looks like the refresh happened. + const unmatched = [...options.refresh].filter(k => !refreshMatched.has(k)); + if (unmatched.length > 0) { + out( + `\n${chalk.yellow('!')} ${chalk.yellow( + `--refresh matched no table: ${unmatched.join(', ')}` + )}` + ); + } + // Write manifest if (!options.dryRun && (totalBuilt > 0 || totalUpToDate > 0)) { if (isNewManifest) { diff --git a/src/malloy/build_graph.ts b/src/malloy/build_graph.ts deleted file mode 100644 index f9ffe0a..0000000 --- a/src/malloy/build_graph.ts +++ /dev/null @@ -1,26 +0,0 @@ -/* Copyright Contributors to the Malloy project / SPDX-License-Identifier: MIT */ - -import {BuildNode} from '@malloydata/malloy'; - -/** - * Flatten a BuildNode tree into topological order (dependencies first). - */ -export function flattenBuildNodes(nodes: BuildNode[]): BuildNode[] { - const result: BuildNode[] = []; - const seen = new Set(); - - function visit(node: BuildNode) { - if (seen.has(node.sourceID)) return; - for (const dep of node.dependsOn) { - visit(dep); - } - seen.add(node.sourceID); - result.push(node); - } - - for (const node of nodes) { - visit(node); - } - - return result; -} diff --git a/test/malloy/build.spec.ts b/test/malloy/build.spec.ts index deb4215..d557cab 100644 --- a/test/malloy/build.spec.ts +++ b/test/malloy/build.spec.ts @@ -4,7 +4,7 @@ import path from 'path'; import fs from 'fs'; import os from 'os'; import {buildFiles, BuildOptions} from '../../src/malloy/build'; -import {createBasicLogger, silenceOut} from '../../src/log'; +import * as logModule from '../../src/log'; import '../../src/connections/connection_manager'; import {loadConfig, malloyConfig} from '../../src/config'; @@ -47,6 +47,29 @@ async function runBuild( }); } +/** Run a build and return what it printed, stripped of color. */ +async function runBuildCapturing( + paths: string[], + options?: Partial +): Promise { + const lines: string[] = []; + const spy = jest.spyOn(logModule, 'out').mockImplementation(m => { + lines.push(m); + }); + try { + await runBuild(paths, options); + } finally { + spy.mockRestore(); + } + // eslint-disable-next-line no-control-regex + return lines.join('\n').replace(/\[[0-9;]*m/g, ''); +} + +/** The per-target result lines, so a test can count what actually happened. */ +function builtLines(output: string): string[] { + return output.split('\n').filter(l => l.includes(' — built')); +} + // Model with two persist sources function modelV1(): string { return `##! experimental.persistence @@ -107,6 +130,71 @@ source: by_manufacturer is recalls -> { `; } +// A persisted source and an extension of it. `persist` is inherited and +// `extend` doesn't change the SQL, so both map onto one table. +function modelInheritedPersist(): string { + return `##! experimental.persistence + +source: recalls is duckdb.table('${AUTO_RECALLS_CSV}') extend { + measure: recall_count is count() +} + +#@ persist name=by_manufacturer +source: by_manufacturer is recalls -> { + group_by: Manufacturer + aggregate: recall_count +} + +source: enriched is by_manufacturer extend { + dimension: shouty is upper(Manufacturer) +} +`; +} + +// Same, but the extension renames the table it inherited. +function modelConflictingNames(): string { + return `${modelInheritedPersist()} +#@ persist name=other_name +source: renamed is by_manufacturer extend { + dimension: quiet is lower(Manufacturer) +} +`; +} + +// The same computation under a caller-chosen name. Two files built from this +// share a BuildID, so they are one table however they are named. +function modelNamed(name: string): string { + return `##! experimental.persistence + +source: recalls is duckdb.table('${AUTO_RECALLS_CSV}') extend { + measure: recall_count is count() +} + +#@ persist name=${name} +source: by_manufacturer is recalls -> { + group_by: Manufacturer + aggregate: recall_count +} +`; +} + +// A named persist whose SQL varies with the grouping, so two of these are +// two BuildIDs however they are named. +function modelNamedGrouping(name: string, groupBy: string): string { + return `##! experimental.persistence + +source: recalls is duckdb.table('${AUTO_RECALLS_CSV}') extend { + measure: recall_count is count() +} + +#@ persist name=${name} +source: grouped is recalls -> { + group_by: ${groupBy} + aggregate: recall_count +} +`; +} + // Model with no persist sources function modelNoPersist(): string { return `##! experimental.persistence @@ -128,8 +216,8 @@ function modelNoFlag(): string { describe('build command', () => { beforeAll(async () => { originalXDG = process.env['XDG_CONFIG_HOME']; - createBasicLogger(); - silenceOut(); + logModule.createBasicLogger(); + logModule.silenceOut(); }); beforeEach(async () => { @@ -277,6 +365,24 @@ describe('build command', () => { ); }); + it('a source and its extension are one table', async () => { + const file = writeModel('test.malloy', modelInheritedPersist()); + + const output = await runBuildCapturing([file]); + + // Both sources are reported on one line, against one build. Asserting + // only on the manifest would not show the merge: they share a BuildID + // either way, so a builder that walked them separately still ends up + // with one entry — after building the table twice. + expect(builtLines(output)).toHaveLength(1); + expect(builtLines(output)[0]).toContain('by_manufacturer, enriched'); + + const manifest = readManifest(); + expect(Object.values(manifest.entries).map(e => e.tableName)).toEqual([ + 'by_manufacturer', + ]); + }); + it('rebuild same model is all up-to-date', async () => { const file = writeModel('test.malloy', modelV1()); await runBuild([file]); @@ -302,6 +408,50 @@ describe('build command', () => { const manifest = readManifest(); expect(Object.keys(manifest.entries)).toHaveLength(0); }); + + it('errors when two sources on one table ask for different names', async () => { + const file = writeModel('test.malloy', modelConflictingNames()); + + const output = await runBuildCapturing([file]); + + expect(output).toContain('one table, two names'); + const manifest = readManifest(); + expect(Object.keys(manifest.entries)).toHaveLength(0); + }); + + // Both directions of the claim are cross-file: within one model they'd + // share a BuildTargets result, and the core would have merged them. + it('errors when two files name the same table differently', async () => { + const a = writeModel('a.malloy', modelNamed('table_a')); + const b = writeModel('b.malloy', modelNamed('table_b')); + + const output = await runBuildCapturing([a, b]); + + expect(output).toContain('one table, two names'); + expect(output).not.toContain('up to date'); + const manifest = readManifest(); + expect(Object.values(manifest.entries).map(e => e.tableName)).toEqual([ + 'table_a', + ]); + }); + + it('errors when two files build different SQL under one name', async () => { + const a = writeModel( + 'a.malloy', + modelNamedGrouping('foo', 'Manufacturer') + ); + const b = writeModel( + 'b.malloy', + modelNamedGrouping('foo', '`Recall Type`') + ); + + const output = await runBuildCapturing([a, b]); + + expect(output).toContain('one name, two tables'); + expect(builtLines(output)).toHaveLength(1); + const manifest = readManifest(); + expect(Object.keys(manifest.entries)).toHaveLength(1); + }); }); describe('refresh', () => { @@ -325,6 +475,18 @@ describe('build command', () => { // BuildIDs should be the same (SQL didn't change) expect(Object.keys(manifest2.entries).sort()).toEqual(buildIds1.sort()); }); + + it('reports a refresh key that matched nothing', async () => { + const file = writeModel('test.malloy', modelV1()); + await runBuild([file]); + + const output = await runBuildCapturing([file], { + refresh: new Set(['duckdb:by_manufacturer', 'duckdb:nonesuch']), + }); + + expect(output).toContain('--refresh matched no table: duckdb:nonesuch'); + expect(builtLines(output)).toHaveLength(1); + }); }); describe('strict flag', () => {