`_build_relationships` emits relationships whose to_columns is not a key, producing OSI that downstream consumers reject
- Dominant language
- Python
- Stars
- 2.1k
- Forks
- 267
- Avg merge
- 4d 20h
- Merged PRs (30d)
- 24
Description
`MSIToOSIConverter._build_relationships` pairs *every* combination of datasets sharing an entity name, including pairs
where **both** sides declare that entity as `FOREIGN`. The resulting relationship has a `to_columns` that is not a
primary or unique key of the `to` dataset, which contradicts the spec's own definition of that field and is rejected by
downstream consumers.
`core-spec/osi-schema.json` defines:
```json
"to_columns": {
"description": "Primary/unique key columns in the 'to' dataset"
}
```
**Root cause**
`converters/dbt/src/ossie_dbt/msi_to_osi.py:395-421`, `_build_relationships`:
```python
for entity_name, entries in entity_index.items():
for entry_a, entry_b in combinations(entries, 2):
```
Every pair is emitted. Direction is then chosen in `_relationship_direction` (same file, `:368-392`):
```python
if a_is_one_side and not b_is_one_side: # FOREIGN -> PRIMARY/UNIQUE ok
...
if b_is_one_side and not a_is_one_side: # PRIMARY/UNIQUE <- FOREIGN ok
...
# Same cardinality tier — use alphabetical order for determinism.
if ds_a <= ds_b:
...
```
The final branch handles two cases that are not equivalent:
- **PRIMARY↔PRIMARY** (or UNIQUE) — `to_columns` *is* a key of the `to` dataset. Valid 1:1 join, and covered by
`test_same_type_entities_produce_relationship`. This should keep working.
- **FOREIGN↔FOREIGN** — `to_columns` is *not* a key of the `to` dataset. This is the invalid case, and it has no test.
In a star schema every fact table declares the same conformed-dimension entity, so this fires between every pair of
fact tables. For *N* fact models sharing one entity, `combinations` yields N(N+1)/2 relationships of which only N are
valid — 465 vs 30 at N=30.
**Repro** (HEAD `88e0011148283302c9a04cd0287e00e0b9d87354`), using the repo's own test helpers:
```python
import sys
sys.path.insert(0, "src"); sys.path.insert(0, "."); sys.path.insert(0, "../../python/src")
from ossie_dbt.msi_to_osi import MSIToOSIConverter
from tests.helpers import _entity, _manifest
from metricflow_semantic_interfaces.test_utils import semantic_model_with_guaranteed_meta
from metricflow_semantic_interfaces.type_enums import EntityType
accounts = semantic_model_with_guaranteed_meta(
name="accounts",
entities=[_entity("account", EntityType.PRIMARY, "account_id")])
bookings = semantic_model_with_guaranteed_meta(
name="bookings",
entities=[_entity("booking", EntityType.PRIMARY, "booking_id"),
_entity("account", EntityType.FOREIGN, "account_id")])
trips = semantic_model_with_guaranteed_meta(
name="trips",
entities=[_entity("trip", EntityType.PRIMARY, "trip_id"),
_entity("account", EntityType.FOREIGN, "account_id")])
res = MSIToOSIConverter().convert(_manifest(semantic_models=[accounts, bookings, trips])).output
for r in res.semantic_model[0].relationships:
print(f"{r.from_dataset} -> {r.to} on {r.from_columns}/{r.to_columns}")
```
Run from `converters/dbt/`.
Observed:
```
bookings -> accounts on ['account_id']/['account_id']
trips -> accounts on ['account_id']/['account_id']
bookings -> trips on ['account_id']/['account_id'] <-- account_id is not a key of `trips`
```
Expected: the first two only.
**Downstream impact**
Passing this through `converters/snowflake` (which is a faithful passthrough for relationships — `_convert_relationship`
only renames fields) and into `SYSTEM$CREATE_SEMANTIC_VIEW_FROM_YAML`:
```
ProgrammingError: 010208 (42601): SQL compilation error:
The referenced key in the relationship 'F_BOOKING REFERENCES F_TRIP'
must be the primary or unique key of the referenced entity.
```
One invalid relationship fails the entire semantic view. Removing only the FOREIGN↔FOREIGN relationship from an
otherwise identical document makes the same call succeed, so it is the sole cause. Note the *valid* fact→fact
relationship between the same two datasets (`bookings -> trips on trip_id`, from the shared `trip` entity) is accepted —
the problem is specifically joining on a column that is a key of neither side.
**Suggested fix**
Skip the pair when neither entry is `PRIMARY`/`UNIQUE`, in `_build_relationships`:
```python
one_side = {EntityType.PRIMARY, EntityType.UNIQUE}
if entry_a.entity_type not in one_side and entry_b.entity_type not in one_side:
continue
```
This preserves `test_same_type_entities_produce_relationship` (PRIMARY↔PRIMARY still emits). A `ConverterIssue` for the
skipped pairs would be friendlier than silence, if that fits the existing issue-reporting pattern.
`tests/test_msi_to_osi.py::TestRelationshipConversion` has no FOREIGN↔FOREIGN case, which is why this shape isn't
covered.
**Note:** the same `_build_relationships` implementation is vendored in `dbt-labs/metricflow` as
`metricflow/converters/msi_to_osi.py` (byte-identical apart from one word in the docstring) and ships inside dbt-core
1.12, where it generates `target/osi_document.json`. Worth fixing in both.
Happy to send a PR if useful.
Contributor guide
Research direction
Start in converters/dbt/src/ossie_dbt/msi_to_osi.py, reading _relationship_direction and _build_relationships around lines 368-421. Add a FOREIGN↔FOREIGN regression case in tests/test_msi_to_osi.py::TestRelationshipConversion, then run the converter tests from converters/dbt. Done means invalid relationships are absent while PRIMARY↔PRIMARY and FOREIGN-to-key relationships still pass; check the vendored metricflow implementation noted in the issue as well.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- data-engineering
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100