Skip to content

Fix DBT converter round-trip losses entity issue #315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MonkeyCanCode wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix DBT converter round-trip losses entity issue #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5a8b3c
Fix DBT round-trip losses entity issue
MonkeyCanCode Aug 11, 2026
b9a7c28
Fix conflict
MonkeyCanCode Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions converters/dbt/src/ossie_dbt/ossie_to_msi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def _classify_field(
Aggregation info lives on metrics (`metric_aggregation_params`), not on
semantic model measures, so there is no measure classification step.
"""
if field.name in primary_key_cols:
if field.name in primary_key_cols or expr in primary_key_cols:

@jbonofre jbonofre Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that matching by expr membership (in addition to name) opens a false-positive collision risk: if an unrelated field's expr happens to equal another field's key expression, it gets misclassified.

For instance, if we have a dataset orders with primary_key=["id"] and two fields sharing expr="id" (something like customer_id and foo_id) both get classified as PRIMARY entities, and the dimension is lost.

On the other hand, a genuine FK field whose expr collides with another field's PK expr gets classified PRIMARY instead of FOREIGN, losing the relationship's cardinality on round-trip.

Since expr isn't guaranteed unique across fields the way name is, this trades one false-negative bug for a false-positive one 😄

entities.append(
PydanticEntity(
name=field.name,
Expand All @@ -201,7 +201,7 @@ def _classify_field(
)
)
return
if field.name in unique_key_cols:
if field.name in unique_key_cols or expr in unique_key_cols:
entities.append(
PydanticEntity(
name=field.name,
Expand All @@ -214,7 +214,7 @@ def _classify_field(
)
)
return
if field.name in foreign_key_cols:
if field.name in foreign_key_cols or expr in foreign_key_cols:
entities.append(
PydanticEntity(
name=field.name,
Expand Down
23 changes: 23 additions & 0 deletions converters/dbt/tests/test_ossie_to_msi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,16 @@
from ossie import OssieDataType, OssieDimension
from ossie_dbt.msi_to_ossie import MSIToOssieConverter
from ossie_dbt.ossie_to_msi import OssieToMSIConverter
from metricflow_semantic_interfaces.test_utils import semantic_model_with_guaranteed_meta
from metricflow_semantic_interfaces.type_enums import (
AggregationType,
DimensionType,
EntityType,
MetricType,
)
from tests.helpers import (
_entity,
_manifest,
_ossie_dataset,
_ossie_doc,
_ossie_field,
Expand Down Expand Up @@ -445,3 +449,22 @@ def test_ossie_to_msi_to_ossie_preserves_structure(self, snapshot: SnapshotAsser
assert metrics[0].name == "revenue"
assert metrics[0].expression.dialects[0].expression == "SUM(orders.amount)"
assert ossie_doc.to_ossie_yaml() == snapshot

def test_entity_with_expr_different_from_name_survives_round_trip(self) -> None:
"""A PRIMARY entity whose expr differs from its name is not lost on MSI -> OSI -> MSI."""
orders = semantic_model_with_guaranteed_meta(
name="orders",
entities=[_entity("customer_id", entity_type=EntityType.PRIMARY, expr="id")],
)

ossie_doc = MSIToOssieConverter().convert(_manifest(semantic_models=[orders])).output

assert ossie_doc.semantic_model[0].datasets[0].primary_key == ["id"]

back = OssieToMSIConverter().convert(ossie_doc).output
sm = back.semantic_models[0]

assert len(sm.entities) == 1
assert sm.entities[0].name == "customer_id"
assert sm.entities[0].expr == "id"
assert sm.entities[0].type is EntityType.PRIMARY