You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "eisber (via GitHub)" <gi...@apache.org> on 2026/09/23 13:11:13 UTC
[PR] Improve Power BI converter round-trip fidelity [ossie]
eisber opened a new pull request, #448:
URL: https://github.com/apache/ossie/pull/448
## Summary
- preserve explicit Power BI default values such as false key flags and active relationships
- keep relationship metadata from overriding edited Ossie endpoints or names
- preserve DAX string-literal calculated columns and exact expression whitespace
- document the strengthened round-trip guarantee
## Private corpus validation
Validated locally against two private PBIX models without committing model-derived artifacts:
- exact properties: 1,944/2,280 (85.26%) -> 2,280/2,280 (100%)
- exact semantic elements: 232/428 (54.21%) -> 428/428 (100%)
## Tests
- focused round-trip matrix: 11 passed
- Microsoft converter suite: 410 passed, 2 skipped
- coverage gate: 95.39%
- Ruff, compile, and diff checks passed
--
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] Improve Power BI converter round-trip fidelity [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #448:
URL: https://github.com/apache/ossie/pull/448#discussion_r4193856007
##########
converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py:
##########
@@ -862,33 +866,46 @@ def _convert_relationships(relationships, table_columns):
from_column, to_column = to_column, from_column
tmsl = {
- "name": stash.get("name", relationship.get("name")),
+ "name": _relationship_name(relationship, stash, endpoints),
"fromTable": from_table,
"fromColumn": from_column,
"toTable": to_table,
"toColumn": to_column,
}
+ guarded_metadata = (
+ RELATIONSHIP_ENDPOINT_METADATA
+ if "normalizedEndpoints" in stash
+ else RELATIONSHIP_CARDINALITY_METADATA
+ )
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 guarded_metadata
):
tmsl.setdefault(key, value)
_apply_ai_context(tmsl, relationship.get("ai_context"))
converted.append(prune(tmsl))
return converted
+def _relationship_name(relationship, stash, endpoints):
+ name = relationship.get("name")
+ if name is None or name == _generated_relationship_name(endpoints):
+ return stash.get("name", name)
Review Comment:
A relationship named `""` is not `None` and differs from the generated name, so it falls through to `return name`. It takes precedence over `stash["name"]`, and the original Power BI name is lost. Would a falsy check (`if not name or name == ...`) be safer here? This is a minor edge case and fine as a follow-up.
--
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] Improve Power BI converter round-trip fidelity [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #448:
URL: https://github.com/apache/ossie/pull/448#discussion_r4157481496
##########
converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py:
##########
@@ -638,21 +638,23 @@ def _source_column(expressions, name, stash):
# No expression at all: the field name is the column name by definition.
return name
- _, expression = _preferred_expression(expressions)
+ dialect, expression = _preferred_expression(expressions)
+ if dialect == DIALECT_DAX:
Review 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?
##########
converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py:
##########
@@ -932,7 +942,12 @@ def _tmsl_text(value):
whenever the value spans multiple lines. Matching that keeps generated files
diffable against ones written by Power BI itself.
"""
- return value.splitlines() if "\n" in value else value
+ if "\n" not in value and "\r" not in value:
+ return value
+ lines = value.splitlines()
Review 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?
##########
converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py:
##########
@@ -852,33 +854,41 @@ def _convert_relationships(relationships, table_columns):
from_column, to_column = to_column, from_column
tmsl = {
- "name": stash.get("name", relationship.get("name")),
+ "name": _relationship_name(relationship, stash, endpoints),
"fromTable": from_table,
"fromColumn": from_column,
"toTable": to_table,
"toColumn": to_column,
}
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
Review 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?
--
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] Improve Power BI converter round-trip fidelity [ossie]
Posted by "eisber (via GitHub)" <gi...@apache.org>.
eisber commented on code in PR #448:
URL: https://github.com/apache/ossie/pull/448#discussion_r4183032970
##########
converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py:
##########
@@ -852,33 +854,41 @@ def _convert_relationships(relationships, table_columns):
from_column, to_column = to_column, from_column
tmsl = {
- "name": stash.get("name", relationship.get("name")),
+ "name": _relationship_name(relationship, stash, endpoints),
"fromTable": from_table,
"fromColumn": from_column,
"toTable": to_table,
"toColumn": to_column,
}
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
Review Comment:
Resolved in 3b3fc4b. Legacy stashes without normalizedEndpoints now retain the old cardinality-only stale-metadata gate, so isActive, crossFilteringBehavior, and relyOnReferentialIntegrity continue to replay. Added test_a_legacy_relationship_stash_keeps_non_cardinality_metadata.
##########
converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py:
##########
@@ -638,21 +638,23 @@ def _source_column(expressions, name, stash):
# No expression at all: the field name is the column name by definition.
return name
- _, expression = _preferred_expression(expressions)
+ dialect, expression = _preferred_expression(expressions)
+ if dialect == DIALECT_DAX:
Review Comment:
Resolved in 3b3fc4b. _dataset_column_index now ignores DAX expressions when deriving physical aliases. Added test_a_dax_identifier_is_not_indexed_as_a_physical_column_alias.
##########
converters/microsoft/src/ossie_microsoft/ossie_to_semantic_model.py:
##########
@@ -932,7 +942,12 @@ def _tmsl_text(value):
whenever the value spans multiple lines. Matching that keeps generated files
diffable against ones written by Power BI itself.
"""
- return value.splitlines() if "\n" in value else value
+ if "\n" not in value and "\r" not in value:
+ return value
+ lines = value.splitlines()
Review Comment:
Resolved in 3b3fc4b. _tmsl_text now splits only LF/CRLF via explicit newline handling; standalone CR and Unicode/control separators remain scalar and unchanged. Added parameterized coverage for all listed separators.
--
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] Improve Power BI converter round-trip fidelity [ossie]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #448:
URL: https://github.com/apache/ossie/pull/448#discussion_r4082960304
##########
converters/microsoft/src/ossie_microsoft/semantic_model_to_ossie.py:
##########
@@ -332,7 +341,7 @@ def _convert_column(column, table_scope):
if column.get("type") == "calculated":
# A calculated column is DAX. It is carried across as DAX rather than rewritten
# into SQL, so no expression semantics are invented.
- expression = make_expression(text(column.get("expression", "")).strip(), DIALECT_DAX)
+ expression = make_expression(text(column.get("expression", "")), DIALECT_DAX)
Review Comment:
The converter now preserves DAX expression whitespace by no longer calling `.strip()` during import for calculated columns (and measures preserve raw text except for an emptiness check). There are new tests for multiline expressions, but there isn’t coverage for single-line expressions with leading/trailing whitespace (e.g., `' \"Earth\" '` or `' COUNTROWS(''T'') '`). Adding a test case for that would better lock in the intended behavior and prevent future regressions.
##########
converters/microsoft/src/ossie_microsoft/semantic_model_to_ossie.py:
##########
@@ -94,6 +94,15 @@
"isActive",
}
)
+_RELATIONSHIP_ENDPOINT_METADATA = frozenset(
+ {
+ "crossFilteringBehavior",
+ "fromCardinality",
+ "isActive",
+ "relyOnReferentialIntegrity",
+ "toCardinality",
+ }
+)
Review Comment:
The relationship endpoint metadata key set is duplicated here and again in `ossie_to_semantic_model.py`. This creates a drift risk (a key added/removed in one direction but not the other). Consider defining a single shared constant (e.g., in a `constants.py` module within `ossie_microsoft`) and importing it from both files.
##########
converters/microsoft/src/ossie_microsoft/semantic_model_to_ossie.py:
##########
@@ -420,11 +432,11 @@ def _convert_metrics(tables):
warn_unsupported(scope, measure, TMSL_UNSUPPORTED_MEASURE, "Apache Ossie", _PRESERVED)
original_expression = measure.get("expression")
expression = (
- text(original_expression).strip()
+ text(original_expression)
if original_expression is not None
else ""
)
- if not expression:
+ if not expression.strip():
Review Comment:
The converter now preserves DAX expression whitespace by no longer calling `.strip()` during import for calculated columns (and measures preserve raw text except for an emptiness check). There are new tests for multiline expressions, but there isn’t coverage for single-line expressions with leading/trailing whitespace (e.g., `' \"Earth\" '` or `' COUNTROWS(''T'') '`). Adding a test case for that would better lock in the intended behavior and prevent future 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] Improve Power BI converter round-trip fidelity [ossie]
Posted by "eisber (via GitHub)" <gi...@apache.org>.
eisber commented on code in PR #448:
URL: https://github.com/apache/ossie/pull/448#discussion_r4087095419
##########
converters/microsoft/src/ossie_microsoft/semantic_model_to_ossie.py:
##########
@@ -94,6 +94,15 @@
"isActive",
}
)
+_RELATIONSHIP_ENDPOINT_METADATA = frozenset(
+ {
+ "crossFilteringBehavior",
+ "fromCardinality",
+ "isActive",
+ "relyOnReferentialIntegrity",
+ "toCardinality",
+ }
+)
Review Comment:
Resolved in 77af6ec: moved the endpoint-bound metadata set to RELATIONSHIP_ENDPOINT_METADATA in _common.py and imported the same constant in both converter directions.
##########
converters/microsoft/src/ossie_microsoft/semantic_model_to_ossie.py:
##########
@@ -332,7 +341,7 @@ def _convert_column(column, table_scope):
if column.get("type") == "calculated":
# A calculated column is DAX. It is carried across as DAX rather than rewritten
# into SQL, so no expression semantics are invented.
- expression = make_expression(text(column.get("expression", "")).strip(), DIALECT_DAX)
+ expression = make_expression(text(column.get("expression", "")), DIALECT_DAX)
Review Comment:
Resolved in 77af6ec: added test_single_line_dax_whitespace_survives_a_round_trip, covering leading and trailing spaces on a calculated-column DAX string literal.
##########
converters/microsoft/src/ossie_microsoft/semantic_model_to_ossie.py:
##########
@@ -420,11 +432,11 @@ def _convert_metrics(tables):
warn_unsupported(scope, measure, TMSL_UNSUPPORTED_MEASURE, "Apache Ossie", _PRESERVED)
original_expression = measure.get("expression")
expression = (
- text(original_expression).strip()
+ text(original_expression)
if original_expression is not None
else ""
)
- if not expression:
+ if not expression.strip():
Review Comment:
Resolved in 77af6ec: the same regression test covers leading and trailing spaces on a single-line measure expression as well.
--
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] Improve Power BI converter round-trip fidelity [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #448:
URL: https://github.com/apache/ossie/pull/448
--
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