osi-orionbelt: fix converter round-trip fidelity + validation robustness (#201)#202
Merged
Merged
Conversation
…ess (#201) Fixes the three pre-existing converter bugs found during the apache/ossie#153 review: - P1 metric round-trip: OSItoOBML now resolves metric references by BOTH the OSI dataset/field name and the physical code (source-table code and bare-identifier field expression). The OBMLtoOSI emitter writes metric SQL against the physical code (e.g. SUM(fact_orders.amount)), so resolving names only dropped such metrics on the return trip when a data object's code differed from its display name. - P2 dimension collision: _extract_dimensions no longer keys dimensions by bare field name. When the same field name occurs in more than one dataset (Orders.date, Invoices.date) the later one is qualified with its data object and a warning is recorded, instead of silently overwriting the first. - P2 validation robustness: validate_osi guards its semantic loops against malformed structures (datasets/fields that are not lists of dicts) so it returns schema errors instead of raising AttributeError. Adds tests/test_osi_converter_roundtrip_robustness.py (7 regression tests). 152 package tests pass; ruff and mypy clean; API convert integration tests pass.
Addresses a review finding on the P1 fix: resolving a physical code to a
display field name containing a space (e.g. net_amount -> 'Net Amount') spliced
'Orders.Net Amount' into the intermediate SQL string, and the downstream \w+.\w+
parsers split it on the space, emitting invalid OBML ('{[Orders].[Net]} Amount')
with no LOSSY warning.
_resolve_column_refs now bracket-quotes any resolved name that is not a bare SQL
word, and the dataset.column parsers (_parse_simple_agg, _parse_expr_agg,
_decompose_complex_metric, _sql_refs_to_obml) accept bracketed identifiers and
unquote them. A metric over a spaced display-name field now converts to a valid
single-column measure (column 'Net Amount') instead of dangling.
Adds a regression test for a physical code mapped to a display field with a
space. 153 package tests pass; ruff and mypy clean.
…view P3) Unquote single-identifier field expressions and source table codes before indexing them for metric-reference resolution. A Snowflake/Databricks quoted physical column (field expression "net_amount") or quoted source table (WH.PUBLIC."fact_orders") referenced by its bare code now converts to a queryable measure instead of being preserved as a LOSSY unconverted metric. Adds a regression test for quoted source table + field expression. 154 package tests pass; ruff and mypy clean.
This was referenced Jul 15, 2026
ralfbecher
added a commit
that referenced
this pull request
Jul 15, 2026
* Bump version to 2.22.0 (osi-orionbelt 0.1.1) Minor rather than patch: sqlglot's floor moved 26 -> 30, so installs pinning sqlglot 26 cannot take this release, and Python 3.14 is newly supported. Bumps osi-orionbelt 0.1.0 -> 0.1.1 in the same tag. The PyPI publish workflow sets skip-existing on that job, so leaving it at 0.1.0 would have skipped the job silently and shipped the converter fixes (#201, #202) nowhere while the release still went green. Release contents (20 commits since v2.21.1, mostly Dependabot): - Python 3.14 across all three Docker images + CI matrix (#216) - sqlglot 30 adaptation; pyarrow <26, structlog <27 (#217) - osi-orionbelt converter roundtrip fidelity + validation robustness (#200, #201, #202) 2621 passed, 167 skipped. ruff and mypy clean. * Fix osi-orionbelt exported __version__ and guard against recurrence osi-orionbelt was bumped to 0.1.1 in pyproject.toml and uv.lock, but the module still exported __version__ = "0.1.0", so the published wheel would have self-reported 0.1.0 while its metadata said 0.1.1. Root cause: the version-ref sweep grepped for the main package's version (2.21.1) only, and osi-orionbelt is versioned independently, so its constant was never in the search. Adds tests/unit/test_version_consistency.py, which asserts every package's exported __version__ matches its pyproject version. Verified it fails on the reintroduced bug, not just passes on the fix. There was no existing release consistency check to extend. 2623 passed, 167 skipped. ruff and mypy clean.
Collaborator
Author
|
Mirroring a review question from the ossie mirror (apache/ossie#206) about the dimension-collision key using a space ( It comes down to what "a dimension" fundamentally is in each model:
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
ralfbecher
added a commit
that referenced
this pull request
Jul 16, 2026
P2 - CLI silently accepted an authored `label:` that the REST API rejects. The API enforces the JSON Schema at its ingestion boundary (api/schema_guards.py) while ModelStore.load_model stays coercion-tolerant for internal callers. The CLI is also an external boundary but called load_model directly, bypassing the guard. Add the schema check to the CLI's single load chokepoint (_local._load), so `obsl compile/execute/describe/...` fail loudly with a clean CliError, the same way the API returns 422. load_model is left unchanged. P3 - the public reference (docs/guide/model-format.md) still listed `label` as an authorable property for dimensions and metrics. Removed both rows; the DataObject and Measure tables already omit it, and the sections are documented as "dictionaries keyed by name". Test gap - test_schema_properties_are_covered_by_manifest only checks schema subset of manifest, so re-adding `label` to the schema would go unnoticed. Add: - dimension/measure/metric.label to test_removed_fields_absent_from_schema - test_authored_label_fails_schema_validation (behavioral, all three types) - test_compile_rejects_authored_label (CLI boundary, clean error, no traceback) Refs #201, #202.
ralfbecher
added a commit
that referenced
this pull request
Jul 16, 2026
P2 (follow-up) - the obml-to-osi conversion path bypassed OBML schema validation on both external surfaces, so an authored `label:` was silently coerced away rather than surfaced. Conversion is advisory-not-blocking by design (the osi-to-obml direction already validates its OSI input into `input_validation` without failing the request). Mirror that for obml-to-osi: - api/routers/convert.py: run advisory OBML input validation and surface it in the response `input_validation` field, matching osi-to-obml. Conversion still runs; a schema violation is reported, not silently dropped. - cli/_local.py::convert_obml_to_osi: validate the OBML input and prepend any schema issues to the returned warnings, so `obsl convert obml-to-osi` emits them instead of succeeding silently. Tests: - test_api_convert.py: replaced TestObmlToOsiBackwardCompat (which codified the old "input_validation is None" behavior) with TestObmlToOsiInputValidation - valid input reports schema_valid, an authored label is surfaced with the request still 200 (advisory). - test_cli.py: convert obml-to-osi with an authored label surfaces the warning, exit 0, output still produced. Refs #201, #202.
This was referenced Jul 16, 2026
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.
Fixes the three pre-existing OSI↔OBML converter bugs tracked in #201 (found during the apache/ossie#153 review, independent of the schema/constants change).
P1 - metric round-trip broke when
dataObject.code!= display nameOBMLtoOSIemits metric SQL against the physical code (e.g.SUM(fact_orders.amount)), butOSItoOBMLresolved metric references only by dataset name, so a measure overOrders.Amount(codefact_orders) came back stashed as an unconverted metric instead of a realRevenuemeasure.Fix:
_convert_metricsnow indexes datasets/fields by both the OSI name and the physical code (source-table code + bare-identifier field expression); names win on collision. Round-trip is restored, and third-party OSI that references physical codes now resolves too.P2 - dimension name collision across datasets
_extract_dimensionskeyed dimensions by bare field name, soOrders.dateandInvoices.datecollapsed to one (the second silently overwrote the first). Now the later one is qualified with its data object (Invoices date) and a warning is recorded - no silent loss.P2 -
validate_osicrashed on malformed inputIt assumed every
datasets/fieldsvalue was a list of dicts and raisedAttributeErroron e.g.datasets: "not-an-array". Now it guards each level and returns schema errors instead of raising.Tests
New
tests/test_osi_converter_roundtrip_robustness.py(7 regression tests: round-trip code-vs-name, third-party code refs, dimension collision, malformed-input no-raise, and a guard that real duplicate errors are still flagged).152 package tests pass; ruff + mypy clean;
tests/integration/test_api_convert.pypasses.Closes #201. Will be mirrored to the ossie converter (
converters/orionbelt/) after review.