fix(orionbelt): carry field and metric datatype in both directions - #410
ralfbecher wants to merge 2 commits into
Conversation
Closes apache#409. The OrionBelt converter ignored the spec `datatype` on import (it read the non-spec `data_type` key against a lowercase map) and never wrote it on export, so other Ossie tools saw wrong or missing logical types. - Ossie -> OBML: field `datatype` maps to OBML `abstractType` (precedence: `datatype` > legacy `data_type` > name heuristic). `Decimal` narrows to `float`, `Opaque` falls back to the heuristic. The stashed `obml_abstract_type` is now restored, so OBML-origin round trips stay exact. - Ossie -> OBML: metric `datatype` maps to the exact measure/metric `dataType` (`Decimal` -> `decimal(18, 2)`), unless one was restored from the extension. - OBML -> Ossie: fields always emit `datatype`; measures/metrics emit it only when an explicit `dataType` is declared, keeping round trips idempotent. Ported from the downstream osi-orionbelt package, where this shipped in ralforion/orionbelt-semantic-layer#246.
| """ | ||
| if not data_type: | ||
| return None | ||
| normalized = data_type.strip().lower() |
There was a problem hiding this comment.
data_type.strip() assumes data_type is a string, but nothing guarantees that if the source OBML document is malformed or hand-authored (the type hint is str | None, not enforced). A non-string dataType (e.g. 123) crashes with AttributeError: 'int' object has no attribute 'strip' and aborts the whole conversion.
I think it's worth guarding with isinstance(data_type, str) up front (returning None otherwise) rather than relying on upstream schema validation that may not have run?
There was a problem hiding this comment.
Good catch, fixed in 7375c7e. obml_datatype_to_ossie now returns None for anything that is not a non-empty string, so a hand-authored dataType: 123 is treated like an unknown type and the conversion carries on. I applied the same guard on the import side: a non-string field datatype or legacy data_type counts as absent, and a non-string metric datatype maps to nothing. Before, a list there raised TypeError on the dict lookup. Covered by TestMalformedDatatype.
| # `dataType` (physical vocabulary: `integer`/`double`/`decimal(p, s)`/...), which | ||
| # is where `Decimal` genuinely belongs. So Ossie metric `datatype` maps to that | ||
| # field, not the coarse `abstractType`. | ||
| OBML_DECIMAL_DEFAULT = "decimal(18, 2)" # mirrors OrionBelt's built-in default |
There was a problem hiding this comment.
OBML_DECIMAL_DEFAULT = "decimal(18, 2)" is applied unconditionally whenever an Ossie metric has datatype: "Decimal". But OBML models can override the default numeric type via settings.defaultNumericDataType, this hardcodes past that override, so a model configured for e.g. decimal(20, 6) gets metrics silently emitted as decimal(18, 2) instead.
Should this read obml_settings.defaultNumericDataType (when present) before falling back to the "decimal(18, 2)" constant?
There was a problem hiding this comment.
Agreed, fixed in 7375c7e. An Ossie Decimal metric now takes settings.defaultNumericDataType when the model has one, and falls back to decimal(18, 2) otherwise. The settings only exist on an OBML-origin model, in the stashed obml_settings, and the importer restored them after converting metrics. So they are now read before the metrics, and the restore itself stays where it was. OrionBelt rejects a defaultNumericDataType that is not a decimal(p, s), so any other value falls back to the built-in default here too. Covered by TestDecimalDefaultFromSettings.
| # already restored from an OBML-origin extension, and skip | ||
| # Opaque/unknown (absent from the map). | ||
| ossie_dt = m.get("datatype") | ||
| if ossie_dt and not target.get("dataType"): |
There was a problem hiding this comment.
This guard skips setting dataType whenever target already has one from _apply_obml_measure_extras (the obml_data_type custom_extensions stash). But that stash can be stale, a user edits a metric's datatype in Ossie after a prior OBML round-trip, and the fresh datatype is silently dropped in favor of the old cached value, with no warning.
Should an explicit datatype on the Ossie metric take precedence over the stashed extension, since it reflects the user's latest edit?
There was a problem hiding this comment.
Yes, the edit should win, fixed in 7375c7e for metrics and for fields, which had the same problem with obml_abstract_type. I did not make datatype win unconditionally, though. On a plain round trip the stash is more exact than the map: decimal(20, 6) and bigint would come back as decimal(18, 2) and integer. So the stash is kept while it agrees with datatype, meaning it maps back to the same Ossie type. When it maps to a different type, the datatype was edited after the export and replaces it. Covered by TestEditedDatatypeBeatsStaleStash, including a check that an agreeing stash stays exact.
- A non-string `dataType` in OBML, or a non-string field or metric `datatype` in Ossie, no longer raises: it has no mapping, the same as an unknown type, instead of aborting the conversion. - An Ossie `Decimal` metric takes the model's `settings.defaultNumericDataType` when the OBML-origin model carries one, and falls back to `decimal(18, 2)` otherwise. OrionBelt only accepts a `decimal(p, s)` there, so anything else falls back too. The settings are read ahead of the metrics; they are still restored after them. - A stashed `obml_data_type` or `obml_abstract_type` is kept only while it agrees with `datatype`. When it names a different type, `datatype` was edited in Ossie after the export and wins. A stash that agrees is kept, so `decimal(20, 6)` and `bigint` still round-trip exactly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
The OrionBelt converter lost a field's logical type in both directions (#409): import read the non-spec
data_typekey against a lowercase map, so the specdatatypewas ignored in favour of the name heuristic, and export never wrotedatatypeat all. This ports the fix that already shipped in the downstreamosi-orionbeltpackage (ralforion/orionbelt-semantic-layer#246).Ossie -> OBML
datatypemaps to OBMLabstractType. Precedence:datatype> legacydata_type> name heuristic.Decimalnarrows tofloat(OBML models exact decimals at the physical layer, not inabstractType);Opaquefalls back to the heuristic.obml_abstract_typestash written on export is now restored on import, so OBML -> Ossie -> OBML stays exact through the narrowing (json,time_tz).datatypemaps to the exact measure/metricdataType(Decimal->decimal(18, 2),Integer->integer,Float->double), unless one was already restored from the extension.OBML -> Ossie
datatypefromabstractType(json->Opaque,timestamp_tz->DateTimeTz), in addition to the existing extension bookkeeping.datatypeonly when an explicitdataTypeis declared, so plain measures stay unchanged and round trips remain idempotent.datatypeabstractTypedataTypeStringstringstringIntegerintintegerFloatfloatdoubleDecimalfloatdecimal(18, 2)BooleanbooleanbooleanDate/Timedate/timedate/timeDateTimetimestamptimestampDateTimeTztimestamp_tztimestampOpaqueRelated Issues
Closes #409
Notes for reviewers
main: 6 schema-validation tests inconverters/orionbeltalready fail onmainsince the flat document change (Define one semantic model per document without a wrapper #383), e.g.test_tpcds_fixture_passes_v02_validation. They fail identically with and without this PR; the converter CI has not run since that change. Migrate converters and Python SDK to flat semantic model documents #396 migrates the converter to the flat shape and should fix them.data_type; they keep working via the legacy fallback and were left unchanged to keep this PR focused.Checklist
Converters
converters/is updated to reflect spec or ontology changesDocumentation
converters/orionbelt/ossie_obml_mapping_analysis.md) updatedTests
pytest/ CI green): 158 pass; the 6 failures above are pre-existing onmaintest_ossie_converter_datatype.py(10 tests) plus the drop-in test from OrionBelt converter drops a field's logical datatype in both directions #409 intest_ossie_metric_no_silent_loss.pyCompliance