Skip to content

Fix DBT converter round-trip losses entity issue - #315

Open
MonkeyCanCode wants to merge 2 commits into
apache:mainfrom
MonkeyCanCode:fix_dbt_roundtrip
Open

Fix DBT converter round-trip losses entity issue#315
MonkeyCanCode wants to merge 2 commits into
apache:mainfrom
MonkeyCanCode:fix_dbt_roundtrip

Conversation

@MonkeyCanCode

@MonkeyCanCode MonkeyCanCode commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Currently a primary, unique, and foreign entity whose expression differs from its name is silently lost on MSI → OSI → MSI conversion. This is due to the following:

From converters/dbt/src/ossie_dbt/msi_to_osi.py, we have following:

    @staticmethod
    def _extract_keys(entities: Sequence[Entity]) -> Tuple[Optional[List[str]], List[List[str]]]:
        primary_key: Optional[List[str]] = None
        unique_keys: List[List[str]] = []

        for entity in entities:
            col = entity.expr if entity.expr is not None else entity.name
            if entity.type is EntityType.PRIMARY:
                primary_key = [col]
            elif entity.type is EntityType.UNIQUE:
                unique_keys.append([col])

        return primary_key, unique_keys

However, from converters/dbt/src/ossie_dbt/osi_to_msi.py following:

        if field.name in primary_key_cols:
            entities.append(
                PydanticEntity(
                    name=field.name,
                    type=EntityType.PRIMARY,
                    expr=expr_or_none,
                    description=field.description,
                    label=field.label,
                    role=None,
                    config=None,
                )
            )
            return
        if field.name in unique_key_cols:
            entities.append(
                PydanticEntity(
                    name=field.name,
                    type=EntityType.UNIQUE,
                    expr=expr_or_none,
                    description=field.description,
                    label=field.label,
                    role=None,
                    config=None,
                )
            )
            return
        if field.name in foreign_key_cols:
            entities.append(
                PydanticEntity(
                    name=field.name,
                    type=EntityType.FOREIGN,
                    expr=expr_or_none,
                    description=field.description,
                    label=field.label,
                    role=None,
                    config=None,
                )
            )
            return

Here is the local test result with the fixed code and newly added test:

➜  dbt git:(fix_dbt_roundtrip) ✗ uv run pytest
========================================================================== test session starts ==========================================================================
platform darwin -- Python 3.11.13, pytest-9.1.1, pluggy-1.6.0
rootdir: /Users/yong/Desktop/GitHome/ossie/converters/dbt
configfile: pyproject.toml
testpaths: tests
plugins: syrupy-5.5.3
collected 100 items

tests/test_msi_to_osi.py ..................................................................                                                                       [ 66%]
tests/test_osi_to_msi.py ..................................                                                                                                       [100%]

------------------------------------------------------------------------ snapshot report summary ------------------------------------------------------------------------
5 snapshots passed.
========================================================================== 100 passed in 0.34s ==========================================================================

Related Issues

Checklist

Specification

  • Spec changes are included in core-spec/ and follow the existing structure
  • Spec changes have been discussed on the mailing list or in a linked issue
  • Breaking changes to the spec are clearly called out in the summary

Ontology

  • Ontology changes in ontology/ are consistent with spec changes
  • New or modified terms are defined and documented

Converters

  • Converter logic in converters/ is updated to reflect spec or ontology changes
  • New converters include tests under the converter's test directory

Validation

  • Validation rules in validation/ are updated if the spec changed
  • New validation cases are covered by tests

Documentation

  • docs/ is updated to reflect any user-facing changes
  • New features or behaviors are documented with examples where appropriate
  • CONTRIBUTING.md is updated if the contribution process changed

Examples

  • examples/ are added or updated for any new spec constructs or converter support

Tests

  • All existing tests pass (pytest / CI green)
  • New functionality is covered by tests

Compliance

  • ASF license headers are present on all new source files
  • No third-party dependencies are added without PMC/IPMC approval

@MonkeyCanCode

Copy link
Copy Markdown
Contributor Author

@QMalcolm mind take a look?

Copilot AI lite review requested due to automatic review settings September 6, 2026 20:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The code change also affects UNIQUE and FOREIGN entity classification, but the added regression test only covers PRIMARY, leaving key paths untested.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a round-trip conversion bug in the DBT converter where PRIMARY/UNIQUE/FOREIGN entities could be silently dropped during MSI → Ossie → MSI when the entity expr differs from the entity name.

Changes:

  • Update OssieToMSIConverter field classification to recognize keys by either field.name or the field expression (expr).
  • Add a regression test to ensure a PRIMARY entity with expr != name survives MSI → Ossie → MSI.
File summaries
File Description
converters/dbt/src/ossie_dbt/ossie_to_msi.py Classifies PRIMARY/UNIQUE/FOREIGN entities by matching either field.name or expr against key sets.
converters/dbt/tests/test_ossie_to_msi.py Adds a regression test covering the PRIMARY entity expr != name round-trip case.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants