Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
This PR strengthens round-trip fidelity between Power BI (TMSL) and Apache Ossie by preserving explicitly serialized defaults and DAX expression formatting, and by preventing preserved relationship metadata from overriding user-edited Ossie relationships.
Changes:
- Preserve explicit
falsekey flags (isKey,isUnique) andisActiverelationship state across round trips. - Preserve DAX calculated column expressions exactly (including whitespace and string-literal expressions).
- Prevent stale relationship metadata from replaying when Ossie relationship endpoints/names have been edited; document the stronger guarantee.
| File | Description |
|---|---|
| converters/microsoft/tests/test_semantic_model_to_ossie.py | Adds focused regression tests for explicit defaults, DAX whitespace/string literals, and relationship metadata precedence. |
| converters/microsoft/src/ossie_microsoft/semantic_model_to_ossie.py | Preserves explicit false flags and DAX whitespace; expands relationship endpoint metadata tracking. |
| converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py | Refines relationship name/metadata precedence rules; preserves multiline/trailing-newline DAX formatting on export. |
| converters/microsoft/README.md | Updates documentation to reflect strengthened round-trip guarantees. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for key, value in stash.items(): | ||
| if key not in _RELATIONSHIP_CONTROL_KEYS and ( | ||
| metadata_is_current or key not in _RELATIONSHIP_CARDINALITY_KEYS | ||
| metadata_is_current or key not in RELATIONSHIP_ENDPOINT_METADATA |
There was a problem hiding this comment.
Widening this gate from the cardinality keys to RELATIONSHIP_ENDPOINT_METADATA means a stash with no normalizedEndpoints and a custom relationship name (e.g. customer_sales) now loses isActive, crossFilteringBehavior and relyOnReferentialIntegrity on export. Those stashes fail the legacy "name equals generated name" check, so metadata_is_current is false.
Before this change only the cardinality keys were dropped, so crossFilteringBehavior: bothDirections was replayed. Now it is dropped silently, so a round trip that used to be lossless loses the cross-filter direction.
Could we keep the old cardinality-only gating for stashes without normalizedEndpoints? Or, if older stashes are intentionally unsupported, could we say so in the README and add a test for a legacy stash with a custom name?
|
|
||
| _, expression = _preferred_expression(expressions) | ||
| dialect, expression = _preferred_expression(expressions) | ||
| if dialect == DIALECT_DAX: |
There was a problem hiding this comment.
_source_column now returns None for DAX, but _dataset_column_index still adds an alias for any dialect's expression. A DAX-only field whose expression is a bare identifier would be indexed as an alias for a source column. Relationship endpoint validation could then accept a column that is never emitted as a physical column.
Should the index skip DAX expressions so the two stay consistent?
| return value.splitlines() if "\n" in value else value | ||
| if "\n" not in value and "\r" not in value: | ||
| return value | ||
| lines = value.splitlines() |
There was a problem hiding this comment.
Nit: splitlines() splits on more than \n and \r: also \x0b, \x0c, \x1c-\x1e, \x85, \u2028, and \u2029. Expressions containing those characters get re-joined with \n on read, so the "original whitespace preserved" claim doesn't hold for them.
Splitting on \n and \r\n explicitly would preserve them. Otherwise, could we narrow the docstring or the test name?
Summary
Private corpus validation
Validated locally against two private PBIX models without committing model-derived artifacts:
Tests