fix(converters): emit schema-conformant OSI documents (drop non-spec root dialects/vendors)#148
Open
andreybavt wants to merge 1 commit into
Open
Conversation
Drop the non-spec root `dialects`/`vendors` fields from OSIDocument and stop the dbt converter emitting a root dialect, so dbt->OSI output validates against core-spec/osi-schema.json. Dialects remain per-expression (no information lost). Add a regression test that schema-validates converter output.
Author
|
@khush-bhatia , let me know if this first PR follows the community guidelines. Happy to take bigger scope after this first one lands |
khush-bhatia
approved these changes
Jun 8, 2026
khush-bhatia
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the fix.
QMalcolm
approved these changes
Jun 11, 2026
QMalcolm
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me! Thank you for putting this together 🙂
Contributor
|
Thanks @andreybavt, We will merge this PR soon, we are waiting on some process to complete. |
Author
|
Thanks @khush-bhatia and @QMalcolm ! Will continue with the other PRs then |
ralfbecher
pushed a commit
to ralfbecher/OSI
that referenced
this pull request
Jun 16, 2026
…a-conformant OSI Addresses review feedback on this PR. The converter only emitted ANSI_SQL on the OBML -> OSI path, so a third-party OSI model (Snowflake/Databricks authored, or a non-SQL dialect) could lose metrics on import with only a warning, contradicting the README's lossless roundtrip claim. - Dialect catching: read ANSI_SQL, then SNOWFLAKE, then DATABRICKS expressions (SQL engines whose aggregations are ANSI-compatible); MDX/TABLEAU/MAQL are not parsed as SQL. - No silent loss: metrics with no OBML representation are preserved verbatim under an OSI-vendor model-level customExtension (obml_unconverted_metrics) and re-emitted on OBML -> OSI, with a loud LOSSY: warning. OSI -> OBML -> OSI stays lossless. - Schema conformance (aligns with PR apache#148): stop emitting root-level dialects/vendors. The published OSI core schema's root is additionalProperties:false (version + semantic_model only). Dialects live per-expression in expression.dialects[] and vendors per-entity in custom_extensions, the schema-valid homes, so no information is lost. Vendored schema updated to match published; a guard test asserts the root arrays are now rejected. - Name-collision safety: a preserved metric whose name a converted metric now owns is skipped on export, so the output has no duplicate metric names. - Idempotency: convert() resets per-run accumulators on both directions. - Docs: scope the README fidelity claim; refresh stale v0.1.1 references and the primary_key entry in the mapping analysis. - Tests: foreign-OSI fixtures covering both former drop paths, schema conformance, name collision, and idempotency.
ralfbecher
pushed a commit
to ralfbecher/OSI
that referenced
this pull request
Jun 16, 2026
…a-conformant OSI
Addresses review feedback on this PR. The converter only emitted ANSI_SQL on the
OBML -> OSI path, so a third-party OSI model (Snowflake/Databricks authored, or a
non-SQL dialect) could lose metrics on import with only a warning, contradicting
the README's lossless roundtrip claim.
- Dialect catching: read ANSI_SQL, then SNOWFLAKE, then DATABRICKS expressions
(SQL engines whose aggregations are ANSI-compatible); MDX/TABLEAU/MAQL are not
parsed as SQL.
- Identifier resolution: parsed dataset.column references are resolved against
the OSI dataset/field map case-insensitively and with SQL quoting stripped, so
Snowflake forms like SUM(ORDERS.AMOUNT) or SUM("Orders"."amount") map to the
real Orders.amount. Unmappable references preserve the metric verbatim instead
of emitting a dangling reference.
- No silent loss: metrics with no OBML representation are preserved verbatim
under an OSI-vendor model-level customExtension (obml_unconverted_metrics) and
re-emitted on OBML -> OSI, with a loud LOSSY: warning. OSI -> OBML -> OSI stays
lossless.
- Schema conformance (aligns with PR apache#148): stop emitting root-level
dialects/vendors. The published OSI core schema's root is
additionalProperties:false (version + semantic_model only). Dialects live
per-expression in expression.dialects[] and vendors per-entity in
custom_extensions, the schema-valid homes, so no information is lost. Vendored
schema updated to match published; a guard test asserts the root arrays are
now rejected.
- Name-collision safety: a preserved metric whose name a converted metric now
owns is skipped on export, so the output has no duplicate metric names.
- Idempotency: convert() resets per-run accumulators on both directions.
- Docs: scope the README fidelity claim; refresh stale v0.1.1 references and the
primary_key entry in the mapping analysis.
- Tests: foreign-OSI fixtures covering both former drop paths, identifier
resolution, schema conformance, name collision, and idempotency.
Contributor
|
@andreybavt Because of the rename, there are some merge conflicts, could you please resolve the conflicts. |
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
3 participants
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.
Problem
Output from the dbt→OSI converter fails OSI's own validator (
validation/validate.py)against the published core schema:
Because the CLI writes via
to_osi_yaml(), everydbt→OSIconversion produces anon-conformant document.
Root cause
OSIDocument(python/src/osi/models.py) declared optional root-leveldialects/vendorsfields that aren't incore-spec/osi-schema.json, whose root isadditionalProperties: false(onlyversion+semantic_model). The dbt converterset
dialects=[self._dialect], sodialectswas emitted at the document root.Fix
dialects/vendorsfields fromOSIDocument.Per-expression dialect tagging (
OSIExpression.dialects) is unchanged and remains theschema-valid home for dialects, so no information is lost. Dialect selection still
flows end-to-end; the two affected converter tests now assert it on the per-expression path.
Regression guard
Adds
converters/dbt/tests/test_schema_conformance.py, which converts a representativemanifest and validates the emitted document (YAML and JSON, for
ANSI_SQLandSNOWFLAKE) againstcore-spec/osi-schema.json, reusingvalidation/validate.py.CI now fails if a converter emits a non-conformant document root again.
Testing
validate.pyon converter output: fails before ('dialects' was unexpected),passes after.
Out of scope
Whether OSI should support a document-level (default) dialect is the open discussion in
#52 (one dialect per document) and #16 (default dialect at dataset level). This PR takes
no position and makes no schema change - it only aligns the reference model and
converter with the schema as published today.