Skip to content

fix(dbt): unqualify compound aggregate arguments per column - #388

Open
barveP wants to merge 1 commit into
apache:mainfrom
barveP:fix/dbt-aggregate-argument-qualifier
Open

barveP wants to merge 1 commit into
apache:mainfrom
barveP:fix/dbt-aggregate-argument-qualifier

Conversation

@barveP

@barveP barveP commented Sep 12, 2026

Copy link
Copy Markdown

Summary

OssieToMSIConverter turned any aggregate argument that is not a single column into MetricFlow's expr by rendering it to text and slicing at the last .: SUM(orders.gross - orders.tax) became expr: tax (and SUM(orders.tax) after a round trip), SUM(COALESCE(orders.tax, 0)) became the unparseable tax, 0), and even the unqualified SUM(amount * 0.5) became 5. No ConverterIssue was emitted.

_col_name now strips the qualifier from each column reference on the sqlglot tree _extract_agg_info already holds (node.transform(_unqualify_column)) and renders the result, so SUM(orders.gross - orders.tax) yields expr: gross - tax, which is what MetricFlow expects inside the semantic model's subquery. The plain-column path, the _extract_agg_info return value, and _strip_qualifier (still used by _find_dataset_for_col for field lookups) are unchanged, so output only changes for arguments that were previously corrupted. Rendering already went through node.sql(), so no new normalisation is introduced. No new dependency.

Tests: one parametrized forward test covering arithmetic (SUM, MAX), function calls, casts, a decimal literal, COUNT(DISTINCT a || b) and PERCENTILE_CONT over a compound argument, plus one Ossie → MSI → Ossie round trip. All fail on main and pass with the change; the 100 existing tests and 5 snapshots are unchanged.

Out of scope, noted in the issue: the fallback path that re-wraps an unrecognised aggregate into SUM(SUM(...)) on export; _qualify_col on export re-qualifying only bare identifiers (its own comment defers compound and quoted expressions), which is why the round-trip test expects SUM(gross - tax) and a ratio with a compound numerator now exports as (SUM(gross - tax)) / (SUM(orders.gross)) (numerically identical within one semantic model; main exported the wrong (SUM(orders.tax)) / (SUM(orders.gross))); and the SUM_BOOLEAN branch, which still keeps the qualifier in its condition (separate issue).

Related Issues

Fixes #385

Testing

cd converters/dbt
uv run pytest

Result: 108 passed (100 existing + 8 new; 5 snapshots unchanged). The 8 new tests fail on main.

Also checked locally: the full suite under the declared floor sqlglot==20.0.0 (108 passed); every Ossie document in examples/ and converters/*/tests/ converts byte-identically on main and this branch; MetricFlow's SemanticManifestValidator accepts manifests containing every new expr shape with no errors or warnings; and a differential run of the old and new _col_name over 31,608 generated aggregate expressions differs only when the rendered argument contains a dot, with no change in which expressions are recognised.
CI matrix reproduced locally with uv on Python 3.11, 3.12, 3.13 and 3.14 against the locked dependencies (metricflow 0.211.0, sqlglot 30.12.0): 108 passed on each, and again with sqlglot pinned to the declared floor 20.0.0 and to the newest release 30.18.0. Rendering the converted metrics through MetricFlow 0.211 on DuckDB returns the same numbers as the original Ossie expressions for every affected shape, where main returns the numbers of the sliced argument.

Checklist

Specification

  • Spec changes are included in core-spec/ and follow the existing structure
  • Spec changes have been discussed on the mailing list or in a linked issue
  • Breaking changes to the spec are clearly called out in the summary

Ontology

  • Ontology changes in ontology/ are consistent with spec changes
  • New or modified terms are defined and documented

Converters

  • Converter logic in converters/ is updated to reflect spec or ontology changes
  • New converters include tests under the converter's test directory

Validation

  • Validation rules in validation/ are updated if the spec changed
  • New validation cases are covered by tests

Documentation

  • docs/ is updated to reflect any user-facing changes
  • New features or behaviors are documented with examples where appropriate
  • CONTRIBUTING.md is updated if the contribution process changed

Examples

  • examples/ are added or updated for any new spec constructs or converter support

Tests

  • All existing tests pass (pytest / CI green)
  • New functionality is covered by tests

Compliance

  • ASF license headers are present on all new source files
  • No third-party dependencies are added without PMC/IPMC approval

`_col_name` rendered a non-column aggregate argument to text and kept
only what followed the last dot, so `SUM(orders.gross - orders.tax)`
became `expr: tax` and `SUM(COALESCE(orders.tax, 0))` became `tax, 0)`.
Strip the dataset qualifier from each column reference on the parsed
tree instead, keeping the surrounding expression intact.

Fixes apache#385
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 slices a compound aggregate argument at its last dot, so SUM(orders.gross - orders.tax) becomes SUM(orders.tax)

1 participant