From e69f0c6e3615e000012d4fb987235b72d41c36c4 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Wed, 2 Sep 2026 11:43:55 +0530 Subject: [PATCH 1/3] fix(dbt): substitute DERIVED metric references in a single pass _resolve_derived ran one re.sub per input metric over the string produced by the previous iteration, so each pass re-scanned text that earlier passes had inserted. A metric named after a column appearing in an already-inlined expression was expanded twice, e.g. `gross - net` with gross = SUM(orders.net) yielded SUM(orders.SUM(orders.net_amount)). Passing the resolved expression as re.sub's replacement also let it be read as a template: a backslash surviving from a metric filter was reinterpreted, turning `LIKE 'a\b'` into a literal backspace character (and raising re.error for sequences such as \d). Collect the references first and substitute them in one pass with a callback replacement, which is not template-expanded, fixing both. --- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 22 +++++- converters/dbt/tests/test_msi_to_ossie.py | 74 ++++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index 3132cdcb..ab46c606 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -338,8 +338,16 @@ def _resolve_derived( """Resolve a DERIVED metric by substituting each input metric's expression into the expr string. Compound sub-expressions (DERIVED/RATIO) are wrapped in parentheses to preserve operator precedence. + + All references are substituted in a single pass. Substituting them one at a + time would re-scan text inserted by an earlier reference, so a metric named + after a column appearing in an already-inlined expression would be expanded + twice. The replacement is a callback rather than a string so that backslashes + in the resolved SQL (e.g. from a `LIKE 'a\\b'` filter) are inserted verbatim + instead of being interpreted as `re.sub` template escapes. """ expr = metric.type_params.expr or "" + replacements: Dict[str, str] = {} for input_metric in metric.type_params.metrics or []: ref = input_metric.alias if input_metric.alias else input_metric.name dep_metric = self._lookup_metric(metric_index, input_metric.name, f"DERIVED metric '{metric.name}'") @@ -347,8 +355,18 @@ def _resolve_derived( resolved = self._resolve_metric_expression(dep_metric, metric_index, cache, input_filter) if dep_metric.type in (MetricType.DERIVED, MetricType.RATIO): resolved = f"({resolved})" - expr = re.sub(rf"\b{re.escape(ref)}\b", resolved, expr) - return expr + replacements[ref] = resolved + + if not replacements: + return expr + + # The `\b` anchors already stop a short reference from matching inside a + # longer identifier; sorting by length keeps the alternation order stable + # and independent of the order metrics happen to be declared in. + pattern = re.compile( + r"\b(" + "|".join(re.escape(ref) for ref in sorted(replacements, key=len, reverse=True)) + r")\b" + ) + return pattern.sub(lambda match: replacements[match.group(0)], expr) @staticmethod def _build_entity_index( diff --git a/converters/dbt/tests/test_msi_to_ossie.py b/converters/dbt/tests/test_msi_to_ossie.py index 98102628..8975ed40 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -644,6 +644,80 @@ def test_derived_metric_uses_alias_for_substitution(self) -> None: profit_ossie = next(m for m in _ossie_metrics(result) if m.name == "profit") assert profit_ossie.expression.dialects[0].expression == "SUM(orders.amount) - SUM(orders.cost_amount)" + def test_derived_metric_does_not_re_expand_an_inlined_reference(self) -> None: + """A reference is substituted once, even when its name appears in already-inlined text. + + `gross` inlines to `SUM(orders.net)`, which contains the name of the second + input metric. Substituting references one at a time would expand `net` inside + that text as well. + """ + sm = semantic_model_with_guaranteed_meta( + name="orders", + measures=[ + _measure("gross", agg=AggregationType.SUM, expr="net"), + _measure("net", agg=AggregationType.SUM, expr="net_amount"), + ], + ) + gross_m = _simple_metric("gross", "gross") + net_m = _simple_metric("net", "net") + margin = PydanticMetric( + name="margin", + description=None, + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="gross - net", + metrics=[ + PydanticMetricInput(name="gross"), + PydanticMetricInput(name="net"), + ], + ), + filter=None, + metadata=default_meta(), + config=None, + ) + result = ( + MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[gross_m, net_m, margin])).output + ) + + margin_ossie = next(m for m in _ossie_metrics(result) if m.name == "margin") + assert margin_ossie.expression.dialects[0].expression == "SUM(orders.net) - SUM(orders.net_amount)" + + def test_derived_metric_preserves_backslashes_from_a_filter(self) -> None: + """Backslashes in an inlined expression survive substitution verbatim. + + A resolved expression is inserted as a literal, not as a `re.sub` replacement + template, so an escape sequence such as `\\b` in a filter's SQL is not + reinterpreted (`\\b` would otherwise become a backspace character). + """ + sm = semantic_model_with_guaranteed_meta( + name="orders", + measures=[_measure("revenue", agg=AggregationType.SUM, expr="amount")], + ) + revenue_m = _simple_metric("revenue", "revenue") + revenue_m.filter = _filter(r"{{ Dimension('order__path') }} LIKE 'a\b'") + scaled = PydanticMetric( + name="scaled", + description=None, + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="revenue * 2", + metrics=[PydanticMetricInput(name="revenue")], + ), + filter=None, + metadata=default_meta(), + config=None, + ) + result = MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[revenue_m, scaled])).output + + revenue_ossie = next(m for m in _ossie_metrics(result) if m.name == "revenue") + scaled_ossie = next(m for m in _ossie_metrics(result) if m.name == "scaled") + assert revenue_ossie.expression.dialects[0].expression == ( + r"SUM(CASE WHEN order__path LIKE 'a\b' THEN orders.amount END)" + ) + assert scaled_ossie.expression.dialects[0].expression == ( + r"SUM(CASE WHEN order__path LIKE 'a\b' THEN orders.amount END) * 2" + ) + def test_derived_metric_nested(self, snapshot: SnapshotAssertion) -> None: sm = semantic_model_with_guaranteed_meta( name="orders", From 040bb53ce33f53f96baf6226a999883a63199024 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Wed, 2 Sep 2026 12:43:23 +0530 Subject: [PATCH 2/3] fix(dbt): give the reference alternation a total order sorted(replacements, key=len, reverse=True) is only deterministic for references of differing lengths; ties keep insertion order, which is the order the input metrics are declared in. Sort by length and then by name so the compiled pattern is fully stable. Matching is unaffected: the \b anchors and the equal length mean at most one tied alternative can match at a given position either way. --- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index ab46c606..2bf978b5 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -361,10 +361,10 @@ def _resolve_derived( return expr # The `\b` anchors already stop a short reference from matching inside a - # longer identifier; sorting by length keeps the alternation order stable + # longer identifier; sorting by length (then name) keeps the alternation order stable # and independent of the order metrics happen to be declared in. pattern = re.compile( - r"\b(" + "|".join(re.escape(ref) for ref in sorted(replacements, key=len, reverse=True)) + r")\b" + r"\b(" + "|".join(re.escape(ref) for ref in sorted(replacements, key=lambda ref: (-len(ref), ref))) + r")\b" ) return pattern.sub(lambda match: replacements[match.group(0)], expr) From 6952a43a09a73ed3303c9024918f163e0ceea5d8 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Mon, 7 Sep 2026 18:56:57 +0530 Subject: [PATCH 3/3] fix(dbt): reject an ambiguous duplicate DERIVED metric reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collecting the references into a dict changed the behaviour for a DERIVED metric that lists the same input metric twice under one reference: the sequential re.sub applied the first occurrence, the dict keeps the last. For two unaliased occurrences carrying different filters, `expr` holds a single token for both, so neither choice is more correct than the other. MetricFlow does not reject the shape upstream — its DerivedMetricRule._validate_alias_collision only compares entries that set an alias — so raise here instead, and point at aliases as the fix. Occurrences that resolve to identical SQL are redundant rather than ambiguous and are still accepted. --- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 15 +++++ converters/dbt/tests/test_msi_to_ossie.py | 59 ++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index 2bf978b5..3dcb3e3a 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -345,6 +345,14 @@ def _resolve_derived( twice. The replacement is a callback rather than a string so that backslashes in the resolved SQL (e.g. from a `LIKE 'a\\b'` filter) are inserted verbatim instead of being interpreted as `re.sub` template escapes. + + Listing the same input metric twice under one reference is rejected when the + two occurrences resolve differently (e.g. distinct per-input filters and no + aliases): the expression has a single token for them, so either resolution + would be an arbitrary choice. MetricFlow does not reject this shape upstream — + `DerivedMetricRule._validate_alias_collision` only compares entries that set an + alias. Occurrences that resolve identically are redundant rather than ambiguous + and are accepted. """ expr = metric.type_params.expr or "" replacements: Dict[str, str] = {} @@ -355,6 +363,13 @@ def _resolve_derived( resolved = self._resolve_metric_expression(dep_metric, metric_index, cache, input_filter) if dep_metric.type in (MetricType.DERIVED, MetricType.RATIO): resolved = f"({resolved})" + previous = replacements.get(ref) + if previous is not None and previous != resolved: + raise ValueError( + "DERIVED metric references an input metric that is listed more than once with " + "differing resolutions, making the reference ambiguous; give each occurrence a " + f"distinct alias: metric_name={metric.name!r}, reference={ref!r}" + ) replacements[ref] = resolved if not replacements: diff --git a/converters/dbt/tests/test_msi_to_ossie.py b/converters/dbt/tests/test_msi_to_ossie.py index 8975ed40..32dc81bf 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -718,6 +718,65 @@ def test_derived_metric_preserves_backslashes_from_a_filter(self) -> None: r"SUM(CASE WHEN order__path LIKE 'a\b' THEN orders.amount END) * 2" ) + def test_derived_metric_rejects_a_reference_listed_twice_with_differing_filters(self) -> None: + """An input metric listed twice under one reference, resolving differently, is ambiguous. + + MetricFlow accepts this shape — `DerivedMetricRule._validate_alias_collision` + only compares entries that set an alias — so the converter has to reject it + rather than silently pick one of the two filters. + """ + sm = semantic_model_with_guaranteed_meta( + name="orders", + measures=[_measure("revenue", agg=AggregationType.SUM, expr="amount")], + ) + revenue_m = _simple_metric("revenue", "revenue") + both = PydanticMetric( + name="both", + description=None, + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="revenue", + metrics=[ + PydanticMetricInput(name="revenue", filter=_filter("{{ Dimension('order__region') }} = 'EU'")), + PydanticMetricInput(name="revenue", filter=_filter("{{ Dimension('order__region') }} = 'US'")), + ], + ), + filter=None, + metadata=default_meta(), + config=None, + ) + with pytest.raises(ValueError, match="listed more than once"): + MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[revenue_m, both])) + + def test_derived_metric_accepts_a_reference_listed_twice_resolving_identically(self) -> None: + """A redundant duplicate is not ambiguous: both occurrences resolve to the same SQL.""" + sm = semantic_model_with_guaranteed_meta( + name="orders", + measures=[_measure("revenue", agg=AggregationType.SUM, expr="amount")], + ) + revenue_m = _simple_metric("revenue", "revenue") + doubled = PydanticMetric( + name="doubled", + description=None, + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="revenue + revenue", + metrics=[ + PydanticMetricInput(name="revenue"), + PydanticMetricInput(name="revenue"), + ], + ), + filter=None, + metadata=default_meta(), + config=None, + ) + result = ( + MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[revenue_m, doubled])).output + ) + + doubled_ossie = next(m for m in _ossie_metrics(result) if m.name == "doubled") + assert doubled_ossie.expression.dialects[0].expression == "SUM(orders.amount) + SUM(orders.amount)" + def test_derived_metric_nested(self, snapshot: SnapshotAssertion) -> None: sm = semantic_model_with_guaranteed_meta( name="orders",