fix(dbt): substitute DERIVED metric references in a single pass - #353
fix(dbt): substitute DERIVED metric references in a single pass#353ayushtkn wants to merge 3 commits into
Conversation
_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.
There was a problem hiding this comment.
🟢 Approval recommended
The refactor directly addresses the described substitution correctness issues and is covered by targeted regression tests, with only minor non-blocking maintainability feedback.
Pull request overview
This PR fixes DERIVED metric reference inlining in the dbt MSI→Ossie converter by performing all substitutions in a single regex pass with a callback replacement, preventing both accidental re-expansion of already-inlined text and re.sub replacement-escape corruption (e.g., backslashes from filters).
Changes:
- Refactors
MSIToOssieConverter._resolve_derivedto collect reference→resolved-expression mappings and substitute them in onere.subpass via a compiled alternation pattern and callback. - Adds regression tests covering (1) no re-expansion when one metric name appears inside another metric’s resolved SQL and (2) preservation of backslashes originating from filters.
File summaries
| File | Description |
|---|---|
| converters/dbt/src/ossie_dbt/msi_to_ossie.py | Changes DERIVED metric substitution to a single-pass alternation+callback approach to avoid re-expansion and replacement-escape issues. |
| converters/dbt/tests/test_msi_to_ossie.py | Adds tests validating single-pass substitution behavior and correct backslash preservation from filtered metrics. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # 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" |
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.
| resolved = f"({resolved})" | ||
| expr = re.sub(rf"\b{re.escape(ref)}\b", resolved, expr) | ||
| return expr | ||
| replacements[ref] = resolved |
There was a problem hiding this comment.
I think that collecting into replacements[ref] = resolved changes the collision behavior from "first entry wins" to "last entry wins". I'm not sure it's addressed anywhere.
if a derived metric lists the same input metric twice with different per-input filters and no alias, this dict collapses to one entry and both occurrences get F2's SQL. This input shape isn't rejected upstream: MetricFlow's DerivedMetricRule._validate_alias_collision only compares entries that have an alias set, so two unaliased duplicates sail through validation.
Worth either erroring on a duplicate unaliased ref here, or confirming this silent overwrite is intentional (and so it should be documented).
There was a problem hiding this comment.
Thanx @jbonofre for the review. Good catch — silent last-wins wasn’t intentional. MetricFlow doesn’t reject this shape (DerivedMetricRule._validate_alias_collision only compares aliased entries), and with a single token in expr neither resolution is more correct. Raised a ValueError when the same reference resolves differently, and pointed at distinct aliases as the fix. Identical duplicates stay accepted since they’re redundant, not ambiguous. Covered in 6952a43.
| 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: |
There was a problem hiding this comment.
I suggest to add a test for a DERIVED metric whose type_params.metrics contains two entries resolving to the same reference (same name, same alias) with different filters?
That's the case where the dict-based replacements collapses to one entry and silently picks whichever occurrence was declared last (there's no coverage for that ordering behavior right now).
There was a problem hiding this comment.
Added two tests in 6952a43: one that rejects the same reference listed twice with differing filters, and one that accepts a redundant identical duplicate (revenue + revenue). Went with reject rather than asserting last-wins ordering.
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.
Summary
MSIToOssieConverter._resolve_derivedinlined each input metric of a DERIVED metricwith its own
re.subcall, run over the string produced by the previous iteration.Because every pass re-scanned text that earlier passes had inserted, and because the
resolved expression was passed as
re.sub's replacement template, the emitted Ossieexpression could be silently corrupted in two ways.
1. A later reference matched text an earlier one inserted. MetricFlow metrics are
commonly named after the column they aggregate, so this is easy to hit. With measures
gross = SUM(net)andnet = SUM(net_amount)and a DERIVED metricexpr = "gross - net":2. Backslashes in the resolved SQL were read as replacement escapes. A backslash
surviving from a metric filter was reinterpreted on inlining. With a SIMPLE metric
filtered on
{{ Dimension('order__path') }} LIKE 'a\b', inlined intoexpr = "revenue * 2":The
\bbecame a literal backspace character. Other sequences (e.g.\d) raisedre.error: bad escapeand aborted the conversion instead.The change. Collect the
{reference: resolved expression}pairs first, thensubstitute them in a single pass using an alternation pattern with a callback
replacement. A callback is not template-expanded, so one change fixes both problems.
This is the same approach already used in the Databricks converter
(
metric_view_to_ossie.py,re.sub(r"\bsource\.", lambda _m: ...)).Related Issues
NA
Checklist
Specification
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changesConverters
converters/is updated to reflect spec or ontology changesValidation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changedExamples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green)Compliance