fix(dbt): stop emitting COUNT(<dataset>.*), SUM(1) is the row count by LukasSchwarzlmueller · Pull Request #495 · apache/ossie

LukasSchwarzlmueller · 2026-10-01T22:04:52Z

Summary

Follow-up to #432. Post-merge review on that PR (thanks @jbonofre, and @kayemkim for reviewing this one) found more problems with how row counts are handled between Ossie and MSI.

1. The dataset-preservation mechanism never worked, and wasn't safe anyway.
MSIToOssieConverter tracked which metrics were row counts so it could emit COUNT(<dataset>.*) instead of SUM(1). On the real ossie-dbt msi-to-ossie path it never fired: the manifest loader already runs ConvertCountMetricToSumRule, so agg was SUM, not COUNT, by the time the pre-scan ran. It also missed legacy measure-based counts. And COUNT(<dataset>.*) isn't in the Ossie spec, several engines reject it or read it differently, and no sibling converter treats it as a row count.

Fix: delete the mechanism. SUM(1) already renders through the generic path. It matches COUNT(*) on any non-empty input; over zero rows it is NULL instead of 0, same as MetricFlow's own COUNT to SUM rewrite.

2. The lost semantic model is now recorded, and refused on the way back.
SUM(1) has no column to say which semantic model it counts.

MSI → Ossie: when a metric over a constant is emitted and the manifest has more than one semantic model, a new CONSTANT_METRIC_SEMANTIC_MODEL_LOSS issue is recorded instead of losing it silently.
Ossie → MSI: SUM(<constant>) now goes through the same dataset check as COUNT(*). With one dataset it converts as before (keeping its value, so SUM(2) stays SUM(2)); with several it is dropped with ROW_COUNT_METRIC_DROPPED instead of landing on the first dataset.

So the round trip COUNT(orders.*) → SUM(1) → MSI now ends with a recorded loss and a refusal, not a metric on the wrong table.

3. COUNT(DISTINCT *) and friends produced a broken metric.
COUNT(DISTINCT *), COUNT(DISTINCT 1), COUNT(DISTINCT orders.*) count whether any row exists (0 or 1), not how many. They fell through to the raw-expression fallback as a nested aggregate MetricFlow can't run, on a guessed dataset. They are now dropped with ROW_COUNT_METRIC_DROPPED wherever they appear in an expression: bare, in redundant parens like COUNT(DISTINCT (*)), or wrapped like COUNT(DISTINCT *) * 100 or COALESCE(COUNT(DISTINCT 1), 0).

Changes

msi_to_ossie.py: removed the row_count_metrics mechanism; records CONSTANT_METRIC_SEMANTIC_MODEL_LOSS.
expression_utils.py: _contains_distinct_row_count() (searches the whole tree, unwraps parens), _is_constant_expr(), and SUM(<constant>) recognition that keeps the constant.
ossie_to_msi.py: SUM(<constant>) goes through the row-count dataset resolver; the fallback drops COUNT(DISTINCT <row count>).
converter_issues.py, cli.py, README.md: the new issue type, its warning, and README entries for the new drops.
Tests: the MSI loss is checked through parse_manifest_from_dbt_generated_manifest, the way the CLI loads manifests, so it can't pass by skipping the loader. The round-trip test asserts the final refusal again. Added drop tests for all COUNT(DISTINCT ...) forms and for SUM(<constant>) with several datasets.

164 tests pass on Python 3.11, 3.12, 3.13 and 3.14.

Not in this PR

The other open threads on #432 touch the same dataset resolution and are left for a follow-up: a bare COUNT(*) nested inside a larger expression (e.g. NULLIF(COUNT(*), 0)), the two dataset resolvers disagreeing on qualifiers, case and quote sensitivity of qualifiers, and the intended behavior for a bare COUNT(*) over several datasets.

Related Issues

Related to #432 (already merged, so not a Closes/Fixes).

Checklist

Converters

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

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

Specification, Ontology, Validation, Documentation and Examples: not applicable.

A SUM metric with expr '1' already renders as SUM(1) through the generic path, with no special-casing needed. The row_count_metrics mechanism that tried to recover a row count's dataset by emitting COUNT(<dataset>.*) never worked on the real CLI path (MetricFlow's own transform already collapses COUNT to SUM before this converter sees the manifest), missed legacy measure-based count metrics entirely, and COUNT(<dataset>.*) isn't portable or spec-conformant anyway. Removed it.

COUNT(DISTINCT *), COUNT(DISTINCT 1) and similar are valid SQL, but counting distinct values of a constant answers "does any row exist" (0 or 1), not a meaningful total. They were falling through to the raw-expression fallback, producing a nested aggregate (SUM of a COUNT(DISTINCT ...) string) that MetricFlow cannot run, bound to a guessed dataset. Dropped instead, through the same path an unresolvable COUNT(*) already uses.

kayemkim

Ran this merged onto current main (b6c702e) through the dbt workflow on 3.11 to 3.14, all green. I also reproduced both findings on main first: a manifest routed through parse_manifest_from_dbt_generated_manifest already came out as SUM(1) with the #432 code, only the in-memory path produced COUNT(orders.*), and COUNT(DISTINCT 1) became expr='COUNT(DISTINCT 1)' under agg sum on the first dataset. On this branch both behave as described. 100.0 * COUNT(DISTINCT *) still reaches the raw fallback, which I read as the nested row-count bucket from the open #432 thread rather than this PR. LGTM.

One shade on the docstrings: SUM(1) and COUNT(*) agree on every non-empty input, but over zero rows COUNT(*) is 0 and SUM(1) is NULL (DuckDB 1.5). MetricFlow's own COUNT to SUM rewrite has the same property, so nothing changes in practice.

For sequencing, this conflicts with #437 in six files and with #388 in expression_utils.py, so whichever lands second needs a rebase.

jbonofre · 2026-10-02T06:25:07Z

+        No special case for a row count: ``SUM`` with ``expr == '1'`` already renders as ``SUM(1)``
+        through the generic path below, which is the same thing ``COUNT(*)`` means and is portable
+        across engines (unlike ``COUNT(<dataset>.*)``, which several engines reject or interpret
+        differently).


This log now drops the only record of which semantic model a row count belongs to. agg_params_obj.semantic_model is still in hand here, but SUM(1) has nowhere to carry it, and nothing is added to issues.

I understand COUNT(<dataset>.*) was dropped deliberately and I'm not asking for it back. I have two proposals:

record a ConverterIssue when a constant-expression SIMPLE metric is emitted and the manifest has more than one semantic model. The README says every MSI -> Ossie loss is recorded, and this one also happens on the CLI.

the round-trip test docstring says the forward gap is "tracked separately". Could we link the issue? My preference is to close it in this PR, since this PR makes SUM(1) the canonical form: have the forward converter refuse SUM(<constant>) when there is more than one dataset, the way a bar COUNT(*) is refused.

Did both. msi-to-ossie now records a CONSTANT_METRIC_SEMANTIC_MODEL_LOSS issue when there's more than one semantic model, and ossie-to-msi refuses SUM() with several datasets like a bare COUNT(*). Fixed in fa59308.

jbonofre · 2026-10-02T06:31:35Z

@@ -752,2 +778,2 @@
        assert "created_at" in field_names
        assert "amount" in field_names


With the third leg removed, nothing asserts which dataset a row count lands on after Ossie -> MSI -> Ossie -> MSI. The removed semantic_model = "orders" assertion was the only one that would catch the drift to customers. The docstring now describes the wrong binding, but no test pins it.

I propose to add the leg back in one of two forms:

If the forward converter refuses SUM(1) with several datasets, assert the metric is dropped with ROW_COUNT_METRIC_DROPPED.

Otherwise, add a pytest.mark.xfail(struct=True) test with the assertion below, so the gap is visible and the test flips when it is fixed.

again = OssieToMSIConverter().convert(ossie_doc).output order_count = next(m for m in again.metrics if m.name == "order_count") params = order_count.type_params.metric_aggregation_params assert params is not None assert params.semantic_model == "orders"

Added it back, it now asserts the drop. Fixed in fa59308.

jbonofre · 2026-10-02T06:33:58Z

+        # COUNT(DISTINCT *) / COUNT(DISTINCT 1) / ...: not a column count and not a row count either
+        # (it answers whether any row exists, 0 or 1). Drop rather than fall back to a raw SUM of it,
+        # which MetricFlow cannot run and which would guess a dataset the way a row count must not.
+        if _is_unsupported_distinct_row_count(expr_str):
+            raise _UnresolvedRowCountDataset(f"{expr_str!r} has no sensible SIMPLE or RATIO translation")


This guard only fires when the whole expression is a bare COUNT(DISTINICT ...). Wrapped form still reach the fallback below and become SUM(<raw>) bound to datasets[0] with no issue:

(COUNT(DISTINCT *))

COUNT(DISTINCT *) * 100

COALESCE(COUNT(DISTINCT 1), 0)

CAST(COUNT(DISTINCT *) AS INT)

COUNT(DISTINCT (*)) and COUNT(DISTINCT (1)) are missed for a different reason: _extract_agg_info returns COUNT_DISTINCT of '(*)' before this line is reached.

I suggest searching the tree and unwrapping parentheses:

def _contains_distinct_row_count(expression: str) -> bool: try: tree = sqlglot.parse_one(expression.strip()) except sqlglot.errors.ParseError: return False for count in tree.find_all(exp.Count): argument = count.this if count.args.get("expressions") or not isinstance(argument, exp.Distinct): continue if len(argument.expressions) == 1 and _is_row_count_argument(argument.expressions[0].unnest()): return True return False

Thanks, used your approach. All six of your examples are dropped now. Fixed in fa59308.

…e MSI loss - ossie-to-msi: SUM(<constant>) goes through the same dataset check as COUNT(*), refusing with ROW_COUNT_METRIC_DROPPED when there is more than one dataset. The constant keeps its value (SUM(2) stays SUM(2)). - msi-to-ossie: record CONSTANT_METRIC_SEMANTIC_MODEL_LOSS when a metric over a constant is emitted and the manifest has more than one semantic model, since SUM(1) has no column to carry it. - COUNT(DISTINCT <row count>) is dropped wherever it appears in an expression, including redundant parens and wrapped forms.

jbonofre · 2026-10-02T15:48:59Z

+    """Return True when ``expr`` is a non-null constant such as ``1``, ``2`` or ``TRUE``, not a column."""
+    try:
+        node = sqlglot.parse_one(expr)
+    except sqlglot.errors.ParseError:


_is_constant_expr only catches sqlglot.errors.ParseError. sqlglot raises TokenError for tokenizer failures such as an unterminated quote or an unsupported character, and TokenError is not a ParseError. This PR now calls it from msi_to_ossie on every SIMPLE metric's expr when the manifest has more than one semantic model. A dbt expr is raw SQL, so one odd metric would abort the whole MSI -> Ossie conversion. Before this PR, that expr was never parsed.

Could we catch sqlglot.errors.SqlglotError instead? A test with an unterminated-quote expr would also be good.

Switched to SqlglotError and added an unterminated-quote test. Fixed in ef964f7.

jbonofre · 2026-10-02T15:50:05Z

+    """
+    try:
+        tree = sqlglot.parse_one(expression.strip())
+    except sqlglot.errors.ParseError:


Same issue as earlier: _contains_distinct_row_count has the same except ParseError gap, and it's now called on the Ossie -> MSI path.

Same fix, I changed all the parse sites in that file. _extract_agg_info runs first on that path and had the same gap. Fixed in ef964f7.

jbonofre · 2026-10-02T15:53:18Z

+        """Return True for a SIMPLE metric over a constant expr, e.g. a row count rewritten to SUM(1)."""
+        params = metric.type_params.metric_aggregation_params
+        expr = metric.type_params.expr
+        return metric.type is MetricType.SIMPLE and params is not None and expr is not None and _is_constant_expr(expr)


Nit: _aggregates_a_constant flags any SIMPLE metric with a constant expr, whatever its aggregation. MAX(1) or AVG(1) would get CONSTANT_METRIC_SEMANTIC_MODEL_LOSS with a message saying converting back "will refuse it". But _extract_agg_info only returns the constant for SUM. Other aggregations go to the column lookup, so the message is wrong for them.

Could we restrict the check to params.agg is AggregationType.SUM?

Makes sense, limited it to SUM. Fixed in ef964f7.

sqlglot raises TokenError (e.g. for an unterminated quote), which is not a ParseError, so one odd expr aborted the whole conversion. Catch SqlglotError at every parse site. Also limit the constant-metric loss issue to SUM, the only aggregation the reverse direction refuses.

jbonofre

Thanks for the quick turnaround @LukasSchwarzlmueller!

LukasSchwarzlmueller · 2026-10-03T08:35:45Z

Thanks for the the reviews @jbonofre

github-actions Bot added converter converter/dbt labels Oct 1, 2026

kayemkim reviewed Oct 2, 2026

View reviewed changes

jbonofre requested changes Oct 2, 2026

View reviewed changes

LukasSchwarzlmueller force-pushed the fix/dbt-row-count-portability branch from b033d06 to fa59308 Compare October 2, 2026 14:02

jbonofre requested changes Oct 2, 2026

View reviewed changes

jbonofre self-requested a review October 3, 2026 02:44

jbonofre approved these changes Oct 3, 2026

View reviewed changes

jbonofre merged commit ff286d5 into apache:main Oct 3, 2026
8 checks passed

		@@ -752,2 +778,2 @@
		assert "created_at" in field_names
		assert "amount" in field_names

Conversation

LukasSchwarzlmueller commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Not in this PR

Related Issues

Checklist

Converters

Tests

Compliance

Uh oh!

kayemkim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LukasSchwarzlmueller commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LukasSchwarzlmueller commented Oct 1, 2026 •

edited

Loading