fix(dbt): emit expr '1' for COUNT(*) metrics

[LukasSchwarzlmueller]

Summary

ossie-to-msi turned COUNT(*) into a count metric with type_params.expr: '*'. MetricFlow renders every count metric as SUM(CASE WHEN <expr> IS NOT NULL THEN 1 ELSE 0 END), so the generated query contained CASE WHEN * IS NOT NULL, which is not valid SQL (DuckDB: STAR expression is only allowed as the root element of an expression).

_extract_agg_info now returns the constant "1" for COUNT(*) and COUNT(dataset.*). 1 is never null, so every row is counted, which is COUNT(*) semantics.

Checked end to end on DuckDB (dbt run, then mf query; the manifest was also patched for agg_time_dimension, the owning semantic model and the time spine, which are separate converter gaps this PR does not touch): before, mf query --metrics order_count fails with the binder error above; after, it returns 4 for four orders, and the per-segment and per-month counts, revenue and average match a plain SQL query on the same tables.

Changes:

• expression_utils.py: new _is_star helper and a COUNT(*) branch ahead of the generic COUNT(col) branch; docstring updated.
• test_ossie_to_msi.py: tests for COUNT(*), count( * ) and COUNT(orders.*), for the qualified form landing on the right semantic model, and for COUNT(*) inside a ratio metric.
• README.md: one line under the Ossie → MSI conversion choices.

The new tests fail without the fix and pass with it (5 cases fail on unchanged main). The full converters/dbt suite passes on Python 3.11, 3.12, 3.13 and 3.14 (111 tests).

Not changed: on the way back, msi_to_ossie gives SUM(1) for this metric (MetricFlow's loader has already turned the count into a sum), not COUNT(*). That is valid SQL with the same result, whereas today's output was invalid. Existing snapshots are unchanged.

Related Issues

None yet.

Checklist

Converters

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

Documentation

• converters/dbt/README.md is updated to reflect the user-facing change

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 new files added)
• No third-party dependencies are added without PMC/IPMC approval

Specification, Ontology, Validation and Examples: not applicable.

[jbonofre]

I believe this hands "1" back as a column name, and the caller resolves it like one.

_convert_metric passes bare_col to _find_dataset_for_col, which for "1" finds no qualifier, matches no field, and falls through to datasets[0].name.
Two ways that goes wrong:

1. Doc with datasets: [customers, orders] and order_count = COUNT(*) produces metric_aggregation_params.semantic_model == "customers". Before this PR, the emitted * made MetricFlow generate CASE WHEN * IS NOT NULL and the query failed to compile. Now it compiles and returns the customers row count under the name order_count. In a "ratio", it's worse: (SUM(amount)) / (COUNT(*)) over those same two datasets gives numerator orders and denominator customers, so MetricFlow joins two unrelated models and the average is quietly wrong.
2. If any dataset has a field whose expression is the constant 1, the field expression scan matches it and every COUNT(*) in the document binds to that dataset.

1 is a constant, not a column, so it should not go through the column -> dataset resolver at all.

I suggest resolving the star case from the qualifier only, and refusing when it's ambiguous, which is that the Microsoft converter already does:

# _sql_to_dax.py:208-210
table = resolve_table()
if table is None:
    return None, "'COUNT(*)' needs exactly one dataset to count rows of"

Trading a compile error for a plausible-looking wrong number is the part I would like to avoid here. Wdyt?

[LukasSchwarzlmueller]

You're right, thanks for catching this. Fixed in 664e3a1.

One thing i was unsure of: it raises instead of skipping the metric, because the converter has no per-metric skip. I can switch to skip-with-a-warning if you prefer.

[jbonofre]

The guard is placed after the DISTINCT branch and ignores extra arguments, so two star forms slip past it.

For instance:

• COUNT(DISTINCT *) still emits *. sqlglot parses it, and it hits tree.this is Distinct, cols == [Star], len(cols) == 1, so it returns (COUNT DISTINCT, '*'). MetricFlow renders COUNT(DISTINCT *) and DuckDB fails with STAR expression is only allowed as the root element of an expression, the exact error this PR set out to remove. The same applies to COUNT(DISTINCT orders.*).
• COUNT(t.*, x) is silently accepted. That parses to exp.Count(this=column(t.*), expressions=[x]). _is_star(tree.this) is True, so the branch returns a plain row count and drops the second argument without a word.

Both fall out of folding the three exp.Count branches into one block that unwraps DISTINCT and reject multi-arg first. DAX converter is already using it:

if isinstance(tree, exp.Count):
    if tree.args.get("expressions"):
        return None                      # COUNT(t.*, x) → raw-expression fallback
    argument, distinct = tree.this, False
    if isinstance(argument, exp.Distinct):
        operands = argument.args.get("expressions") or []
        if len(operands) != 1:
            return None
        argument, distinct = operands[0], True
    if _is_star(argument):
        return None if distinct else (AggregationType.COUNT, "1", None, False)
    ...

That also drops the trip isinstance(tree, exp.Count) test currently spread in different places.

[LukasSchwarzlmueller]

Done as you suggested, thanks. Fixed in 664e3a1.

[jbonofre]

This accepts any multi-part qualified star, and the qualifier is then used verbatim as a semantic model name.

COUNT(db.orders, *) satisfies isinstance(node.this, exp.Star), so we take the star branch and _get_dataset_qualifier joins part[:-1] into "db.orders". The metric comes out with metric_aggregation_params.semantic_model == "db.orders" while the manifest's model is named orders, and MetricFlow can't resolve the metric owning model. Unlike the plain column path there is no bare column fallback to recover from it, because the star branch discards the node entirely.

I suggest to either restrict _is_star to a single part qualifier, or have the star path resolve the qualifier last segment against the actual dataset names and refuse when there is no match.

[LukasSchwarzlmueller]

Went with your second option. Fixed in 664e3a1.

• Restricting _is_star would send db.orders.* down the column path and give * again.
• Your literal COUNT(db.orders, *) example is caught by the extra-argument check from the other thread.

[jbonofre]

Also, I believe COUNT(dataset.*) doesn't survive a round trip.

With datasets: [customers, orders] and COUNT(orders.*):

• ossie -> msi gives expr "1", semantic_model "orders", agg count: it's correct and that's what test_qualified_count_star_uses_dataset_qualifier asserts.
• on the trip msi -> ossie, _qualify_col leaves "1" alone, it matches neither the bare identifier regex nor COUNT_CONVERSION_RE: so the Ossie expression comes back as SUM(1), with the orders binding gone.
• on the trip ossie -> msi again, it re-converts that to expr "1", semantic_model "customers", agg sum.

The PR description "SUM(1) is valid SQL with the name result" holds only for a single dataset document.
With more than one dataset the round trip silently moves the metric to a different table. _qualify_col needs a case for the constant sentinel (re-emit COUNT(<semantic_model>.*) when agg == count and expr == "1"), and a round trip test over a two dataset doc would pin it.

[LukasSchwarzlmueller]

Reproduced. Fixed in 664e3a1, with one difference from your suggestion:

• _qualify_col can't check agg == count, because the count is already a SUM by then. msi_to_ossie records the row-count metrics before the transform instead.
• Filtered counts are still not covered, since there's no dataset.* form with a filter.

[jbonofre]

@LukasSchwarzlmueller thanks for the updates! I will do a new pass. Much appreciated 😄