msi_to_osi silently drops offset_window / offset_to_grain on DERIVED metric inputs, producing constant-zero expressions
- Dominant language
- Python
- Stars
- 2.1k
- Forks
- 267
- Avg merge
- 4d 20h
- Merged PRs (30d)
- 24
Description
`MSIToOSIConverter._resolve_derived` ignores `offset_window` and `offset_to_grain` on DERIVED metric inputs. The offset is dropped silently — no `ConverterIssue` is emitted — so a period-over-period metric converts into an expression that is algebraically constant, and looks like a successful conversion.
**Root cause**
`converters/dbt/src/ossie_dbt/msi_to_osi.py:337-351`, `_resolve_derived`:
```python
expr = metric.type_params.expr or ""
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}'")
input_filter = _merge_filter_sqls(filter_sql, _collect_filter_sql(input_metric.filter))
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)
```
`input_metric.alias`, `.name` and `.filter` are all read; `input_metric.offset_window` and `input_metric.offset_to_grain` are not read anywhere in the file. Two inputs referencing the same metric — one current, one offset — therefore resolve to byte-identical SQL, and the difference between them collapses to zero.
The year-over-year shape this breaks is the canonical use of `offset_window`, and the one in the MetricFlow docs.
**Repro** (HEAD `88e0011148283302c9a04cd0287e00e0b9d87354`), using the repo's own test helpers:
```python
import sys
sys.path.insert(0, "src"); sys.path.insert(0, "."); sys.path.insert(0, "../../python/src")
from ossie_dbt.msi_to_osi import MSIToOSIConverter
from tests.helpers import _manifest, _simple_metric
from metricflow_semantic_interfaces.implementations.elements.measure import PydanticMeasure
from metricflow_semantic_interfaces.implementations.metric import (
PydanticMetric, PydanticMetricInput, PydanticMetricTimeWindow, PydanticMetricTypeParams,
)
from metricflow_semantic_interfaces.test_utils import default_meta, semantic_model_with_guaranteed_meta
from metricflow_semantic_interfaces.type_enums import AggregationType, MetricType
sm = semantic_model_with_guaranteed_meta(
name="bookings",
measures=[PydanticMeasure(name="booking_count", agg=AggregationType.SUM, expr="1")],
)
base = _simple_metric("booking_count", "booking_count")
yoy = PydanticMetric(
name="bookings_yoy_growth",
description=None,
type=MetricType.DERIVED,
type_params=PydanticMetricTypeParams(
expr="(this_year - last_year) / NULLIF(last_year, 0) * 100",
metrics=[
PydanticMetricInput(name="booking_count", alias="this_year"),
PydanticMetricInput(name="booking_count", alias="last_year",
offset_window=PydanticMetricTimeWindow(count=1, granularity="year")),
],
),
filter=None, metadata=default_meta(), config=None,
)
res = MSIToOSIConverter().convert(_manifest(semantic_models=[sm], metrics=[base, yoy]))
out = [m for m in res.output.semantic_model[0].metrics if m.name == "bookings_yoy_growth"][0]
print("EXPRESSION:", out.expression.dialects[0].expression)
print("ISSUES:", res.issues)
```
Run from `converters/dbt/`.
Observed:
```
EXPRESSION: (SUM(1) - SUM(1)) / NULLIF(SUM(1), 0) * 100
ISSUES: []
```
The expression is algebraically `0` for every input row, and nothing signals that anything was lost.
Expected: either the offset represented in the output, or — since Ossie expressions have no window semantics to represent it with — a `ConverterIssue` reporting the loss, as `CUMULATIVE` metrics already do.
**Why this is worse than the cumulative case**
`CUMULATIVE` metrics already emit `ConverterIssueType.CUMULATIVE_SEMANTICS_LOSS` (`msi_to_osi.py:95-98`), so a consumer can see the semantics didn't survive and decide what to do. `offset_window` has no equivalent, so the failure mode is different in kind: the cumulative metric comes out *approximate*, the offset metric comes out *wrong*, and both look identical to anything downstream.
Concretely, converting through `converters/snowflake` and creating the semantic view in Snowflake produces a metric that runs successfully and returns `0` for every period — indistinguishable from genuinely flat year-on-year growth. It is the only failure I hit in an end-to-end dbt → Ossie → Snowflake conversion that produced a plausible wrong number rather than an error or an obvious gap.
**Suggested fix**
At minimum, emit a `ConverterIssue` when an input metric carries `offset_window` or `offset_to_grain`, matching the existing `CUMULATIVE_SEMANTICS_LOSS` pattern — e.g. `OFFSET_SEMANTICS_LOSS`. That turns a silent wrong answer into a visible gap, and lets consumers gate on it.
Beyond that it's a spec question rather than a converter one: representing a time-offset input needs something in the Ossie spec to carry it, which I don't think exists today (related to #290's shape, though not the same issue). Happy to raise that separately if it's useful.
`tests/test_msi_to_osi.py` has DERIVED coverage but no case where an input carries `offset_window`, which is why this isn't caught.
Happy to send a PR for the `ConverterIssue` part.
Contributor guide
Research direction
Start in converters/dbt/src/ossie_dbt/msi_to_osi.py, especially _resolve_derived and the existing CUMULATIVE_SEMANTICS_LOSS handling. Reproduce the year-over-year case from converters/dbt/ and add coverage in tests/test_msi_to_osi.py for an input with offset_window or offset_to_grain. Done means the lost offset is reported as a ConverterIssue and the test verifies the issue.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 82/100