Skip to content

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

Description

@barveP

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:

- 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:

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:

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:

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.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions