Skip to content

fix(dbt): warn on non-portable metric semantics - #318

Open
fanruan-tuco wants to merge 2 commits into
apache:mainfrom
fanruan-tuco:fix/317-time-context-semantics
Open

fanruan-tuco wants to merge 2 commits into
apache:mainfrom
fanruan-tuco:fix/317-time-context-semantics

Conversation

@fanruan-tuco

Copy link
Copy Markdown
Contributor

Summary

MetricFlow time-context semantics such as cumulative metrics, windows, time spine behavior, and offset-period inputs cannot be represented faithfully in Ossie. This change emits NON_PORTABLE_METRIC_SEMANTICS instead of silently producing a lossy conversion.

It also checks derived and ratio dependencies recursively, including cases such as:

- name: month_over_month_growth
  type: derived
  type_params:
    expr: (cur - pre) / pre
    metrics:
      - name: revenue
        alias: cur
      - name: revenue
        alias: pre
        offset_window:
          count: 1
          granularity: month

Fixes #317

Validation

uv run pytest -q in converters/dbt: 103 passed.

@jbonofre

Copy link
Copy Markdown
Member

@fanruan-tuco can you please rebase and resolve conflicts? Thanks!

issues.append(
ConverterIssue(issue_type=ConverterIssueType.CUMULATIVE_SEMANTICS_LOSS, element_name=metric.name)
)
elif _has_nonportable_semantics(metric, metric_index):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new branch records a NON_PORTABLE_METRIC_SEMANTICS issue but doesn't change control flow: the next line (expr = self._resolve_metric_expression(...), unchanged) still runs and emits the lossy expression regardless.

I verified against the example in the PR (month_over_month_growth = (cur - pre) / pre): the emitted expression is (SUM(orders.amount) - SUM(orders.amount)) / SUM(orders.amount), always 0. The output document carries no marker of the problem, only a side-channel entry in result.issues: a caller that doesn't check issues ships an always-zero metric silently.

Should this branch continue/skip emitting the metric (like CONVERSION_METRIC_DROPPED above) instead of failing through?

visiting: Optional[set[str]] = None,
) -> bool:
"""Return whether a metric uses semantics Ossie cannot represent."""
if metric.type is MetricType.CUMULATIVE:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only special-cases CUMULATIVE. A DERIVED/RATIO metric that depends on a CONVERSION-type metric return False here (not portable-safe, just unrecognized), so convert() proceeds into _unresolve_metric_expression, which raises an unhandled RuntimeError ("Unexpected CONVERSION metric in expression resolver") and aborts conversion of the entire manifest.

Maybe worth adding a MetricType.CONVERSION case here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dbt converter loses derived metric time-context semantics in MetricFlow to Ossie conversion

2 participants