From aef59aac22b579e03f0c8093c94cade208f7d0f8 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Wed, 2 Sep 2026 11:43:55 +0530 Subject: [PATCH 1/4] 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 638be16f0..3a0731d60 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -332,8 +332,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}'") @@ -341,8 +349,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 7ed0aa894..433323489 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -686,6 +686,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 7271b5093a262bd6a6a66eccda96c58a721961b9 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Wed, 2 Sep 2026 12:43:23 +0530 Subject: [PATCH 2/4] 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 3a0731d60..ab5ad3ce2 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -355,10 +355,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 fe662192cf390fb0699cafe966c4423c64a0b898 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Mon, 7 Sep 2026 18:56:57 +0530 Subject: [PATCH 3/4] 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 ab5ad3ce2..1fb828683 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -339,6 +339,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] = {} @@ -349,6 +357,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 433323489..8b761e8f2 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -760,6 +760,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", From f17616460e1f755bc4056a1ca1bebac22e117c9f Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Mon, 21 Sep 2026 18:15:18 +0530 Subject: [PATCH 4/4] fix(dbt): narrow the ambiguous-reference check and fail soft Addresses review feedback on the duplicate DERIVED reference guard. False positives. Two duplicate listings that are not actually ambiguous were rejected: - An occurrence restating a filter the enclosing metric already applies resolved to "(X) AND (X)" where the bare occurrence resolved to "X", so identical row sets compared unequal. _merge_filter_sqls now drops repeated fragments; AND is idempotent, so this also stops the redundant text reaching the emitted SQL. - A duplicated reference the expression never substitutes cannot affect the result, so only references the expression actually uses are checked. Hard failure. An ambiguous reference now raises a dedicated AmbiguousDerivedReferenceError, which convert() catches to drop that one metric and record AMBIGUOUS_REFERENCE_METRIC_DROPPED, matching how every other unsupported shape is handled instead of propagating a ValueError out through cli.py and ending the run. The CLI's reason table covers the new issue type, with a test asserting every type has an entry, since the lookup is by key. --- converters/dbt/README.md | 1 + converters/dbt/src/ossie_dbt/cli.py | 5 ++ .../dbt/src/ossie_dbt/converter_issues.py | 1 + converters/dbt/src/ossie_dbt/filter_utils.py | 10 ++- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 63 ++++++++++---- converters/dbt/tests/test_msi_to_ossie.py | 87 +++++++++++++++++-- 6 files changed, 141 insertions(+), 26 deletions(-) diff --git a/converters/dbt/README.md b/converters/dbt/README.md index 06385b847..cbb3f0993 100644 --- a/converters/dbt/README.md +++ b/converters/dbt/README.md @@ -124,6 +124,7 @@ manifest_json = result.output.model_dump_json(by_alias=True, exclude_none=True, | `PRIVATE_METRIC_DROPPED` | Ossie has no visibility modifiers | | `NATURAL_ENTITY_DROPPED` | Ossie has no natural-key entity type | | `CUMULATIVE_SEMANTICS_LOSS` | Window/grain semantics cannot be expressed in an Ossie expression string; the base aggregation is preserved | +| `AMBIGUOUS_REFERENCE_METRIC_DROPPED` | An input metric is listed more than once under one reference with differing filters, so the expression reference is ambiguous; give each occurrence a distinct alias | **Ossie → MSI** reconstructs a best-effort MSI manifest from Ossie's simpler schema. Nothing is dropped for supported inputs, but Ossie carries less structural information than MSI, so the converter makes the following choices: diff --git a/converters/dbt/src/ossie_dbt/cli.py b/converters/dbt/src/ossie_dbt/cli.py index a4fce8d24..14ba22cb6 100644 --- a/converters/dbt/src/ossie_dbt/cli.py +++ b/converters/dbt/src/ossie_dbt/cli.py @@ -40,12 +40,17 @@ ConverterIssueType.PRIVATE_METRIC_DROPPED: "Ossie has no visibility modifiers", ConverterIssueType.NATURAL_ENTITY_DROPPED: "Ossie has no natural-key entity type", ConverterIssueType.CUMULATIVE_SEMANTICS_LOSS: "Ossie expressions cannot represent window or grain semantics; the base aggregation was preserved", + ConverterIssueType.AMBIGUOUS_REFERENCE_METRIC_DROPPED: ( + "an input metric is listed more than once under one reference with differing filters, " + "so the expression reference is ambiguous; give each occurrence a distinct alias" + ), } _DROPPED_ISSUE_TYPES = { ConverterIssueType.CONVERSION_METRIC_DROPPED, ConverterIssueType.PRIVATE_METRIC_DROPPED, ConverterIssueType.NATURAL_ENTITY_DROPPED, + ConverterIssueType.AMBIGUOUS_REFERENCE_METRIC_DROPPED, } diff --git a/converters/dbt/src/ossie_dbt/converter_issues.py b/converters/dbt/src/ossie_dbt/converter_issues.py index e1606fa16..3a890f2e9 100644 --- a/converters/dbt/src/ossie_dbt/converter_issues.py +++ b/converters/dbt/src/ossie_dbt/converter_issues.py @@ -27,6 +27,7 @@ class ConverterIssueType(Enum): PRIVATE_METRIC_DROPPED = "PRIVATE_METRIC_DROPPED" NATURAL_ENTITY_DROPPED = "NATURAL_ENTITY_DROPPED" CUMULATIVE_SEMANTICS_LOSS = "CUMULATIVE_SEMANTICS_LOSS" + AMBIGUOUS_REFERENCE_METRIC_DROPPED = "AMBIGUOUS_REFERENCE_METRIC_DROPPED" @dataclass(frozen=True) diff --git a/converters/dbt/src/ossie_dbt/filter_utils.py b/converters/dbt/src/ossie_dbt/filter_utils.py index 2ef178a37..4b1ac8341 100644 --- a/converters/dbt/src/ossie_dbt/filter_utils.py +++ b/converters/dbt/src/ossie_dbt/filter_utils.py @@ -154,8 +154,14 @@ def _collect_filter_sql(*filters: Optional[WhereFilterIntersection]) -> Optional def _merge_filter_sqls(*parts: Optional[str]) -> Optional[str]: - """Join non-None SQL filter strings with AND, wrapping each in parens when multiple.""" - active = [p for p in parts if p] + """Join non-None SQL filter strings with AND, wrapping each in parens when multiple. + + Repeated fragments are dropped. Filters are conjoined and AND is idempotent, so an + input metric that restates a filter an enclosing metric already applies contributes + nothing but ``(X) AND (X)`` noise — and would otherwise make two logically identical + resolutions of the same reference compare unequal. + """ + active = list(dict.fromkeys(p for p in parts if p)) if not active: return None if len(active) == 1: diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index 1fb828683..0735f0d8d 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -72,6 +72,14 @@ class _RelationshipDirection: to_col: str +class AmbiguousDerivedReferenceError(Exception): + """A DERIVED metric's expression reference does not resolve to a single expression. + + Raised by the expression resolver and handled in :meth:`MSIToOssieConverter.convert`, + which drops the metric and records a ConverterIssue rather than failing the run. + """ + + class MSIToOssieConverter: """Converts an MSI SemanticManifest into an Ossie Document.""" @@ -109,7 +117,18 @@ def convert( issues.append( ConverterIssue(issue_type=ConverterIssueType.CUMULATIVE_SEMANTICS_LOSS, element_name=metric.name) ) - expr = self._resolve_metric_expression(metric, metric_index, expression_cache) + try: + expr = self._resolve_metric_expression(metric, metric_index, expression_cache) + except AmbiguousDerivedReferenceError: + # Every other unsupported shape drops one metric and records an issue; + # an ambiguous reference is no reason to fail the whole conversion. + issues.append( + ConverterIssue( + issue_type=ConverterIssueType.AMBIGUOUS_REFERENCE_METRIC_DROPPED, + element_name=metric.name, + ) + ) + continue ossie_metrics.append( OssieMetric( name=metric.name, @@ -340,16 +359,18 @@ def _resolve_derived( 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 + Listing the same input metric twice under one reference raises + :class:`AmbiguousDerivedReferenceError` when the expression uses that reference + and 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. + alias. Occurrences that resolve identically are redundant rather than ambiguous, + and a duplicated reference the expression never substitutes cannot affect the + result; both are accepted. """ expr = metric.type_params.expr or "" - replacements: Dict[str, str] = {} + resolutions: Dict[str, List[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}'") @@ -357,25 +378,31 @@ 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 + distinct = resolutions.setdefault(ref, []) + if resolved not in distinct: + distinct.append(resolved) - if not replacements: + if not resolutions: return expr # The `\b` anchors already stop a short reference from matching inside a # 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=lambda ref: (-len(ref), ref))) + r")\b" + r"\b(" + "|".join(re.escape(ref) for ref in sorted(resolutions, key=lambda ref: (-len(ref), ref))) + r")\b" ) - return pattern.sub(lambda match: replacements[match.group(0)], expr) + + # Only a reference the expression actually uses can be ambiguous; one that is + # listed but never substituted has no bearing on the result. + for ref in sorted(set(pattern.findall(expr))): + if len(resolutions[ref]) > 1: + raise AmbiguousDerivedReferenceError( + "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}" + ) + + return pattern.sub(lambda match: resolutions[match.group(0)][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 8b761e8f2..b55e5281f 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -760,12 +760,13 @@ 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. + def test_derived_metric_drops_a_used_reference_listed_twice_with_differing_filters(self) -> None: + """An input metric listed twice under one used 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. + only compares entries that set an alias — so the converter has to handle it. It + drops the metric and records an issue, as it does for every other unsupported + shape, rather than failing the whole conversion. """ sm = semantic_model_with_guaranteed_meta( name="orders", @@ -787,8 +788,76 @@ def test_derived_metric_rejects_a_reference_listed_twice_with_differing_filters( metadata=default_meta(), config=None, ) - with pytest.raises(ValueError, match="listed more than once"): - MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[revenue_m, both])) + result = MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[revenue_m, both])) + + assert [m.name for m in _ossie_metrics(result.output)] == ["revenue"] + assert len(result.issues) == 1 + assert result.issues[0].issue_type == ConverterIssueType.AMBIGUOUS_REFERENCE_METRIC_DROPPED + assert result.issues[0].element_name == "both" + + def test_derived_metric_accepts_a_duplicate_reference_the_expression_never_uses(self) -> None: + """A reference that is never substituted cannot be ambiguous, however it resolves.""" + sm = semantic_model_with_guaranteed_meta( + name="orders", + measures=[_measure("revenue", agg=AggregationType.SUM, expr="amount")], + ) + revenue_m = _simple_metric("revenue", "revenue") + constant = PydanticMetric( + name="constant", + description=None, + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="1 + 1", + 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, + ) + result = MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[revenue_m, constant])) + + assert next(m for m in _ossie_metrics(result.output) if m.name == "constant").expression.dialects[ + 0 + ].expression == "1 + 1" + assert result.issues == [] + + def test_derived_metric_accepts_a_duplicate_that_restates_the_enclosing_filter(self) -> None: + """Restating a filter the enclosing metric already applies is redundant, not ambiguous. + + AND is idempotent, so the bare occurrence and the one repeating the parent's own + filter describe the same set of rows and must not be treated as two resolutions. + """ + region_eu = "{{ Dimension('order__region') }} = 'EU'" + sm = semantic_model_with_guaranteed_meta( + name="orders", + measures=[_measure("revenue", agg=AggregationType.SUM, expr="amount")], + ) + revenue_m = _simple_metric("revenue", "revenue") + scaled = PydanticMetric( + name="scaled", + description=None, + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="revenue * 2", + metrics=[ + PydanticMetricInput(name="revenue"), + PydanticMetricInput(name="revenue", filter=_filter(region_eu)), + ], + ), + filter=_filter(region_eu), + metadata=default_meta(), + config=None, + ) + result = MSIToOssieConverter().convert(_manifest(semantic_models=[sm], metrics=[revenue_m, scaled])) + + scaled_ossie = next(m for m in _ossie_metrics(result.output) if m.name == "scaled") + assert scaled_ossie.expression.dialects[0].expression == ( + "SUM(CASE WHEN order__region = 'EU' THEN orders.amount END) * 2" + ) + assert result.issues == [] 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.""" @@ -819,6 +888,12 @@ def test_derived_metric_accepts_a_reference_listed_twice_resolving_identically(s 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_every_issue_type_has_a_cli_reason(self) -> None: + """The CLI looks each issue type up by key, so an unmapped one would crash the run.""" + from ossie_dbt.cli import _ISSUE_REASON + + assert set(_ISSUE_REASON) == set(ConverterIssueType) + def test_derived_metric_nested(self, snapshot: SnapshotAssertion) -> None: sm = semantic_model_with_guaranteed_meta( name="orders",