apache / apache/ossie

msi_to_osi silently drops offset_window / offset_to_grain on DERIVED metric inputs, producing constant-zero expressions

Open Beginner friendly
#302 3 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.