mdcode: add Knowledge Catalog pull for the semantic model - #277
Conversation
d196c07 to
90634ae
Compare
| project: string; | ||
| location: string; | ||
| entryGroup: string; | ||
| model?: string; // limit to a single model by name (default: all) |
There was a problem hiding this comment.
Can there be multiple models in your entry group?
There was a problem hiding this comment.
Yes — one entry group can hold many models. Each semantic-model entry is a separate anchor; the reader groups the semantic-entity/semantic-metric entries under their anchor by parentEntry and returns one reconstructed model per anchor (modelsFromCatalogResources). --model narrows both the fetch and the write to a single anchor. Now documented in the new Pull section of docs/semantic-model.md.
| @@ -0,0 +1,256 @@ | |||
| // Serializes the Semantic Model IR (./ir) back to the open AI-first semantics | |||
There was a problem hiding this comment.
The file name is a little bit too general. It is named as serialize, but serialize from what to what? Can we be clear?
There was a problem hiding this comment.
| } | ||
|
|
||
|
|
||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
I remember there are some files like load_knowledge_catalog or something of this sort. So, should we have a separate file for this new capability? Or if we put them in the same file, is it going to be too crowded? Please think through. We need to make sure the code is very clean.
There was a problem hiding this comment.
Agreed — pulled the new code into its own files: the KC reader is now kc_converter.ts and the network pull is pull_kc.ts. knowledge_catalog.ts and deploy_knowledge_catalog.ts return to their #278 state (emit-only / push-only). This is scaffolding for the eventual two-layer split — pure converters (osi_converter/kc_converter/bigquery) vs push/pull orchestration — with the remaining halves moving in after #278 merges and no further renames. (2f0531b)
| @@ -0,0 +1,290 @@ | |||
| // Behavior specification for the semantic-model serializer | |||
| // (src/libts/semantic/serialize.ts). | |||
There was a problem hiding this comment.
Please think through the tests. Usually, I prefer to have a test fixture so that you know the input and output files as a whole, not like validated one row at a time. You don't get the full picture if that's the case. Can you think through all these tests in this PR and organize in a better way?
There was a problem hiding this comment.
Reorganized around committed golden files. Each corpus fixture now has an .osi.golden.yaml (IR → OSI) and a .pull.golden.yaml (KC entries → IR → OSI), so you see the whole input and output as files rather than row-by-row asserts — and diffing the two shows exactly what a Knowledge Catalog round trip drops (keys, ai_context, labels, relationships; an is_time dimension collapses to a bare {}). The IR round-trip and the focused mapping/warning tests are kept as invariants alongside the goldens. Test files renamed to match their modules: osi_converter/kc_converter/pull_kc.test.ts. (2f0531b)
b28f4f5 to
58bfd99
Compare
|
Added
Each is a mechanical move + import repoint deferred until #278 merges, so this PR keeps the old push files at their #278 state and only introduces the new-pattern files. |
| | Flag | Effect | | ||
| |------|--------| | ||
| | `--dry-run` | Reconstruct from the catalog and report what would be written, but write no files. | | ||
| | `--model <name>` | Pull a single model by name; other models in the entry group are left alone. | |
There was a problem hiding this comment.
for now we assume there's only on model in the entry group. We don't need --model flag
|
|
||
| Pull writes with the same last-write-wins policy as the core pull: a model that | ||
| already exists locally is overwritten in place, and a local-only document (one | ||
| with no matching catalog entry) is left untouched — pull never deletes. |
There was a problem hiding this comment.
"pull never deletes" --- can we fail pull by default if the id of the model available in the entryGroup that we're pulling from is different from the id of the model available locally on disk? if user wants to proceed then they'll need to explicitly use the new --force-remove flag we already support in push. If force-remove is specified then pull will erase the existing model and will pull the model from the KC.
| > | ||
| > **Recovered, but normalized** — the content survives, the form changes: | ||
| > - Relationship *names* come back lowercased/hyphenated (the catalog stores the | ||
| > name only in the link id, e.g. `Places Order` → `places-order`). |
There was a problem hiding this comment.
if we change relationship name on push then kcmd push should warn user about name change.
| > name only in the link id, e.g. `Places Order` → `places-order`). | ||
| > - A field marked as a dimension comes back as a bare `dimension: {}` marker, | ||
| > without its detail (`is_time`, and so on). | ||
| > - A metric authored with no data type comes back as an explicit `Decimal` |
There was a problem hiding this comment.
I think we should use 'Opaque' type instead (for both fields and metrics) if type is not provided by the user.
| > (push must write a type, and defaults it to `NUMERIC`). | ||
| > | ||
| > **Not recovered** — push never wrote these, so pull cannot return them: | ||
| > - Entity keys / unique keys. |
There was a problem hiding this comment.
pk/unique keys should be persisted in the schema aspect. let's add their support in a follow up PR.
There was a problem hiding this comment.
Agreed — tracked as a follow-up (persist pk/unique keys in the schema aspect). Listed under "Deferred" in #298; not in this follow-up.
| const linkType = schemaJoinLinkType(entry.entryType); | ||
| const res = await cat.lookupEntryLinks(opts.project, opts.location, { | ||
| entry: entry.name, | ||
| entryLinkTypes: linkType ? [linkType] : undefined, |
There was a problem hiding this comment.
why fallback to undefined here? The linkType is always schema-join. If schemaJoinLinkType() returns undefined then it's an error and kcmd pull should fail.
| }); | ||
| if (res.status !== 200 || !res.result) { | ||
| return { | ||
| warning: `failed to fetch entry links for '${entry.name}' (status ${ |
There was a problem hiding this comment.
hm. do we differentiate between legitimate no entry links case and failure during fetch? If entity has no relationships then it's ok (no warning needed), but if there's a fetch error then we should fail pull here. Let's not ignore real errors.
| // Defense in depth: keep only the requested model even if the reader surfaced | ||
| // another anchor (e.g. a child whose parentEntry pointed outside the scope). | ||
| let models = read.models; | ||
| if (opts.model) { |
There was a problem hiding this comment.
we should assume there's only 1 model in the entryGroup. if there's more than one kcmd pull print their ids and should fail. (bad state on the KC side)
| // emitter referenced). Used to filter :lookupEntryLinks to just the | ||
| // relationship links. Returns undefined for an entryType with no recognizable | ||
| // base. | ||
| function schemaJoinLinkType(entryType: string): string|undefined { |
There was a problem hiding this comment.
I thought schema-join link type has a fixed name/location. Why do we compute it here? In any case, this method should never return undefined
| const matchedAnchorNames = | ||
| new Set(targets.filter(isAnchor) | ||
| .filter( | ||
| t => (t.entry.entrySource?.displayName ?? |
There was a problem hiding this comment.
why are we using displayName here? can you elaborate?
There was a problem hiding this comment.
push writes each element's authored name into the entry's entrySource.displayName (the human-facing field); the entry id is a normalized slug. So the reader recovers the original-case name from displayName, falling back to the id slug only when it's absent (kc_converter.ts:99 for the anchor, :152 for entities). That's also why entity/model/metric names round-trip exactly, while relationship names come back lowercased/hyphenated — a schema-join link carries no displayName, so the name survives only in the link id.
Add the inverse of the KC emitter: read semantic-model / -entity / -metric entries and their aspects back into the IR, serialize to YAML, and wire a 'pull' command (with --dry-run and --model) for the semantic-model scope. Re-stacked onto the KC-push follow-ups: the emitter no longer writes importedExpression, so the reader no longer recovers it; idOf is shared from knowledge_catalog.ts; and push entry/link writes use the same bounded mapConcurrent pool as pull hydration.
…oldens + docs Addresses PR review feedback on the KC pull leg: - Rename serialize.ts -> osi_converter.ts (the OSI <-> IR converter). The name now says what it converts between; a header banner notes it currently holds only the serialize direction and that the loader migrates in post-GoogleCloudPlatform#278. - Extract the KC reader into kc_converter.ts and the network pull into pull_kc.ts, so the new capability lives in its own files rather than swelling knowledge_catalog.ts / deploy_knowledge_catalog.ts. Those two files return to their GoogleCloudPlatform#278 state (emit-only / push-only). This is scaffolding for the eventual two-layer split (pure converters vs push/pull orchestration); the remaining halves move in once GoogleCloudPlatform#278 merges, with no further file renames. - Reorganize the pull tests around committed golden artifacts: each corpus fixture now has an .osi.golden.yaml (IR -> OSI) and a .pull.golden.yaml (KC entries -> IR -> OSI). A reviewer sees the whole input and output as files and can diff the two to see exactly what a Knowledge Catalog round trip drops. Test files renamed to match their modules (osi_converter/kc_converter/pull_kc). - Document `kcmd pull` in docs/semantic-model.md: the --dry-run/--model flags, multiple models per entry group, last-write-wins overwrite policy, and the catalog-not-a-full-copy round-trip loss.
…erter-scaffold files Each new converter/orchestration file now carries an actionable TODO spelling out how the scaffold collapses once GoogleCloudPlatform#278 merges, so reviewers can see the plan: - osi_converter.ts: fold loader.ts (OSI read) in, delete loader.ts, repoint importers. - kc_converter.ts: fold generateCatalogResources (KC write) in, delete knowledge_catalog.ts, repoint importers, demote the shared idOf to a local. - pull_kc.ts: rename deploy_knowledge_catalog.ts -> push_kc.ts for push_kc/pull_kc symmetry (rename only, no logic moves).
Reviewers asked the docs to be clear about lossless vs lossy. Both directions are lossy; say so plainly and enumerate exactly what each drops: - Push to BigQuery is lossy: captures the queryable structure (node/edge tables, measures) but not descriptive metadata; non-reducible metrics are skipped. - Push to Knowledge Catalog is lossy: stores a metadata subset (keeps 1:1/1:N as schema-join links) and drops keys, ai_context, labels, vendor SQL, M:N. - Pull is lossy: recovers even less than the catalog holds (no relationships, no deploymentTargets). A push followed by a pull does not return the original file.
Pull previously dropped two things push had already written to Knowledge Catalog: the model's deployment targets (stored in the semantic-model aspect) and its 1:1/1:N relationships (stored as schema-join entry links). The reader only opened per-entry aspects and pull only fetched entries, so both were silently lost even though the catalog held them. - kc_converter: read deploymentTargets back into the GOOGLE custom_extensions block, and invert schema-join links into Relationships -- endpoints resolved by data source, FK direction and columns from the join aspect. modelsFromCatalogResources grows an entryLinks argument. Relationship names come back normalized (lowercased/hyphenated): the emitter stores the name only in the link id. - pull_kc: add a second fetch pass over the entity entries via lookupEntryLinks, deduping the undirected links (each is returned from both endpoints). - Tests cover endpoint/direction recovery, name normalization, the M:N drop, deployment-target recovery, and the pull fetch+dedup path; the .pull.golden fixtures are regenerated and the docs pull note rewritten. M:N (association) relationships remain unrecovered -- push never emits them. Writer files are untouched.
Address code-review findings on the gap-3 pull leg: - Resolve schema-join endpoints from the link's entryReferences, matched by entry id, instead of a dataSource->entity index. The id is unique per entity (fixes two entities sharing a table collapsing last-wins) and is stable across the project-number/id normalization lookupEntry applies to entries but lookupEntryLinks does not apply to link references (fixes relationships silently dropping on the live path). The schema-join aspect is now used only for FK direction + join columns; undecidable direction keeps the reference order and warns rather than dropping the edge. - Dedup entry links by a sorted endpoint-pair key when a link has no name (shared linkDedupKey, reused by pull_kc) so a nameless link returned from both endpoints is not counted twice. - Rewrite the pull 'lossy' note in the user guide as recovered-exactly / recovered-but-normalized / not-recovered bullets. Tests: shared-table endpoints, un-normalized project-number references, prefix-stripping across tricky model names, and nameless-link dedup.
Two fixture-coverage gaps from the round-trip review: 1. Symmetry assertion. The golden pull files let a human eyeball what a Knowledge Catalog round trip drops, but nothing asserted it. Add a symmetry test over the converter corpus: load the authored IR, run a full emit -> read round trip, and assert the result equals the authored IR reduced to the "KC floor" (stripToKcFloor) -- the documented losses and normalizations applied to both sides. An undocumented regression (a dropped column, a lost description, an un-stripped M:N edge) now fails here even though each individual loss is already pinned by a targeted test. Export linkNamePrefix so the normalizer reproduces the relationship slug rather than reimplementing it. 2. sales_bq_graph_target had OSI/KC/pull goldens but no BigQuery golden. Add it to the BigQuery corpus and generate the golden: a valid single-node property graph with a MEASURE, so the fixture now carries a complete four-arm round-trip suite. No production behavior change; reader/emitter untouched apart from the linkNamePrefix export.
The BigQuery corpus golden path names the graph from the test's build opts (sqlgen-testing.demo.sales), ignoring the fixture's deployment target -- so the golden neither reflected the fixture's purpose nor matched a real deploy (demo.sales.sales_graph). That target-driven name is already covered by deploy_bigquery.test.ts, making this golden redundant. Revert the corpus addition and remove the generated file; the fixture keeps its OSI/KC/pull goldens and the pull symmetry assertion.
Address code-review findings on the KC->IR reader (kc_converter.ts): - readMetric: when the expression does not pin exactly one known entity (none, or several), fall back to the attach entity metricAspectData persisted instead of dropping it. A cross-entity metric now recovers its authored entity. - readField: skip a schema field with no name (warn) instead of emitting a Field with an undefined name into the entity. - linkNamePrefix: mirror linkSlug's 63-char cap and trailing-hyphen re-strip so the read-side prefix stays aligned with the emitter's link id. - Header comment: add importedExpression (vendor SQL) and String/Opaque- typed metrics to the documented round-trip loss list. Add regression tests for the metric-entity fallback and the nameless-field skip.
PR GoogleCloudPlatform#278 landed on main gating the KC SQL-expression fields off by default: the emitter now omits the per-field `semantics` block (field expression + role) and the metric expression unless `--emit-expressions` is set, and the committed emitter golden was regenerated without them. The pull leg was built against the old always-emit behavior, so after rebasing onto main it read back less than its tests and goldens assumed. Reconcile the reader to the new default: - readMetric no longer warns when a metric aspect has no expression -- that is now the expected default, not a malformed aspect. It still derives the attach entity from the expression when one is present, else from the persisted `entity`, and still warns on an expression that pins no known entity. - Reader header + user guide: field/metric expressions and the DIMENSION role are recovered only from an `--emit-expressions` push; a default push -> pull drops them. - Tests: split the round trip into roundTrip (default, drops the semantics block) and roundTripFull (`--emit-expressions`, keeps it); point the lossless-slice, DIMENSION, canonical-expression, and expression-derivation cases at the full round trip, and add a case pinning the default drop. The symmetry floor now strips field/metric expressions and the dimension role. - Regenerate the .pull.golden.yaml fixtures: they drop only expressions and bare dimension markers, matching the default emitter golden.
25e36c2 to
8768a40
Compare
Distinguish inherent pull losses from write-side limits: relationship names (not stored in the schema-join aspect) and non-canonical deployment targets (dropped on write) could round-trip faithfully with a writer change. Reader already recovers everything the catalog holds.
A non-canonical BigQuery Graph deployment target fails push at the validation gate, before any leg and for every --target, so nothing is written to BigQuery or Knowledge Catalog. Correct the earlier writer-side follow-up note, which wrongly implied such targets are silently dropped on write, and sharpen the relationship-name follow-up to name the server-side schema-join aspect-type template as the fix.
|
LGTM |
|
Thanks for the review. Since #277 is already merged, I've addressed these in follow-up #298. Mapping:
The three schema-aspect enrichments (pk/unique keys, |
* mdcode: address KC pull review comments (post-merge follow-up) Follow-up to the merged PR #277 (semantic-model KC pull leg), addressing dlychagin-gg's inline review. Reader/orchestration/CLI only; the writer is untouched. - pull_kc: hard-fail on any entry-hydration fetch error and on a non-200 link lookup; an empty link list stays a silent no-op. - Enforce one semantic model per entry group: drop the `--pull --model` flag; >1 anchor is a hard error naming both. - Add `pull --force-remove` to authorize replacing a differently-named local model with the catalog's, rather than leaving two in the group. - Schema-join link type is a fixed built-in constant, never undefined. - push: warn when a relationship name will be normalized (KC stores it only in the link id, so a pull returns it lowercased/hyphenated). - Default untyped fields and metrics to Opaque (STRING + OTHER) instead of guessing STRING/NUMERIC; a typeless metric now round-trips un-typed. - osi_schema: exclude the expression-free .pull.golden.yaml fixtures from the OSI guardrail with a TODO(#290); PR #290 restores expressions on push+pull, at which point they can be schema-checked again. - Docs + tests + regenerated goldens to match. * mdcode: harden pull reconcile, concurrency, and OSI guardrail Follow-up fixes from a review pass over the pull review-fixes PR: - pull reconcile compares local vs catalog models by their on-disk path, not the raw name, so a model name that sanitizes to the same file (e.g. 'a/b' -> 'a_b.yaml') is recognized as the same model rather than flagged as a stale conflict on every re-pull. - mapConcurrent stops claiming new items once a worker throws: Promise.all already rejects on the first failure, so the remaining fan-out was wasted fetches whose rejections surfaced as unhandled-rejection noise. - the OSI guardrail now validates .pull.golden.yaml fixtures too, tolerating ONLY the known missing-`expression` gap (TODO #290) instead of skipping them by filename -- so unrelated schema drift is caught now, and the fixtures schema-check with no special-casing once #290 restores expressions.
2 participants
Follows PR4 (the KC push leg, #275, now merged). Rebased onto the merged
main, so this PR's diff is the pull delta only.What
The read counterpart of the Knowledge Catalog push leg.
kcmd pullin a semantic-model workspace now reads thesemantic-*entries back from Knowledge Catalog, reconstructs the IR, and serializes each model tocatalog/EntryGroups/<entryGroup>/<model>.yaml— the inverse of the push leg against the same built-in-type schema (go/semantic-model-kc-v2).Pieces
serialize.ts(new): pure IR → open-format YAML, the exact inverse ofloader.ts.knowledge_catalog.ts:modelsFromCatalogResources, the inverse of the emitter — parses the built-inschemaaspect (dataType/metadataType → logical type,DIMENSIONrole, per-fieldsemantics), reverses the BigQuery resource URI back to thedataSourcestring, and re-derives each metric's attach entity from its expression (as the loader does).deploy_knowledge_catalog.ts:pullKnowledgeCatalog— lists the entry group, hydrates each entry's aspects (an entity needs BOTH itssemantic-entityaspect and the built-inschemaaspect), and applies a--modelfilter (scoped to the target model's entries before hydration).SemanticModelLayout:modelPath/hasModel/writeModelDocumentwrite sink.commands.pull/main.ts:--dry-runand--model. Overwrite policy matches the core pull: last-write-wins; local-only documents are never deleted.Fidelity
IR-level, bounded by what the push leg persists. Entity keys,
ai_context, field labels,importedDialect, and relationships are not written by push (the graph edges live in the BigQuery property graph) and so do not come back. An authoredStringdatatype is indistinguishable from an un-typed field after emit and reads back as un-typed. A typeless metric round-trips asDecimal: the push leg makessemantic-metric.dataTyperequired and defaults it toNUMERIC, which the reader maps back toDecimal.Testing
Hermetic round trips are the acceptance bar (no live KC needed — PR4's server types are still nonprod-pending):
loader ↔ serializeround trip over 6 corpus fixtures (relationships, ai_context, datatypes, dimensions, custom_extensions, vendor dialects).emitter ↔ readerround trip + dataType-inverse / role / resource-URI / metric-attach unit tests.pullKnowledgeCatalogover a stubbed catalog client (aspect hydration,--modelscoping, skipped entries, foreign-entry ignore).SemanticModelLayoutwrite-path test.Full
test:semantic+test:libtsgreen;tsc --noEmitclean.