apache / apache/ossie

dbt converter slices a compound aggregate argument at its last dot, so `SUM(orders.gross - orders.tax)` becomes `SUM(orders.tax)`

Open
#385 0 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

When an Ossie metric aggregates anything more than a single column, the dbt converter turns the argument into MetricFlow's `expr` by rendering it to text and keeping only what follows the last `.`. The metric is emitted with no `ConverterIssue`, converts cleanly back to Ossie, and computes a different number.

Smallest example. `orders` has fields `gross` and `tax`; this is a valid Ossie metric:

```yaml
- name: net_sales
expression:
dialects:
- dialect: ANSI_SQL
expression: SUM(orders.gross - orders.tax)
```

`OssieToMSIConverter` produces a SIMPLE metric with `agg: sum` and `expr: tax` on semantic model `orders`. Converting that manifest back with `MSIToOssieConverter` gives `SUM(orders.tax)`. Over two rows with `gross`/`tax` of 10/2 and 20/3, `SUM(gross - tax)` is 8 + 17 = 25, but the round-tripped metric computes `SUM(tax)` = 5.

The same slicing hits every recognised aggregate (`SUM`, `AVG`, `MIN`, `MAX`, `COUNT`, `COUNT(DISTINCT …)`, `PERCENTILE_*`) whenever the rendered argument contains a dot, including an unqualified argument with a decimal literal:

| Ossie expression | MSI `expr` | After MSI → Ossie | Result |
|---|---|---|---|
| `SUM(orders.gross - orders.tax)` | `tax` | `SUM(orders.tax)` | 25 → 5 |
| `SUM(orders.amount * orders.tax)` | `tax` | `SUM(orders.tax)` | 32 → 5 |
| `MAX(orders.gross - orders.tax)` | `tax` | `MAX(orders.tax)` | 17 → 3 |
| `COUNT(DISTINCT orders.status \|\| orders.region)` | `region` | `COUNT(DISTINCT orders.region)` | 2 → 1 |
| `SUM(amount * 0.5)` | `5` | `SUM(5)` | 5.5 → 10 |
| `SUM(COALESCE(orders.tax, 0))` | `tax, 0)` | `SUM(tax, 0))` | not parseable |
| `SUM(CAST(orders.tax AS DOUBLE))` | `tax AS DOUBLE)` | `SUM(tax AS DOUBLE))` | not parseable |

The result column runs both expressions over this two-row `orders` table (`SELECT … FROM (VALUES (10, 2, 1, 'paid', 'US'), (20, 3, 10, 'unpaid', 'US')) t(gross, tax, amount, status, region)` in DuckDB):

| gross | tax | amount | status | region |
|---|---|---|---|---|
| 10 | 2 | 1 | paid | US |
| 20 | 3 | 10 | unpaid | US |

`SUM(orders.amount)` and `SUM(gross - tax)` (no dot in the rendered argument) are unaffected. Reproduced on `main` at 28365cd.

## Reproduction

From `converters/dbt`, 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.ossie_to_msi import OssieToMSIConverter
from ossie_dbt.msi_to_ossie import MSIToOssieConverter
from tests.helpers import _ossie_dataset, _ossie_doc, _ossie_field, _ossie_metric

doc = _ossie_doc(
datasets=[_ossie_dataset("orders", fields=[_ossie_field("gross"), _ossie_field("tax")])],
metrics=[_ossie_metric("net_sales", "SUM(orders.gross - orders.tax)")],
)
msi = OssieToMSIConverter().convert(doc)
metric = msi.output.metrics[0]
print(metric.type_params.expr, metric.type_params.metric_aggregation_params.semantic_model, msi.issues)
back = MSIToOssieConverter().convert(msi.output)
print(back.output.semantic_model[0].metrics[0].expression.dialects[0].expression, back.issues)
```

Observed:

```text
tax orders []
SUM(orders.tax) []
```

Expected: `expr` is `gross - tax` and the round trip gives back an aggregate over `gross - tax`.

## Root cause

`converters/dbt/src/ossie_dbt/expression_utils.py`:

```python
def _strip_qualifier(col: str) -> str:
return col.rsplit(".", 1)[-1] if "." in col else col

def _col_name(node: exp.Expression) -> str:
if isinstance(node, exp.Column):
return node.name
rendered = node.sql()
return _strip_qualifier(rendered)
```

`_extract_agg_info` already has the parsed sqlglot tree and calls `_col_name` on the aggregate's argument node. For a plain column it returns the bare name, which is right: MetricFlow evaluates `expr` inside the semantic model's own subquery (`metricflow/dataset/convert_semantic_model.py`, `_make_element_sql_expr`), so the reference must be unqualified. For any other node it falls back to rendering the whole argument to a string and applying `rsplit(".", 1)`, which was written for `dataset.column` and does not know about operators, function calls, or decimal literals.

This is the same idiom that #265 / #292 removed from `_find_dataset_for_col` (semantic-model attribution now reads the qualifier from the parsed expression, which is why `semantic_model` is correct above while `expr` is not).

## Proposed fix

Strip the qualifier per column reference on the AST the function already holds (`node.transform(...)` rebuilding each `exp.Column` without its table part), instead of on the rendered text. `SUM(orders.gross - orders.tax)` then yields `expr: gross - tax`, `SUM(COALESCE(orders.tax, 0))` yields `COALESCE(tax, 0)`, and quoted identifiers inside a compound argument keep their quotes (the single-column path is unchanged). Plain-column behaviour, the tuple returned by `_extract_agg_info`, and `_strip_qualifier` (still used by `_find_dataset_for_col` for field lookups) are unchanged, so only arguments that were previously corrupted produce different output. No new dependency: sqlglot is already how this module reads expressions.

I have a patch with regression tests for the shapes above and can open the PR.

Contributor guide

Open the contributing guide

Research direction

Start in converters/dbt/src/ossie_dbt/expression_utils.py, especially _extract_agg_info and _col_name, and reproduce the issue with the provided tests.helpers setup. Check aggregate arguments with operators, function calls, qualified columns, and decimal literals; done means compound expressions retain their structure and the MSI-to-Ossie round trip returns the equivalent aggregate without ConverterIssue values.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.