mdcode: store SQL expressions in the sql-expressions companion aspect - #290
Draft
libei wants to merge 14 commits into
Draft
mdcode: store SQL expressions in the sql-expressions companion aspect#290libei wants to merge 14 commits into
libei wants to merge 14 commits into
Conversation
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.
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.
Push and pull now move field/metric SQL through a single sql-expressions companion aspect (data_classification METADATA_AND_DATA), per the V2 "SQL Expression Storage in USL Semantic Models" proposal, instead of the core schema/semantic-metric aspects. The core aspects stay metadata-only; the schema field carries only a DIMENSION marker. A shared codec (sql_expressions.ts) fixes the aspect shape and the qualifier convention -- omitted qualifier = primary GoogleSQL (IR expression), `imported` = the vendor form (IR importedExpression) -- and the emitter and reader are exact inverses over it, so a --emit-expressions push round-trips both expression forms through pull. The source dialect label is still not stored (inferred from importedSystem), so importedDialect stays unset. Both the companion aspect and the DIMENSION marker remain gated behind --emit-expressions (off by default); a default push emits neither and matches the live published system types byte-for-byte.
libei
requested review from
amirhormati and
dlychagin-gg
and removed request for
amirhormati and
dlychagin-gg
August 14, 2026 05:04
Code-review follow-ups on the sql-expressions aspect change. - Generate each corpus pull golden from a --emit-expressions round trip instead of the default-push emitter JSON, so the reviewable <fixture>.pull.golden.yaml artifacts exhibit the recovered field/metric expressions and the corpus golden-pull test exercises that path. - Exclude the lossy KC pull reconstructions from the OSI schema guardrail: a KC round trip does not persist an imported expression's source dialect, so a recovered vendor form serializes under the `IMPORTED` placeholder, which is outside the OSI dialect enum. The pull goldens' exact content and documented losses stay pinned by kc_converter.test.ts; the guardrail keeps validating the authored fixtures and the full-fidelity OSI-converter goldens, the artifacts that must be valid OSI. - Fix a dead assertion: sf.semantics -> sf.semantic (the schema field was renamed from the plural `semantics` block to a `semantic` marker).
libei
force-pushed
the
sql-expressions-aspect
branch
from
August 14, 2026 18:07
3c6c14a to
b875998
Compare
libei
added a commit
to libei/knowledge-catalog
that referenced
this pull request
Aug 15, 2026
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 GoogleCloudPlatform#290) instead of skipping them by filename -- so unrelated schema drift is caught now, and the fixtures schema-check with no special-casing once GoogleCloudPlatform#290 restores expressions.
libei
added a commit
that referenced
this pull request
Aug 15, 2026
* 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Stores field/metric SQL in Knowledge Catalog through a single
sql-expressionscompanion aspect (data_classification: METADATA_AND_DATA) on entity and metric entries, per the V2 "SQL Expression Storage in USL Semantic Models" proposal (D. Lychagin, 2026-08-13). Adds this to both push and pull together, replacing PR #278's approach of writing expressions onto the coreschema/semantic-metricaspects.The core aspects stay metadata-only — they never carry executable SQL. The
schemafield carries only asemantic: "DIMENSION"marker.How
sql_expressions.ts(new) — a shared codec that fixes the aspect's shape and its qualifier convention: omitted qualifier = primary GoogleSQL (IRexpression);imported= the vendor form (IRimportedExpression). The emitter and reader are exact inverses over it.knowledge_catalog.ts) — attaches thesql-expressionsaspect alongsideschema/semantic-metriconly when a field/metric actually has an expression (never an empty aspect); emits theDIMENSIONmarker.kc_converter.ts) — recovers both the primary and imported forms from the aspect (matched by type suffix); reads theDIMENSIONmarker. A--emit-expressionspush now round-trips both expression forms through pull.pull_kc.ts) — requests thesql-expressionsaspect for entity and metric entries (harmless when absent).The source dialect label is still not stored (inferred from
importedSystem), soimportedDialectstays unset — the one remaining expression-side loss.Gating / compatibility
Both the companion aspect and the
DIMENSIONmarker remain gated behind--emit-expressions(off by default), because the published system-type templates don't carry them yet. A default push emits neither and matches the live published system types byte-for-byte — the committed emitter/pull golden fixtures are unchanged.Testing
bun test ./tests/libts/semantic/— 328 pass. Newsql_expressions.test.tscovers the codec (primary/imported/both/neither, inverse, first-wins, malformed/unknown-qualifier skipping); emitter, reader, and pull tests assert the companion aspect and a--emit-expressionsfield+metric round-trip.Stacking
Stacked on #277 (the KC pull PR) — this branch builds on
upstream-pr5-kc-pull, so until #277 merges the diff here also shows #277's commits; it will narrow to just this change once #277 lands. Merge #277 first.Assumes the
sql-expressionsaspect type will be provisioned in the published system-type templates before--emit-expressionsis used against live types.