fix(dbt): substitute DERIVED metric references in a single pass - #353
Conversation
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" |
| 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.
|
Hi @jbonofre does the latest changes look good? Let me know if it requires further changes or investigation :-) |
|
@ayushtkn I'm doing a new pass. Thanks! |
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
Thanks for the fix, I think we are very close!
The single-pass substitution change itself is solid and well covered by the new tests.
Before merging, I propose:
- to fix the two false positives at
msi_to_ossie.py:367: a duplicated input metric with no per-input filter vs one that restates the parent own filter currently render to different text ("(X)"vs"(X) AND (X)") and trips the check even though they are logically identical. And a duplicated-but-unused reference (never appearing inexpr) should not be checked at all. - to fail soft instead of hard-raising: catch this case the same way the converter handles every other unsupported shape (
CONVERSION_METRIC_DROPPED, etc), append aConverterIssueand drop that one metric, rather than lettingValueErrorpropagate uncaught throughcli.pyand kill the whole run.
I would be happy to pair on this or take a stab at the fix myself if that's faster.
Once those two are addressed I'm good to approve and merge.
_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.
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.
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.
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.
6952a43 to
f176164
Compare
|
Thanx @jbonofre for helping with the review. I have quickly tried and pushed a commit to address the review comment. I am good with if you want to collaborate or wanna take it over as well. I can help with testing and reviewing the changes :-) |
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