orionbelt converter: fix round-trip fidelity + validation robustness#206
Merged
khush-bhatia merged 1 commit intoJul 17, 2026
Merged
Conversation
Mirrors orionbelt-semantic-layer#201 (canonical source) into the ossie converter: - Metric round-trip: OSItoOBML resolves metric references by BOTH the OSI dataset/field name and the physical code (source-table code and field expression, quoted or bare), so metrics emitted against physical codes (e.g. SUM(fact_orders.amount)) round-trip instead of dropping when a data object's code differs from its display name. Resolved refs are bracket-quoted so display names with spaces stay intact through the dataset.column parsers. - Dimension collision: _extract_dimensions qualifies a field name that occurs in more than one dataset (Orders.date / Invoices.date) with its data object and warns, instead of silently overwriting the earlier dimension. - Validation robustness: validate_osi guards its semantic loops against malformed structures (datasets/fields that are not lists of dicts) and returns schema errors instead of raising AttributeError. Adds tests/test_osi_converter_roundtrip_robustness.py. 154 converter tests pass; ruff clean; mypy clean with the dev extra.
| # instead of silently overwriting the earlier dimension. | ||
| key = field_name | ||
| if key in dimensions and dimensions[key].get("dataObject") != ds_name: | ||
| key = f"{ds_name} {field_name}" |
Contributor
There was a problem hiding this comment.
For the key field, Is the use of space here instead of . intentional ? Wouldn't it better to use the SQL identifier style and use . ?
Contributor
Author
There was a problem hiding this comment.
Good question. It comes down to what "a dimension" fundamentally is in each model:
- OSI is field-centric - a dimension is a field (a physical column), so a dot (
table.column) is the natural SQL-identifier spelling. - OBML is name-centric - a dimension is a named view onto a column. Its identity is the name; the physical reference lives in the dimension's
dataObject+columnpair, not the name. OBML names are display-grade by convention and convenience (Country Name,Total Sales), not SQL identifiers, so a dot here would make a display name read like a physical reference.
That's why the qualifier uses a space: the key becomes a name, not an identifier. It also only fires as a fallback - when two datasets share a bare field name and there's no OBML-origin name to prefer - so it stays rare.
This was referenced Jul 16, 2026
khush-bhatia
approved these changes
Jul 17, 2026
ralfbecher
changed the title
ralfbecher
changed the title
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
2 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.
Mirrors ralfbecher/orionbelt-semantic-layer#201 (the canonical converter source) into
converters/orionbelt/. Three pre-existing bugs found while reviewing #153:Metric round-trip when a data object's code differs from its display name
OSItoOBMLresolved metric references only by dataset name, but the OBML -> OSI emitter writes physical codes (e.g.SUM(fact_orders.amount)), so such metrics were dropped as unconverted on the return trip. Resolution now indexes datasets/fields by both name and physical code (source-table code + field expression, quoted or bare). Resolved refs are bracket-quoted so display names containing spaces stay intact through thedataset.columnparsers, and Snowflake/Databricks quoted identifiers resolve too.Dimension name collision across datasets
_extract_dimensionskeyed dimensions by bare field name, soOrders.dateandInvoices.datecollapsed to one. The later one is now qualified with its data object and a warning is recorded - no silent loss.validate_osirobustnessIt assumed every
datasets/fieldsvalue was a list of dicts and raisedAttributeErroron malformed input. It now guards each level and returns schema errors instead.Tests
Adds
tests/test_osi_converter_roundtrip_robustness.py. 154 converter tests pass; ruff clean; mypy clean with the dev extra.Canonical change: ralforion/orionbelt-semantic-layer#202 (merged).
Follow-up converter fixes (canonical #228 dimension-name restoration, #229 N-dimensions-per-column) are mirrored in a separate PR, not this one.