mdcode: add semantic-model scope and BigQuery Graph push - #270
Conversation
68868b9 to
db5af00
Compare
Introduces a semantic-model scope and a push-only SemanticModel layout so the standard init/push verbs manage a locally-authored Apache Ossie model. - semantic-model source + SemanticModel layout, recorded in catalog.yaml; init scaffolds catalog/EntryGroups/<eg> and EntryLinks/<eg>. - push parses each Ossie document to the semantic IR, lowers it to CREATE OR REPLACE PROPERTY GRAPH DDL, and executes it against the project named by the model's GOOGLE custom_extension deploymentTargets (//bigquery.googleapis.com/projects/<p>/datasets/<d>/propertyGraphs/<g>). - BigQueryClient.query executes DDL synchronously via jobs.query. - Knowledge Catalog resource emit for the semantic model is a follow-on. Tests cover deploymentTargets parsing, the deploy leg (validateOnly, execute, BQ error, no-target) with the BigQuery client stubbed, and the scope/layout wiring over an authored workspace.
db5af00 to
9f946a8
Compare
|
Ready for review. @amirhormati requested; @dlychagin-gg PTAL as well (couldn't add you as a formal reviewer — not currently a repo collaborator). Live-validated against real BigQuery; details in the PR description. |
…ures Address review of the semantic-model push path: - Poll jobs.getQueryResults until the query job reports completion. A slow CREATE OR REPLACE PROPERTY GRAPH could return from jobs.query with jobComplete=false and no errors; the deploy leg treated that as success. Errors are now judged only on a completed job. - Fail (instead of silently succeeding) when a push finds no model documents or parses no models, so a misplaced Ossie file no longer reports success having deployed nothing. - Return a structured DeployResult on malformed GOOGLE custom_extension JSON rather than throwing out of deployBigQuery. - Collapse the duplicated `options.semanticModel` branch in `init`. - Correct the `--semantic-model` help text to show the 3-part scope. Move the inline Ossie documents in deploy.test.ts into fixtures (sales_bq_graph_target.yaml, sales_no_target.yaml), matching the existing fixtures/ convention, and add tests for the poll, error, and no-document paths.
Follow-up review fixes on the semantic-model BigQuery deploy leg: - When a completed query job reports errors[], consult the job's status.errorResult (via jobs.get) before failing. jobs.query / getQueryResults errors[] mixes fatal errors with non-fatal warnings, so a deploy that only produced warnings was reported as a failure; it now succeeds. getJob is only called when errors[] is non-empty, so the happy path adds no extra request. - Collapse the two overlapping empty-input guards in deployBigQuery into a single post-loop check that still distinguishes "no documents" from "no models parsed". - Add tests for the warning-vs-errorResult distinction, and reuse the existing sales_google_ext.yaml fixture to cover deploy over a non-BigQuery (Dataplex) target.
amirhormati
left a comment
There was a problem hiding this comment.
Structural
- deploy.ts is the only file in src/libts/ that writes to the console.
sync.ts, snapshot.ts, source.ts, manifest.ts, semantic/loader.ts and layouts/* all have zero console. calls — the library layer is silent by construction and src/tool/commands.ts owns output. deploy.ts adds eight: lines 157, 187, 191, 195-196, 221, 223-225.
The concrete cost is at deploy.ts:191 — the --validate-only DDL only exists on stdout, so no non-CLI caller can reach it. Widening DeployResult (deploy.ts:42-45) to {success,
details?, ddl: string[], warnings: string[], deployed: number} and printing from commands.push fixes this, and drops the spyOn(console, 'log') boilerplate from all eight deploy tests.
-
Partial deploys are silently discarded on failure.
deployed is tracked at deploy.ts:150 and incremented at 206, but the early return at deploy.ts:199-204 drops it. These are CREATE OR REPLACE, so a run that replaces graph 1 and then fails on graph 2 has already mutated production, and the operator sees only Failed to
deploy 'graph2'. Fold the count into details at minimum. -
The layout globs every EntryGroup, ignoring the scoped one.
init scaffolds catalog/EntryGroups/ (commands.ts:53), but SemanticModelLayout.init() globs EntryGroups//.yaml (layouts/semantic-model.ts:39-43). Any other group directory in the tree gets deployed too. Compounding it, layouts/semantic-model.ts:52 keys _index on
bare basename, so EntryGroups/a/sales.yaml and EntryGroups/b/sales.yaml collide silently, last-write-wins. SemanticModelSource already carries entryGroup (sources/semantic-model.ts:27); threading it into the layout constructor fixes both. -
listEntries() and loadEntry() disagree, and MCP walks straight into it.
layouts/semantic-model.ts:61-63 returns model handles; layouts/semantic-model.ts:80-83 throws for any of them. commands.push short-circuits at commands.ts:97 so the CLI is safe — but mcp.ts:27 calls listEntries() and mcp.ts:44 calls lookupEntry(name) on each result.
The MCP server will list the semantic model as an entry and then throw on read. For a push-only layout, returning [] from listEntries() with modelDocuments() as the sole accessor is the honest contract. -
kcmd pull on a semantic-model scope reports success while doing nothing.
sources/semantic-model.ts:42-45 is an empty generator, so sync.pull() iterates nothing and returns {success: true}, and commands.ts:83 prints "Successfully updated local snapshot." push got an explicit guard at commands.ts:97; pull (commands.ts:72-90) deserves the
symmetric one.
Correctness
-
Misleading error when there's no job reference. At deploy.ts:107, the while guard requires jobId. If jobComplete === false and jobId is undefined, the loop never runs, and deploy.ts:116-122 then reports did not complete after 30 polls when zero polls occurred.
-
The poll loop has no backoff. deploy.ts:107-114 depends entirely on the server honoring timeoutMs: 10000 (bigquery.ts:98). If getQueryResults returns promptly for any reason, that's 30 back-to-back requests with no pause.
-
jobs.query is issued without a location. bigquery.ts:88-91 sends only {query, useLegacySql}, and deploy.ts:197 has target.project/target.dataset in hand but passes neither. BigQuery infers location from referenced tables, which should hold for property-graph DDL,
but the live validation was one US project — worth confirming against a non-US dataset. Note getQueryResults and getJob both already accept location (bigquery.ts:96, 108), so the initial call is the odd one out. -
Identifier interpolation into DDL. deploy.ts:27 captures [^/]+ for project/dataset/graph, which permits backticks and semicolons, and qualifyGraph at semantic/bigquery.ts:676 does a bare
${parts.join('.')}with no escaping. Low severity — the document is
authored by whoever already holds the BQ credentials — but it's a parsed-data boundary, and tightening the capture groups to [A-Za-z0-9_-]+ costs nothing and rejects malformed URIs earlier with a better message. -
A document that parses to zero models is silently skipped. The modelsSeen guard at deploy.ts:211 is global. Given two docs where the second yields no models, the push succeeds and never mentions it. Per-document reporting would be more useful, especially since
loader warnings only go to stderr (deploy.ts:157).
…oy hardening Structural - deploy.ts no longer writes to the console: DeployResult now carries the generated ddl, collected warnings, and deployed count, and the CLI (commands.ts) prints them. --validate-only DDL is now reachable by non-CLI callers. - Report partial deploys: a CREATE OR REPLACE that fails mid-run notes how many graphs were already deployed (not rolled back). - Scope the SemanticModel layout glob to the manifest's entryGroup (EntryGroups/<eg>/*.yaml), avoiding unrelated groups and the cross-group basename collision. - listEntries() returns [] for the push-only layout so the MCP server no longer lists a model and then throws on read; modelDocuments() is the sole accessor. - Guard pull() for the semantic-model scope instead of printing "Successfully updated local snapshot" while doing nothing. Correctness - Clearer error when a query never completes and has no job reference to poll (previously reported "did not complete after 30 polls" with zero polls). - Back off between getQueryResults polls so a promptly-returning server is not hit MAX_QUERY_POLLS times back-to-back. - Thread the dataset location to jobs.query (resolved via getDataset, best-effort) so submit/poll/getJob agree on a region. - Tighten the deployment-target regex capture groups to [A-Za-z0-9_-]+ so backticks/semicolons cannot reach the unescaped DDL identifier. - Turn a malformed document into a clean, document-scoped failure instead of an uncaught loader exception.
|
Thanks @amirhormati — all 10 addressed in f14a632. Notes on two where I diverged slightly: Structural
Correctness 139 tests pass, |
|
A. The regex comment claims a behavior the code doesn't have. deploy.ts:32-33 says malformed URIs "fail the match and are reported rather than producing broken DDL downstream." They aren't reported — a For a typo'd URI that's actively misleading — the target is declared, it just didn't parse, and the message sends the author looking for a missing extension they already wrote. Now that the regex is strict this B. datasetLocation swallows a real failure. deploy.ts:185-186 returns undefined on any non-200, including 404. A typo'd dataset in the URI now silently skips the location pin and fails later inside the DDL with C. New permission dependency, undocumented. deploy.ts:250 means bigquery.datasets.get on the target project is now on the path for every deploy. It degrades gracefully rather than failing, which is the right D. datasetLocation is called per-target, not per-dataset. deploy.ts:250 sits inside the target loop, so a model with three graphs in one dataset issues three identical datasets.get calls. A Map<string, E. The backoff makes the exhaustion path untestable. POLL_BACKOFF_MS = 1000 (deploy.ts:108) × MAX_QUERY_POLLS = 30 (deploy.ts:103) means a test for deploy.ts:150-151 would burn 29 real seconds — which is likely F. entryExists and listEntries now disagree. layouts/semantic-model.ts:75-77 correctly returns [], but layouts/semantic-model.ts:65-68 still answers from _index — so entryExists('sales') is true for a name Still open from last round All minor, all your call — but two got slightly easier or slightly worse:
|
…ast, per-dataset location cache, testable polls Round-2 review from amirhormati on PR GoogleCloudPlatform#270: - Report BigQuery-prefixed deployment-target URIs that fail the strict match (bigQueryGraphTargets now returns {targets, malformed}); a typo'd URI is named in the error instead of the misleading "declares no target" message. - datasetLocation fails fast on 404 (typo'd dataset) as a precise pre-flight, keeps best-effort fallback on other failures (e.g. 403 without bigquery.datasets.get). - Cache the location lookup per project/dataset so a model with several graphs in one dataset issues a single datasets.get. - Thread the poll bound and backoff through DeployOptions so the job-never-completes exhaustion path is testable without burning wall-clock. - entryExists now agrees with listEntries (returns false) for the push-only layout. Also swept the round-1 minor items: derive the init entryGroup from manifest.source.entryGroup and drop unused EntryLinks scaffolding; remove the unreachable instanceof guard in push; drop the now-inaccurate hardcoded glob in the no-documents error; drop the unread DeployOptions.force; use path.basename; rename the SemanticModel layout enum value to 'semantic-model' for consistency; document --semantic-model and the datasets.get dependency in the README.
|
Thanks for the thorough second pass, @amirhormati — all of A–F plus the round-2 sweep are in A. Regex comment lied. B. C. New permission dependency. Documented in the README push section: the semantic-model push reads D. Per-target E. Untestable exhaustion path. F. Round-2 sweep — all done except one: The one I left: 145 tests pass, |
…malformed-target detection, verified query completion - Qualify under-qualified dataset sources with the scope's declared project (threaded through deployBigQuery) instead of the ambient gcloud project, which can silently drift from where the model's tables live. - Broaden malformed-target detection to a host/segment hint so a host or scheme typo (e.g. missing `.com`, `https://`) is reported rather than misclassified as "no target declared". - Fail a jobs.query 200 that carries no response body rather than treating an unverifiable response as a successful deploy. - Warn (via DeployResult.warnings) when the dataset-location pre-flight is forbidden and the deploy falls back to BigQuery's location inference. - Make `push --validate-only` a clean no-op over an empty workspace; a real push still fails on "nothing to deploy". Tests: +5 (150 pass); README documents the source-qualification default.
|
Proactive hardening from a self-review pass (
+5 tests (150 pass), tsc clean; README documents the source-qualification default. |
Adds the Knowledge Catalog push leg for the semantic-model scope, the counterpart to the merged BigQuery Graph leg (#270). Emits semantic-model/semantic-entity/semantic-metric entries against built-in dataplex-types/global system types, publishes relationships as schema-join entry links, shares model loading across legs, and reconciles removed entities/metrics on re-push. --target selects destinations (bq|kc|all); --print dumps each destination's native artifact. BigQuery leg is live-validated; the KC write path is covered hermetically (gated on the semantic-model system types CL).
Third change in the mdcode Semantic Model series, on top of the now-merged
IR (#258) and BigQuery Graph generator (#269). Targets
main.What this adds
A
semantic-modelscope and a push-onlySemanticModellayout, so theexisting
init/pushverbs manage a locally-authored Apache Ossie modelinstead of a bespoke command:
catalog.yaml(scope: semantic-model.<p>.<l>.<eg>);initscaffoldscatalog/EntryGroups/<eg>andcatalog/EntryLinks/<eg>.catalog/EntryGroups/<eg>/<model>.yaml.pushparses each document to the semantic IR (PR1 loader), lowers it toCREATE OR REPLACE PROPERTY GRAPHDDL (PR2 generator), and executes itagainst the project named by the model's
GOOGLEcustom_extensiondeploymentTargets, e.g.//bigquery.googleapis.com/projects/<p>/datasets/<d>/propertyGraphs/<g>.BigQueryClient.queryruns DDL synchronously viajobs.query.Scope / follow-on
This PR is the BigQuery leg only. Knowledge Catalog resource emit
(entries + entryLinks) for the semantic model is a follow-on;
pushcurrently deploys only the BigQuery Graph and logs that the KC emit is not
yet implemented.
Tests
deploymentTargetsparsing (GOOGLE-vendor filter, BigQuery-URI filter,malformed JSON, multiple targets).
stubbed:
--validate-only(no query), execute against the targetproject, BigQuery-error failure, and no-target failure.
snapshot over an authored workspace resolves to
SemanticModelLayoutandreads the model document while ignoring sidecars.
tsc --noEmitclean; full mdcode suite green.Live validation
Validated end-to-end against real BigQuery (
sqlgen-testing): authored anOssie model with a
//bigquery.googleapis.com/.../propertyGraphs/sales_graphdeploymentTarget, ran
kcmd init --semantic-model+kcmd push, andconfirmed:
CREATE OR REPLACE PROPERTY GRAPHDDL executes viajobs.query;MEASURE(
GRAPH_EXPAND) both reproduce a direct-SUMcontrol (per-customerrevenue correct through the FK edge, no fan-out);
pushis idempotent (second run re-deploys cleanly).