From feeaed12ec57134eedafc2ed76d44884a4936cfb Mon Sep 17 00:00:00 2001 From: Ralf Becher Date: Fri, 17 Jul 2026 15:54:52 +0200 Subject: [PATCH] orionbelt converter: dimension round-trip fidelity (name, N-per-column) Mirrors two further canonical converter fixes on top of #206 (round-trip robustness): - ralfbecher/orionbelt-semantic-layer#228: restore the OBML dimension name across the round trip. The OSI field name is the physical column code, so a round trip renamed every dimension to its code and could trip the collision fallback for names unique in the source. The name is preserved in an obml_dimension_name extension and restored on import, making the collision fallback (the space-qualified key from #206) genuinely foreign-OSI only. Non-string values are ignored so a foreign payload cannot crash the converter on an unhashable key. - ralfbecher/orionbelt-semantic-layer#229: preserve N dimensions over one column. OBML allows many dimensions on one column (grain variants, role playing); OSI is field-centric. Extras are preserved in an obml_extra_dimensions list extension with a fidelity warning (warn, not raise) and rebuilt on import. Both primary and extra dimensions carry their own synonyms and vendor extensions, with shape guards on the opaque extension data. Also adds the ASF license header to test_osi_converter_roundtrip_robustness.py, which was missing it. 177 converter tests pass; ruff clean. --- .../src/ossie_orionbelt/obml_to_osi.py | 50 ++- .../src/ossie_orionbelt/osi_to_obml.py | 114 ++++++- .../tests/test_osi_converter_properties.py | 17 +- ...test_osi_converter_roundtrip_robustness.py | 321 ++++++++++++++++++ 4 files changed, 477 insertions(+), 25 deletions(-) diff --git a/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py b/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py index db9ce97..2d0142b 100644 --- a/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py +++ b/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py @@ -405,11 +405,24 @@ def _convert_column( ext_data["obml_comment"] = col_obj["comment"] if col_obj.get("owner"): ext_data["obml_owner"] = col_obj["owner"] - # Preserve OBML-only dimension properties (timeGrain, format, resultType, etc.) + # Preserve OBML-only dimension properties (timeGrain, format, resultType, etc.). + # OBML allows N dimensions over one column (grain variants, role-playing + # via ``via``); OSI is field-centric (one dimension per field). The first + # match is the primary dimension carried on the field; any extras are + # preserved as descriptors so the reverse trip can rebuild them instead of + # dropping them silently. matched_dim: dict[str, Any] | None = None + extra_dims: list[dict[str, Any]] = [] for _dim_name, dim_obj in obml_dimensions.items(): - if dim_obj.get("dataObject") == do_name and dim_obj.get("column") == col_name: + if dim_obj.get("dataObject") != do_name or dim_obj.get("column") != col_name: + continue + if matched_dim is None: matched_dim = dim_obj + # Preserve the dimension's OBML name explicitly. The OSI field + # name is the physical code, so without this the round-trip + # would rename the dimension to its column code. Authoritative + # (do not rely on synonyms, which mix names and user aliases). + ext_data["obml_dimension_name"] = _dim_name if dim_obj.get("timeGrain"): ext_data["obml_time_grain"] = dim_obj["timeGrain"] if dim_obj.get("format"): @@ -422,7 +435,38 @@ def _convert_column( ext_data["obml_dimension_owner"] = dim_obj["owner"] if dim_obj.get("via"): ext_data["obml_dimension_via"] = dim_obj["via"] - break + # The dimension's own synonyms and vendor extensions have no + # native OSI slot (OSI has no dimension entity), so preserve them + # authoritatively here for the reverse trip. The customExtensions + # are also emitted as field foreign extensions below for OSI-tool + # visibility; that path lands them on the column on re-import, + # while this one restores them to the dimension. + if dim_obj.get("synonyms"): + ext_data["obml_dimension_synonyms"] = dim_obj["synonyms"] + if dim_obj.get("customExtensions"): + ext_data["obml_dimension_custom_extensions"] = dim_obj["customExtensions"] + else: + descriptor: dict[str, Any] = {"name": _dim_name} + for prop in ("resultType", "timeGrain", "format", "description", "owner", "via"): + if dim_obj.get(prop): + descriptor[prop] = dim_obj[prop] + # Carry the extra dimension's own synonyms and vendor extensions + # too, so it round-trips with the same fidelity as the primary. + if dim_obj.get("synonyms"): + descriptor["synonyms"] = dim_obj["synonyms"] + if dim_obj.get("customExtensions"): + descriptor["customExtensions"] = dim_obj["customExtensions"] + extra_dims.append(descriptor) + if extra_dims: + ext_data["obml_extra_dimensions"] = extra_dims + extra_names = ", ".join(d["name"] for d in extra_dims) + self.warnings.append( + f"Column '{do_name}.{col_name}' backs {len(extra_dims) + 1} OBML " + f"dimensions; OSI represents one dimension per field, so the " + f"{len(extra_dims)} additional dimension(s) ({extra_names}) are " + f"preserved via an OBSL extension for the reverse conversion but " + f"are not natively visible to other OSI tools." + ) field["custom_extensions"] = [ { "vendor_name": _VENDOR_OBML, diff --git a/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py b/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py index 6ca635d..b5d03cd 100644 --- a/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py +++ b/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py @@ -582,10 +582,20 @@ def _extract_dimensions(self, datasets: list) -> dict: if isinstance(ai_ctx, dict) and ai_ctx.get("synonyms"): dim_def["synonyms"] = list(ai_ctx["synonyms"]) # Restore OBML-only dimension properties from custom_extensions + restored_name: str | None = None + extra_descriptors: list[Any] = [] for ext in field.get("custom_extensions", []): if ext.get("vendor_name") in _OBML_VENDOR_READ: try: ext_data = json.loads(ext.get("data", "{}")) + # Extension data is opaque to ``validate_osi``, so a + # foreign payload may put any JSON here. Only accept a + # non-empty string as the dimension name (it becomes a + # dict key); otherwise ignore it and fall back to the + # field name. + _name = ext_data.get("obml_dimension_name") + if isinstance(_name, str) and _name: + restored_name = _name if ext_data.get("obml_time_grain"): dim_def["timeGrain"] = ext_data["obml_time_grain"] if ext_data.get("obml_dimension_format"): @@ -598,28 +608,98 @@ def _extract_dimensions(self, datasets: list) -> dict: dim_def["owner"] = ext_data["obml_dimension_owner"] if ext_data.get("obml_dimension_via"): dim_def["via"] = ext_data["obml_dimension_via"] + # The dimension's own synonyms / vendor extensions, + # restored authoritatively to the dimension. Opaque + # foreign data, so keep only well-shaped entries. + _syns = ext_data.get("obml_dimension_synonyms") + if isinstance(_syns, list): + clean_syns = [s for s in _syns if isinstance(s, str) and s] + if clean_syns: + dim_def["synonyms"] = clean_syns + _exts = ext_data.get("obml_dimension_custom_extensions") + if isinstance(_exts, list): + clean_exts = [e for e in _exts if isinstance(e, dict)] + if clean_exts: + dim_def["customExtensions"] = clean_exts + # Additional dimensions over the same column, preserved + # by the export because OSI has no slot for them. + _extras = ext_data.get("obml_extra_dimensions") + if isinstance(_extras, list): + extra_descriptors = _extras except (json.JSONDecodeError, TypeError): pass break - # Dimension names must be unique across the model. When the same - # field name occurs in more than one data object (e.g. Orders.date - # and Invoices.date), qualify the later one with its data object - # 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}" - suffix = 2 - while key in dimensions: - key = f"{ds_name} {field_name} {suffix}" - suffix += 1 - self.warnings.append( - f"Dimension name '{field_name}' occurs in multiple data " - f"objects; emitted '{ds_name}.{field_name}' as dimension " - f"'{key}' to avoid a collision." - ) - dimensions[key] = dim_def + # Prefer the dimension's restored OBML name (export stashes it on + # the field). The OSI field name is the physical code, so this is + # what keeps an OBML-origin round-trip from renaming dimensions to + # their code. Drop it from synonyms to avoid a self-referential + # alias. + if restored_name and dim_def.get("synonyms"): + dim_def["synonyms"] = [s for s in dim_def["synonyms"] if s != restored_name] + if not dim_def["synonyms"]: + del dim_def["synonyms"] + base_name = restored_name or field_name + self._insert_dimension(dimensions, ds_name, base_name, dim_def) + # Rebuild any additional OBML dimensions the export preserved for + # this column (OSI is one-dimension-per-field). Each descriptor is + # opaque foreign-modifiable data, so guard its shape. + for desc in extra_descriptors: + if not isinstance(desc, dict): + continue + dname = desc.get("name") + if not (isinstance(dname, str) and dname): + continue + extra_def: dict[str, Any] = { + "dataObject": ds_name, + "column": field_name, + "resultType": desc.get("resultType") or abstract_type, + } + for prop in ("timeGrain", "format", "description", "owner", "via"): + value = desc.get(prop) + if isinstance(value, str) and value: + extra_def[prop] = value + # Restore the extra dimension's own synonyms / vendor + # extensions. Opaque foreign data, so keep only well-shaped + # entries (string synonyms; dict extensions). + syns = desc.get("synonyms") + if isinstance(syns, list): + clean_syns = [s for s in syns if isinstance(s, str) and s] + if clean_syns: + extra_def["synonyms"] = clean_syns + exts = desc.get("customExtensions") + if isinstance(exts, list): + clean_exts = [e for e in exts if isinstance(e, dict)] + if clean_exts: + extra_def["customExtensions"] = clean_exts + self._insert_dimension(dimensions, ds_name, dname, extra_def) return dimensions + def _insert_dimension( + self, dimensions: dict[str, Any], ds_name: str, base_name: str, dim_def: dict[str, Any] + ) -> None: + """Insert ``dim_def`` under a unique key. + + Dimension names must be unique across the model. When ``base_name`` + already names a dimension on a *different* data object (foreign OSI where + two datasets share a bare field name and no OBML-origin name was + restored), qualify the later one with its data object and warn instead of + silently overwriting the earlier dimension. A restored OBML name is unique + by construction, so the qualification is foreign-OSI only. + """ + key = base_name + if key in dimensions and dimensions[key].get("dataObject") != ds_name: + key = f"{ds_name} {base_name}" + suffix = 2 + while key in dimensions: + key = f"{ds_name} {base_name} {suffix}" + suffix += 1 + self.warnings.append( + f"Dimension name '{base_name}' occurs in multiple data " + f"objects; emitted '{ds_name}.{base_name}' as dimension " + f"'{key}' to avoid a collision." + ) + dimensions[key] = dim_def + def _convert_metrics(self, osi_metrics: list, ds_map: dict) -> tuple[dict, dict]: """ Convert OSI metrics to OBML measures and metrics. diff --git a/converters/orionbelt/tests/test_osi_converter_properties.py b/converters/orionbelt/tests/test_osi_converter_properties.py index 4d07303..5b9433e 100644 --- a/converters/orionbelt/tests/test_osi_converter_properties.py +++ b/converters/orionbelt/tests/test_osi_converter_properties.py @@ -270,21 +270,28 @@ def test_data_object_comment_roundtrip(self): class TestDimensionProperties: """Dimension properties roundtrip. - Note: OSI uses column *code* as the field name, so after roundtrip the - dimension key becomes the physical code (ORDER_DATE). + The OSI field name is the physical column code, but the export stashes the + dimension's OBML name in an ``obml_dimension_name`` extension, so the + round-trip restores the original key (``Order Date``), not the code (#220). """ + def test_dimension_name_roundtrip(self): + result = _roundtrip(_OBML_FULL) + # The OBML dimension name is restored, not renamed to the column code. + assert "Order Date" in result["dimensions"] + assert "ORDER_DATE" not in result["dimensions"] + def test_dimension_result_type_roundtrip(self): result = _roundtrip(_OBML_FULL) - assert result["dimensions"]["ORDER_DATE"].get("resultType") == "date" + assert result["dimensions"]["Order Date"].get("resultType") == "date" def test_dimension_description_roundtrip(self): result = _roundtrip(_OBML_FULL) - assert result["dimensions"]["ORDER_DATE"].get("description") == "Order date dimension" + assert result["dimensions"]["Order Date"].get("description") == "Order date dimension" def test_dimension_owner_roundtrip(self): result = _roundtrip(_OBML_FULL) - assert result["dimensions"]["ORDER_DATE"].get("owner") == "analytics" + assert result["dimensions"]["Order Date"].get("owner") == "analytics" class TestCumulativeMetricDataType: diff --git a/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py b/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py index 02c79f2..1e80efa 100644 --- a/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py +++ b/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py @@ -1,3 +1,20 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + """Regression tests for issue #201: OSI<->OBML converter round-trip fidelity and validation robustness. @@ -17,6 +34,8 @@ import json from typing import Any +import pytest + import ossie_orionbelt.converter as conv # --------------------------------------------------------------------------- @@ -253,6 +272,308 @@ def test_both_dimensions_survive(self) -> None: assert any("multiple data objects" in w for w in c.warnings) +# --------------------------------------------------------------------------- +# #220: OBML dimension name restored across the round-trip +# --------------------------------------------------------------------------- + + +class TestDimensionNameRoundTrip: + """An OBML-origin round-trip restores each dimension's name (the OSI field + name is the physical code, so without the ``obml_dimension_name`` extension + the dimension would be renamed to its code) and never trips the collision + fallback, since the restored names are unique by construction.""" + + _OBML = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "FACT_ORDERS", + "database": "WH", + "schema": "PUBLIC", + "columns": { + "Order Date": {"code": "order_dt", "abstractType": "date"}, + "Region": {"code": "rgn", "abstractType": "string"}, + "Amount": {"code": "amt", "abstractType": "float"}, + }, + }, + "Invoices": { + "code": "FACT_INV", + "database": "WH", + "schema": "PUBLIC", + "columns": {"Invoice Date": {"code": "inv_dt", "abstractType": "date"}}, + }, + }, + # Names deliberately differ from their column codes; both date dims would + # collide on the code path if names were not restored. + "dimensions": { + "Order Placed On": { + "dataObject": "Orders", + "column": "Order Date", + "resultType": "date", + }, + "Sales Region": {"dataObject": "Orders", "column": "Region", "resultType": "string"}, + "Invoice Raised On": { + "dataObject": "Invoices", + "column": "Invoice Date", + "resultType": "date", + }, + }, + "measures": { + "Revenue": { + "columns": [{"dataObject": "Orders", "column": "Amount"}], + "aggregation": "sum", + "resultType": "float", + } + }, + } + + def _roundtrip(self) -> tuple[dict[str, Any], list[str]]: + osi = conv.OBMLtoOSI(self._OBML, model_name="sales").convert() + c = conv.OSItoOBML(osi) + return c.convert(), c.warnings + + def test_names_restored_not_renamed_to_code(self) -> None: + obml, _ = self._roundtrip() + assert set(obml["dimensions"]) == { + "Order Placed On", + "Sales Region", + "Invoice Raised On", + } + + def test_no_collision_fallback_for_obml_origin(self) -> None: + _, warnings = self._roundtrip() + assert not any("collision" in w.lower() for w in warnings), warnings + + def test_restored_name_not_left_as_a_synonym(self) -> None: + obml, _ = self._roundtrip() + # The dimension must not carry its own restored name as a synonym. + for name, dim in obml["dimensions"].items(): + assert name not in dim.get("synonyms", []) + + def test_primary_dimension_synonyms_and_extensions_preserved(self) -> None: + # The primary (single) dimension on a column keeps its own synonyms and + # vendor extensions across the round-trip, not just the scalar props. + obml = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "o", + "database": "W", + "schema": "P", + "columns": {"Status": {"code": "status", "abstractType": "string"}}, + } + }, + "dimensions": { + "Status": { + "dataObject": "Orders", + "column": "Status", + "synonyms": ["state", "condition"], + "customExtensions": [{"vendor": "TABLEAU", "data": '{"role": "dim"}'}], + } + }, + } + back = conv.OSItoOBML(conv.OBMLtoOSI(obml).convert()).convert() + dim = back["dimensions"]["Status"] + assert dim.get("synonyms") == ["state", "condition"] + assert dim.get("customExtensions") == [{"vendor": "TABLEAU", "data": '{"role": "dim"}'}] + + @pytest.mark.parametrize("bad", [["x"], 5, "", {}, None]) + def test_non_string_restored_name_is_ignored(self, bad: Any) -> None: + # obml_dimension_name is opaque to validate_osi, so a foreign payload may + # put any JSON there. A non-string (would be an unhashable dict key) must + # be ignored and fall back to the field name, never crash the converter. + osi = { + "version": "0.2.0.dev0", + "semantic_model": [ + { + "name": "s", + "datasets": [ + { + "name": "Orders", + "source": "WH.PUBLIC.orders", + "fields": [ + { + "name": "dt", + "expression": { + "dialects": [{"dialect": "ANSI_SQL", "expression": "dt"}] + }, + "dimension": {}, + "custom_extensions": [ + { + "vendor_name": "ORIONBELT", + "data": json.dumps({"obml_dimension_name": bad}), + } + ], + } + ], + } + ], + } + ], + } + obml = conv.OSItoOBML(osi).convert() + assert list(obml["dimensions"]) == ["dt"] + + +# --------------------------------------------------------------------------- +# #222: N OBML dimensions over one column +# --------------------------------------------------------------------------- + + +class TestMultipleDimensionsPerColumn: + """OBML allows N dimensions over one column (grain variants, role-playing); + OSI is one-dimension-per-field. The export preserves the extras via an + extension and warns (not silent); the import rebuilds them all (#222).""" + + _OBML = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "FACT", + "database": "WH", + "schema": "PUBLIC", + "columns": { + "date": {"code": "order_date", "abstractType": "date"}, + "Amount": {"code": "amt", "abstractType": "float"}, + }, + } + }, + "dimensions": { + "Order Day": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "day", + }, + "Order Month": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "month", + }, + "Order Quarter": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "quarter", + }, + }, + "measures": { + "Revenue": { + "columns": [{"dataObject": "Orders", "column": "Amount"}], + "aggregation": "sum", + "resultType": "float", + } + }, + } + + def _roundtrip(self) -> tuple[dict[str, Any], list[str]]: + exp = conv.OBMLtoOSI(self._OBML, model_name="s") + osi = exp.convert() + return conv.OSItoOBML(osi).convert(), exp.warnings + + def test_all_dimensions_survive_with_distinct_grains(self) -> None: + obml, _ = self._roundtrip() + dims = obml["dimensions"] + assert set(dims) == {"Order Day", "Order Month", "Order Quarter"} + assert {d["timeGrain"] for d in dims.values()} == {"day", "month", "quarter"} + # All three still point at the same physical column. + assert {d["column"] for d in dims.values()} == {"order_date"} + + def test_export_warns_not_silent(self) -> None: + _, warnings = self._roundtrip() + assert any("backs 3 OBML dimensions" in w for w in warnings), warnings + + def test_extra_dimension_synonyms_and_extensions_preserved(self) -> None: + # An extra dimension's own synonyms and vendor extensions must survive, + # matching the primary's fidelity (not just the scalar props). + obml = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "F", + "database": "W", + "schema": "P", + "columns": {"date": {"code": "dt", "abstractType": "date"}}, + } + }, + "dimensions": { + "Order Day": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "day", + }, + "Order Month": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "month", + "synonyms": ["monthly date"], + "customExtensions": [{"vendor": "ACME", "data": '{"y": 2}'}], + }, + }, + } + back = conv.OSItoOBML(conv.OBMLtoOSI(obml, model_name="s").convert()).convert() + month = back["dimensions"]["Order Month"] + assert month.get("synonyms") == ["monthly date"] + assert month.get("customExtensions") == [{"vendor": "ACME", "data": '{"y": 2}'}] + + @pytest.mark.parametrize( + "bad", + [ + "not-a-list", + [5], + [{"no_name": 1}], + [{"name": ["x"]}], + [{"name": ""}], + [{"name": "OK"}, 7], + [{"name": "OK", "synonyms": "not-a-list"}], + [{"name": "OK", "synonyms": [1, {}]}], + [{"name": "OK", "customExtensions": "bad"}], + [{"name": "OK", "customExtensions": [7]}], + ], + ) + def test_malformed_extra_dimensions_never_crash(self, bad: Any) -> None: + # obml_extra_dimensions is opaque to validate_osi. A malformed payload + # (not a list, or items that are not dicts / lack a string name) must be + # skipped, never crash the converter on an unhashable key. + osi = { + "version": "0.2.0.dev0", + "semantic_model": [ + { + "name": "s", + "datasets": [ + { + "name": "Orders", + "source": "WH.PUBLIC.orders", + "fields": [ + { + "name": "dt", + "expression": { + "dialects": [{"dialect": "ANSI_SQL", "expression": "dt"}] + }, + "dimension": {}, + "custom_extensions": [ + { + "vendor_name": "ORIONBELT", + "data": json.dumps({"obml_extra_dimensions": bad}), + } + ], + } + ], + } + ], + } + ], + } + obml = conv.OSItoOBML(osi).convert() + # The primary field-name dimension is always present; any well-formed + # extra (the {"name": "OK"} case) is rebuilt, malformed items skipped. + assert "dt" in obml["dimensions"] + assert all(isinstance(k, str) for k in obml["dimensions"]) + + # --------------------------------------------------------------------------- # P2: validate_osi robustness on malformed input # ---------------------------------------------------------------------------