fix(dbt): substitute DERIVED metric references in a single pass - #353
Open
ayushtkn wants to merge 2 commits into
Open
fix(dbt): substitute DERIVED metric references in a single pass#353ayushtkn wants to merge 2 commits into
ayushtkn wants to merge 2 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.
Comment on lines
+364
to
+367
| # 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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