You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "MonkeyCanCode (via GitHub)" <gi...@apache.org> on 2026/08/11 04:39:22 UTC
[PR] Fix DBT converter round-trip losses entity issue [ossie]
MonkeyCanCode opened a new pull request, #315:
URL: https://github.com/apache/ossie/pull/315
<!--
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.
-->
## 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
```
## Related Issues
<!-- Link any related GitHub issues: Closes #123, Fixes #456 -->
## 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
- [x] Converter logic in `converters/` is updated to reflect spec or ontology changes
- [x] 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
- [x] All existing tests pass (`pytest` / CI green)
- [x] New functionality is covered by tests
### Compliance
- [x] ASF license headers are present on all new source files
- [x] No third-party dependencies are added without PMC/IPMC approval
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converter round-trip losses entity issue [ossie]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #315:
URL: https://github.com/apache/ossie/pull/315#discussion_r3945185006
##########
converters/dbt/tests/test_ossie_to_msi.py:
##########
@@ -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
Review Comment:
The converter fix also changes UNIQUE and FOREIGN entity classification to match on `expr`, but the added regression test only covers PRIMARY. Adding round-trip tests for UNIQUE and FOREIGN entities where `expr` differs from `name` would better protect the new behavior and prevent regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converter round-trip losses entity issue [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #315:
URL: https://github.com/apache/ossie/pull/315#discussion_r3958142670
##########
converters/dbt/src/ossie_dbt/ossie_to_msi.py:
##########
@@ -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:
Review Comment:
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 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converter round-trip losses entity issue [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on PR #315:
URL: https://github.com/apache/ossie/pull/315#issuecomment-6334012476
@MonkeyCanCode the converters have moved to their own dedicated repository (https://github.com/apache/ossie-converters). Do you mind moving (creating) the PR on the new repo? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converter round-trip losses entity issue [ossie]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #315:
URL: https://github.com/apache/ossie/pull/315#issuecomment-5249211981
@QMalcolm mind take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix DBT converter round-trip losses entity issue [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db closed pull request #315: Fix DBT converter round-trip losses entity issue
URL: https://github.com/apache/ossie/pull/315
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org