From 94085aff55a8ea15b68669b066afa9c946e0b912 Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Sat, 19 Sep 2026 23:35:57 +0200 Subject: [PATCH 1/3] fix(dbt): emit expr '1' for COUNT(*) metrics ossie-to-msi turned COUNT(*) into a count metric with expr '*'. MetricFlow renders count metrics as SUM(CASE WHEN IS NOT NULL THEN 1 ELSE 0 END), so the generated query contained CASE WHEN * IS NOT NULL, which is invalid SQL. Use the constant 1 instead. It is never null, so every row is counted, which is COUNT(*) semantics. COUNT(dataset.*) is handled the same way. --- converters/dbt/README.md | 1 + .../dbt/src/ossie_dbt/expression_utils.py | 11 ++++- converters/dbt/tests/test_ossie_to_msi.py | 41 +++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/converters/dbt/README.md b/converters/dbt/README.md index 06385b84..7554b1ac 100644 --- a/converters/dbt/README.md +++ b/converters/dbt/README.md @@ -129,6 +129,7 @@ manifest_json = result.output.model_dump_json(by_alias=True, exclude_none=True, - Composite primary and unique keys are rejected because MSI entities cannot preserve grouped key semantics - Single aggregations (`SUM(col)`, `COUNT(DISTINCT col)`, etc.) → SIMPLE metric with `metric_aggregation_params` +- `COUNT(*)` → `count` SIMPLE metric with `expr: '1'`, because MetricFlow cannot render a bare `*` inside a count - `(expr_a) / (expr_b)` → RATIO metric with auto-generated sub-metrics - Anything else → SIMPLE metric with the raw expression stored verbatim - Time dimensions always receive `TimeGranularity.DAY` (Ossie carries no granularity field) diff --git a/converters/dbt/src/ossie_dbt/expression_utils.py b/converters/dbt/src/ossie_dbt/expression_utils.py index 4e553da9..daf91fd9 100644 --- a/converters/dbt/src/ossie_dbt/expression_utils.py +++ b/converters/dbt/src/ossie_dbt/expression_utils.py @@ -36,13 +36,18 @@ def _col_name(node: exp.Expression) -> str: return _strip_qualifier(rendered) +def _is_star(node: exp.Expression) -> bool: + """Return True for ``*`` and for a qualified ``dataset.*``.""" + return isinstance(node, exp.Star) or (isinstance(node, exp.Column) and isinstance(node.this, exp.Star)) + + def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float], bool]]: """Parse a SQL aggregation expression using sqlglot. Returns ``(agg_type, bare_col, percentile, use_discrete_percentile)`` for recognised patterns, ``None`` otherwise. ``percentile`` is only set for ``PERCENTILE`` aggregations; it is ``None`` for all others. ``use_discrete_percentile`` is ``True`` only for ``PERCENTILE_DISC``. - The returned column name has any dataset qualifier stripped. + The returned column name has any dataset qualifier stripped; ``COUNT(*)`` returns the constant ``"1"``. """ try: tree = sqlglot.parse_one(expression.strip()) @@ -56,6 +61,10 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O return AggregationType.COUNT_DISTINCT, _col_name(cols[0]), None, False return None + # COUNT(*) → count of the constant 1 (MetricFlow cannot render a bare * inside a count) + if isinstance(tree, exp.Count) and _is_star(tree.this): + return AggregationType.COUNT, "1", None, False + # COUNT(col) if isinstance(tree, exp.Count): return AggregationType.COUNT, _col_name(tree.this), None, False diff --git a/converters/dbt/tests/test_ossie_to_msi.py b/converters/dbt/tests/test_ossie_to_msi.py index c68e547d..bf0901ad 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -319,6 +319,47 @@ def test_count_distinct_expression(self) -> None: sm = result.semantic_models[0] assert len(sm.measures) == 0 + @pytest.mark.parametrize("expression", ["COUNT(*)", "count( * )", "COUNT(orders.*)"]) + def test_count_star_uses_constant_expr(self, expression: str) -> None: + doc = _ossie_doc( + datasets=[_ossie_dataset("orders", fields=[_ossie_field("order_id")])], + metrics=[_ossie_metric("order_count", expression)], + ) + result = OssieToMSIConverter().convert(doc).output + + m = result.metrics[0] + assert m.type_params.metric_aggregation_params is not None + assert m.type_params.metric_aggregation_params.agg == AggregationType.COUNT + assert m.type_params.expr == "1" + + def test_qualified_count_star_uses_dataset_qualifier(self) -> None: + doc = _ossie_doc( + datasets=[ + _ossie_dataset("customers", fields=[_ossie_field("customer_id")]), + _ossie_dataset("orders", fields=[_ossie_field("order_id")]), + ], + metrics=[_ossie_metric("order_count", "COUNT(orders.*)")], + ) + result = OssieToMSIConverter().convert(doc).output + + m = result.metrics[0] + assert m.type_params.expr == "1" + assert m.type_params.metric_aggregation_params is not None + assert m.type_params.metric_aggregation_params.semantic_model == "orders" + + def test_count_star_in_ratio_uses_constant_expr(self) -> None: + doc = _ossie_doc( + datasets=[_ossie_dataset("orders", fields=[_ossie_field("amount")])], + metrics=[_ossie_metric("avg_order_value", "(SUM(amount)) / (COUNT(*))")], + ) + result = OssieToMSIConverter().convert(doc).output + + by_name = {m.name: m for m in result.metrics} + denominator = by_name["avg_order_value__denominator"] + assert denominator.type_params.metric_aggregation_params is not None + assert denominator.type_params.metric_aggregation_params.agg == AggregationType.COUNT + assert denominator.type_params.expr == "1" + def test_ratio_expression_produces_ratio_metric(self) -> None: doc = _ossie_doc( datasets=[ From 664e3a13f7fc847760ff669789fb1d241120ab1c Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Sun, 20 Sep 2026 12:28:11 +0200 Subject: [PATCH 2/3] fix(dbt): bind COUNT(*) to its dataset and keep it on round trip --- converters/dbt/README.md | 2 +- .../dbt/src/ossie_dbt/expression_utils.py | 32 ++--- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 17 ++- converters/dbt/src/ossie_dbt/ossie_to_msi.py | 39 +++++- converters/dbt/tests/test_msi_to_ossie.py | 52 ++++++++ converters/dbt/tests/test_ossie_to_msi.py | 112 ++++++++++++++++-- 6 files changed, 230 insertions(+), 24 deletions(-) diff --git a/converters/dbt/README.md b/converters/dbt/README.md index 7554b1ac..76a1776b 100644 --- a/converters/dbt/README.md +++ b/converters/dbt/README.md @@ -129,7 +129,7 @@ manifest_json = result.output.model_dump_json(by_alias=True, exclude_none=True, - Composite primary and unique keys are rejected because MSI entities cannot preserve grouped key semantics - Single aggregations (`SUM(col)`, `COUNT(DISTINCT col)`, etc.) → SIMPLE metric with `metric_aggregation_params` -- `COUNT(*)` → `count` SIMPLE metric with `expr: '1'`, because MetricFlow cannot render a bare `*` inside a count +- `COUNT(*)` / `COUNT(.*)` → `count` SIMPLE metric with `expr: '1'`, because MetricFlow cannot render a bare `*` inside a count. The counted dataset comes from the qualifier, so with more than one dataset write `COUNT(orders.*)`; a bare `COUNT(*)` is rejected as ambiguous - `(expr_a) / (expr_b)` → RATIO metric with auto-generated sub-metrics - Anything else → SIMPLE metric with the raw expression stored verbatim - Time dimensions always receive `TimeGranularity.DAY` (Ossie carries no granularity field) diff --git a/converters/dbt/src/ossie_dbt/expression_utils.py b/converters/dbt/src/ossie_dbt/expression_utils.py index daf91fd9..9d82e38a 100644 --- a/converters/dbt/src/ossie_dbt/expression_utils.py +++ b/converters/dbt/src/ossie_dbt/expression_utils.py @@ -22,6 +22,9 @@ from metricflow_semantic_interfaces.type_enums import AggregationType +# expr for "count all rows": MetricFlow wraps a count's expr in CASE WHEN, where a bare * is invalid +ROW_COUNT_EXPR = "1" + def _strip_qualifier(col: str) -> str: """Strip a leading dataset qualifier, e.g. 'orders.amount' → 'amount'.""" @@ -47,27 +50,28 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O Returns ``(agg_type, bare_col, percentile, use_discrete_percentile)`` for recognised patterns, ``None`` otherwise. ``percentile`` is only set for ``PERCENTILE`` aggregations; it is ``None`` for all others. ``use_discrete_percentile`` is ``True`` only for ``PERCENTILE_DISC``. - The returned column name has any dataset qualifier stripped; ``COUNT(*)`` returns the constant ``"1"``. + The returned column name has any dataset qualifier stripped. ``COUNT(*)`` and ``COUNT(dataset.*)`` return + ``ROW_COUNT_EXPR`` instead of a column name; ``COUNT(DISTINCT *)`` and multi-argument ``COUNT`` return ``None``. """ try: tree = sqlglot.parse_one(expression.strip()) except sqlglot.errors.ParseError: return None - # COUNT(DISTINCT col) - if isinstance(tree, exp.Count) and isinstance(tree.this, exp.Distinct): - cols = tree.this.expressions - if len(cols) == 1: - return AggregationType.COUNT_DISTINCT, _col_name(cols[0]), None, False - return None - - # COUNT(*) → count of the constant 1 (MetricFlow cannot render a bare * inside a count) - if isinstance(tree, exp.Count) and _is_star(tree.this): - return AggregationType.COUNT, "1", None, False - - # COUNT(col) if isinstance(tree, exp.Count): - return AggregationType.COUNT, _col_name(tree.this), None, False + # COUNT(a, b) has no single-column equivalent + if tree.args.get("expressions"): + return None + argument, distinct = tree.this, False + if isinstance(argument, exp.Distinct): + operands = argument.expressions + if len(operands) != 1: + return None + argument, distinct = operands[0], True + if _is_star(argument): + # COUNT(*) → count all rows; COUNT(DISTINCT *) is not valid SQL + return None if distinct else (AggregationType.COUNT, ROW_COUNT_EXPR, None, False) + return (AggregationType.COUNT_DISTINCT if distinct else AggregationType.COUNT), _col_name(argument), None, False # SUM(CASE WHEN col THEN 1 ELSE 0 END) → SUM_BOOLEAN if isinstance(tree, exp.Sum) and isinstance(tree.this, exp.Case): diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index 638be16f..c78e976d 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -19,7 +19,7 @@ from collections import defaultdict from dataclasses import dataclass from itertools import combinations -from typing import Dict, List, Optional, Sequence, Tuple +from typing import Dict, FrozenSet, List, Optional, Sequence, Tuple from ossie import ( OssieDataset, @@ -33,6 +33,7 @@ OssieRelationship, ) from ossie_dbt.converter_issues import ConverterIssue, ConverterIssueType, ConverterResult +from ossie_dbt.expression_utils import ROW_COUNT_EXPR from ossie_dbt.filter_utils import _collect_filter_sql, _merge_filter_sqls from metricflow_semantic_interfaces.enum_extension import assert_values_exhausted @@ -77,10 +78,21 @@ class MSIToOssieConverter: def __init__(self, dialect: OssieDialect = OssieDialect.ANSI_SQL) -> None: self._dialect = dialect + self._row_count_metrics: FrozenSet[str] = frozenset() def convert( self, manifest: PydanticSemanticManifest, ossie_model_name: str = "semantic_model" ) -> ConverterResult[OssieDocument]: + # The transformer rewrites COUNT to SUM (leaving expr '1' as SUM(1)), which loses the dataset a row + # count belongs to. Remember these metrics so they come back as COUNT(.*). + self._row_count_metrics = frozenset( + metric.name + for metric in manifest.metrics + if metric.type is MetricType.SIMPLE + and metric.type_params.metric_aggregation_params is not None + and metric.type_params.metric_aggregation_params.agg is AggregationType.COUNT + and metric.type_params.expr == ROW_COUNT_EXPR + ) manifest = PydanticSemanticManifestTransformer.transform(manifest) issues: List[ConverterIssue] = [] @@ -251,6 +263,9 @@ def _resolve_simple( raise ValueError( f"SIMPLE metric has no metric_aggregation_params after transformation: metric_name={metric.name!r}" ) + # With a filter the count is emitted as SUM(CASE WHEN THEN 1 END), which has no `dataset.*` form. + if metric.name in self._row_count_metrics and not filter_sql: + return f"COUNT({agg_params_obj.semantic_model}.*)" col = metric.type_params.expr if metric.type_params.expr is not None else metric.name col = self._qualify_col(col, agg_params_obj.semantic_model) return self._build_agg_expression(agg_params_obj.agg, col, agg_params_obj.agg_params, filter_sql) diff --git a/converters/dbt/src/ossie_dbt/ossie_to_msi.py b/converters/dbt/src/ossie_dbt/ossie_to_msi.py index 4374d61e..c4a50772 100644 --- a/converters/dbt/src/ossie_dbt/ossie_to_msi.py +++ b/converters/dbt/src/ossie_dbt/ossie_to_msi.py @@ -28,6 +28,7 @@ ) from ossie_dbt.converter_issues import ConverterResult from ossie_dbt.expression_utils import ( + ROW_COUNT_EXPR, _extract_agg_info, _get_dataset_qualifier, _strip_qualifier, @@ -293,7 +294,11 @@ def _convert_metric( agg_result = _extract_agg_info(expr_str) if agg_result is not None: agg, col, percentile, use_discrete = agg_result - semantic_model_name = self._find_dataset_for_col(expr_str, col, datasets) + if agg is AggregationType.COUNT and col == ROW_COUNT_EXPR: + # A constant, not a column: it must not go through the column → dataset lookup. + semantic_model_name = self._find_dataset_for_row_count(name, expr_str, datasets) + else: + semantic_model_name = self._find_dataset_for_col(expr_str, col, datasets) agg_params = ( PydanticMeasureAggregationParameters( percentile=percentile, @@ -404,6 +409,38 @@ def _find_dataset_for_col( return datasets[0].name if datasets else "" + @staticmethod + def _find_dataset_for_row_count( + metric_name: str, + raw_expr_str: str, + datasets: List[OssieDataset], + ) -> str: + """Determine which dataset a ``COUNT(*)`` counts the rows of. + + Only the qualifier decides: ``COUNT(orders.*)`` names ``orders`` (a schema prefix such as + ``db.orders.*`` is matched on its last segment). A bare ``COUNT(*)`` is only unambiguous when the + document has a single dataset; otherwise guessing would count the rows of an unrelated table. + """ + dataset_names = [dataset.name for dataset in datasets] + qualifier = _get_dataset_qualifier(raw_expr_str) + if qualifier is None: + if len(dataset_names) == 1: + return dataset_names[0] + raise ValueError( + f"Metric {metric_name!r}: 'COUNT(*)' is ambiguous with {len(dataset_names)} datasets " + f"({', '.join(dataset_names)}); qualify it as 'COUNT(.*)'" + ) + + if qualifier in dataset_names: + return qualifier + by_last_segment = [name for name in dataset_names if _strip_qualifier(name) == _strip_qualifier(qualifier)] + if len(by_last_segment) == 1: + return by_last_segment[0] + raise ValueError( + f"Metric {metric_name!r}: 'COUNT({qualifier}.*)' does not match exactly one dataset " + f"(datasets: {', '.join(dataset_names)})" + ) + def _get_expression(self, ossie_expr: OssieExpression) -> str: """Return the expression string for the preferred dialect (fallback: first available).""" for dialect_expr in ossie_expr.dialects: diff --git a/converters/dbt/tests/test_msi_to_ossie.py b/converters/dbt/tests/test_msi_to_ossie.py index 7ed0aa89..a27c13e7 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -81,6 +81,34 @@ def _ossie_metrics(result: OssieDocument) -> list: return metrics +def _metric_with_agg( + name: str, + agg: AggregationType, + expr: str, + semantic_model: str, + filter_sql: Optional[str] = None, +) -> PydanticMetric: + """A SIMPLE metric that carries its aggregation in metric_aggregation_params.""" + return PydanticMetric( + name=name, + description=None, + type=MetricType.SIMPLE, + type_params=PydanticMetricTypeParams( + expr=expr, + metric_aggregation_params=PydanticMetricAggregationParams( + semantic_model=semantic_model, + agg=agg, + agg_params=None, + agg_time_dimension=None, + non_additive_dimension=None, + ), + ), + filter=_filter(filter_sql) if filter_sql else None, + metadata=default_meta(), + config=None, + ) + + # --------------------------------------------------------------------------- # Tests # --------------------------------------------------------------------------- @@ -584,6 +612,30 @@ def test_simple_metric_with_metric_aggregation_params(self) -> None: assert _ossie_metrics(result)[0].expression.dialects[0].expression == "AVG(orders.price)" + def test_count_of_all_rows_keeps_its_semantic_model(self) -> None: + customers = semantic_model_with_guaranteed_meta(name="customers") + orders = semantic_model_with_guaranteed_meta(name="orders") + metric = _metric_with_agg("order_count", AggregationType.COUNT, "1", "orders") + result = MSIToOssieConverter().convert(_manifest(semantic_models=[customers, orders], metrics=[metric])).output + + assert _ossie_metrics(result)[0].expression.dialects[0].expression == "COUNT(orders.*)" + + def test_sum_of_constant_one_stays_a_sum(self) -> None: + orders = semantic_model_with_guaranteed_meta(name="orders") + metric = _metric_with_agg("row_total", AggregationType.SUM, "1", "orders") + result = MSIToOssieConverter().convert(_manifest(semantic_models=[orders], metrics=[metric])).output + + assert _ossie_metrics(result)[0].expression.dialects[0].expression == "SUM(1)" + + def test_filtered_count_of_all_rows_keeps_the_filter(self) -> None: + orders = semantic_model_with_guaranteed_meta(name="orders") + metric = _metric_with_agg("paid_orders", AggregationType.COUNT, "1", "orders", filter_sql="status = 'paid'") + result = MSIToOssieConverter().convert(_manifest(semantic_models=[orders], metrics=[metric])).output + + assert ( + _ossie_metrics(result)[0].expression.dialects[0].expression == "SUM(CASE WHEN status = 'paid' THEN 1 END)" + ) + # --- RATIO --- def test_ratio_metric_inlines_sub_expressions(self, snapshot: SnapshotAssertion) -> None: diff --git a/converters/dbt/tests/test_ossie_to_msi.py b/converters/dbt/tests/test_ossie_to_msi.py index bf0901ad..8475aff4 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -319,7 +319,7 @@ def test_count_distinct_expression(self) -> None: sm = result.semantic_models[0] assert len(sm.measures) == 0 - @pytest.mark.parametrize("expression", ["COUNT(*)", "count( * )", "COUNT(orders.*)"]) + @pytest.mark.parametrize("expression", ["COUNT(*)", "COUNT(orders.*)"]) def test_count_star_uses_constant_expr(self, expression: str) -> None: doc = _ossie_doc( datasets=[_ossie_dataset("orders", fields=[_ossie_field("order_id")])], @@ -347,18 +347,86 @@ def test_qualified_count_star_uses_dataset_qualifier(self) -> None: assert m.type_params.metric_aggregation_params is not None assert m.type_params.metric_aggregation_params.semantic_model == "orders" - def test_count_star_in_ratio_uses_constant_expr(self) -> None: + @staticmethod + def _customers_and_orders() -> list: + return [ + _ossie_dataset("customers", fields=[_ossie_field("customer_id")]), + _ossie_dataset("orders", fields=[_ossie_field("order_id"), _ossie_field("amount")]), + ] + + @pytest.mark.parametrize("expression", ["COUNT(*)", "COUNT(1)"]) + def test_bare_row_count_with_multiple_datasets_is_ambiguous(self, expression: str) -> None: + doc = _ossie_doc(datasets=self._customers_and_orders(), metrics=[_ossie_metric("order_count", expression)]) + + with pytest.raises(ValueError, match="ambiguous with 2 datasets"): + OssieToMSIConverter().convert(doc) + + def test_bare_count_star_in_ratio_with_multiple_datasets_is_ambiguous(self) -> None: doc = _ossie_doc( - datasets=[_ossie_dataset("orders", fields=[_ossie_field("amount")])], + datasets=self._customers_and_orders(), metrics=[_ossie_metric("avg_order_value", "(SUM(amount)) / (COUNT(*))")], ) + + with pytest.raises(ValueError, match="ambiguous"): + OssieToMSIConverter().convert(doc) + + def test_qualified_count_star_in_ratio_binds_both_sides_to_the_same_dataset(self) -> None: + doc = _ossie_doc( + datasets=self._customers_and_orders(), + metrics=[_ossie_metric("avg_order_value", "(SUM(orders.amount)) / (COUNT(orders.*))")], + ) result = OssieToMSIConverter().convert(doc).output by_name = {m.name: m for m in result.metrics} - denominator = by_name["avg_order_value__denominator"] - assert denominator.type_params.metric_aggregation_params is not None - assert denominator.type_params.metric_aggregation_params.agg == AggregationType.COUNT - assert denominator.type_params.expr == "1" + for sub_metric in ("avg_order_value__numerator", "avg_order_value__denominator"): + params = by_name[sub_metric].type_params.metric_aggregation_params + assert params is not None + assert params.semantic_model == "orders" + + def test_schema_qualified_count_star_matches_dataset_by_last_segment(self) -> None: + doc = _ossie_doc( + datasets=self._customers_and_orders(), + metrics=[_ossie_metric("order_count", "COUNT(db.orders.*)")], + ) + result = OssieToMSIConverter().convert(doc).output + + params = result.metrics[0].type_params.metric_aggregation_params + assert params is not None + assert params.semantic_model == "orders" + + def test_count_star_of_unknown_dataset_is_rejected(self) -> None: + doc = _ossie_doc( + datasets=self._customers_and_orders(), + metrics=[_ossie_metric("order_count", "COUNT(nope.*)")], + ) + + with pytest.raises(ValueError, match="does not match exactly one dataset"): + OssieToMSIConverter().convert(doc) + + def test_count_star_matching_several_datasets_by_last_segment_is_rejected(self) -> None: + doc = _ossie_doc( + datasets=[ + _ossie_dataset("a.orders", fields=[_ossie_field("order_id")]), + _ossie_dataset("b.orders", fields=[_ossie_field("order_id")]), + ], + metrics=[_ossie_metric("order_count", "COUNT(orders.*)")], + ) + + with pytest.raises(ValueError, match="does not match exactly one dataset"): + OssieToMSIConverter().convert(doc) + + @pytest.mark.parametrize( + "expression", + ["COUNT(DISTINCT *)", "COUNT(orders.*, amount)", "COUNT(db.orders, *)"], + ) + def test_unsupported_count_star_forms_fall_back_to_the_raw_expression(self, expression: str) -> None: + doc = _ossie_doc( + datasets=[_ossie_dataset("orders", fields=[_ossie_field("order_id"), _ossie_field("amount")])], + metrics=[_ossie_metric("odd_count", expression)], + ) + result = OssieToMSIConverter().convert(doc).output + + assert result.metrics[0].type_params.expr == expression def test_ratio_expression_produces_ratio_metric(self) -> None: doc = _ossie_doc( @@ -537,6 +605,36 @@ def test_ossie_to_msi_to_ossie_preserves_structure(self, snapshot: SnapshotAsser assert metrics[0].expression.dialects[0].expression == "SUM(orders.amount)" assert ossie_doc.to_ossie_yaml() == snapshot + def test_count_star_keeps_its_dataset_across_round_trip(self) -> None: + """COUNT(orders.*) must not drift to another dataset (or to SUM) on Ossie → MSI → Ossie → MSI.""" + original = _ossie_doc( + datasets=[ + _ossie_dataset("customers", fields=[_ossie_field("customer_id")]), + _ossie_dataset("orders", fields=[_ossie_field("order_id"), _ossie_field("amount")]), + ], + metrics=[ + _ossie_metric("order_count", "COUNT(orders.*)"), + _ossie_metric("avg_order_value", "(SUM(orders.amount)) / (COUNT(orders.*))"), + ], + ) + + msi = OssieToMSIConverter().convert(original).output + ossie_doc = MSIToOssieConverter().convert(msi).output + + expressions = {m.name: m.expression.dialects[0].expression for m in ossie_doc.metrics or []} + assert expressions["order_count"] == "COUNT(orders.*)" + assert "COUNT(orders.*)" in expressions["avg_order_value"] + + 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.agg, order_count.type_params.expr, params.semantic_model) == ( + AggregationType.COUNT, + "1", + "orders", + ) + def test_discrete_percentile_survives_round_trip(self) -> None: """A PERCENTILE_DISC metric keeps use_discrete_percentile through MSI -> Ossie -> MSI.""" orders = semantic_model_with_guaranteed_meta( From 08a693dfb4468f51f71088ae8192657926e21e8b Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Sun, 20 Sep 2026 21:22:33 +0200 Subject: [PATCH 3/3] fix(dbt): skip ambiguous COUNT(*) metrics with a warning --- converters/dbt/README.md | 2 +- converters/dbt/src/ossie_dbt/cli.py | 19 +++++--- .../dbt/src/ossie_dbt/converter_issues.py | 1 + converters/dbt/src/ossie_dbt/ossie_to_msi.py | 45 ++++++++++--------- converters/dbt/tests/test_cli.py | 21 +++++++++ converters/dbt/tests/test_ossie_to_msi.py | 34 ++++++++------ 6 files changed, 82 insertions(+), 40 deletions(-) diff --git a/converters/dbt/README.md b/converters/dbt/README.md index 76a1776b..48b61f47 100644 --- a/converters/dbt/README.md +++ b/converters/dbt/README.md @@ -129,7 +129,7 @@ manifest_json = result.output.model_dump_json(by_alias=True, exclude_none=True, - Composite primary and unique keys are rejected because MSI entities cannot preserve grouped key semantics - Single aggregations (`SUM(col)`, `COUNT(DISTINCT col)`, etc.) → SIMPLE metric with `metric_aggregation_params` -- `COUNT(*)` / `COUNT(.*)` → `count` SIMPLE metric with `expr: '1'`, because MetricFlow cannot render a bare `*` inside a count. The counted dataset comes from the qualifier, so with more than one dataset write `COUNT(orders.*)`; a bare `COUNT(*)` is rejected as ambiguous +- `COUNT(*)` / `COUNT(.*)` → `count` SIMPLE metric with `expr: '1'`, because MetricFlow cannot render a bare `*` inside a count. The counted dataset comes from the qualifier, so with more than one dataset write `COUNT(orders.*)`; a bare `COUNT(*)`, or a qualifier that matches no dataset, is skipped with a `ROW_COUNT_METRIC_DROPPED` warning - `(expr_a) / (expr_b)` → RATIO metric with auto-generated sub-metrics - Anything else → SIMPLE metric with the raw expression stored verbatim - Time dimensions always receive `TimeGranularity.DAY` (Ossie carries no granularity field) diff --git a/converters/dbt/src/ossie_dbt/cli.py b/converters/dbt/src/ossie_dbt/cli.py index a4fce8d2..7f683f18 100644 --- a/converters/dbt/src/ossie_dbt/cli.py +++ b/converters/dbt/src/ossie_dbt/cli.py @@ -25,11 +25,12 @@ import argparse import sys from pathlib import Path +from typing import Sequence import yaml from ossie import OssieDocument -from ossie_dbt.converter_issues import ConverterIssueType +from ossie_dbt.converter_issues import ConverterIssue, ConverterIssueType from ossie_dbt.msi_to_ossie import MSIToOssieConverter from ossie_dbt.ossie_to_msi import OssieToMSIConverter @@ -40,15 +41,24 @@ ConverterIssueType.PRIVATE_METRIC_DROPPED: "Ossie has no visibility modifiers", ConverterIssueType.NATURAL_ENTITY_DROPPED: "Ossie has no natural-key entity type", ConverterIssueType.CUMULATIVE_SEMANTICS_LOSS: "Ossie expressions cannot represent window or grain semantics; the base aggregation was preserved", + ConverterIssueType.ROW_COUNT_METRIC_DROPPED: "COUNT(*) does not identify exactly one dataset to count rows of; write it as COUNT(.*)", } _DROPPED_ISSUE_TYPES = { ConverterIssueType.CONVERSION_METRIC_DROPPED, ConverterIssueType.PRIVATE_METRIC_DROPPED, ConverterIssueType.NATURAL_ENTITY_DROPPED, + ConverterIssueType.ROW_COUNT_METRIC_DROPPED, } +def _print_issues(issues: Sequence[ConverterIssue]) -> None: + for issue in issues: + verb = "was dropped" if issue.issue_type in _DROPPED_ISSUE_TYPES else "was converted with loss" + reason = _ISSUE_REASON[issue.issue_type] + print(f"[WARNING] {issue.issue_type.value}: {issue.element_name} {verb} during conversion because {reason}", file=sys.stderr) + + def _cmd_msi_to_ossie(args: argparse.Namespace) -> None: input_path = Path(args.input) output_path = Path(args.output) @@ -56,11 +66,7 @@ def _cmd_msi_to_ossie(args: argparse.Namespace) -> None: manifest = parse_manifest_from_dbt_generated_manifest(input_path.read_text()) result = MSIToOssieConverter().convert(manifest, ossie_model_name=args.model_name) - if result.issues: - for issue in result.issues: - verb = "was dropped" if issue.issue_type in _DROPPED_ISSUE_TYPES else "was converted with loss" - reason = _ISSUE_REASON[issue.issue_type] - print(f"[WARNING] {issue.issue_type.value}: {issue.element_name} {verb} during conversion because {reason}", file=sys.stderr) + _print_issues(result.issues) output_path.write_text(result.output.to_ossie_yaml()) print(f"Written to {output_path}", file=sys.stderr) @@ -73,6 +79,7 @@ def _cmd_ossie_to_msi(args: argparse.Namespace) -> None: raw = yaml.safe_load(input_path.read_text()) document = OssieDocument.model_validate(raw) result = OssieToMSIConverter().convert(document) + _print_issues(result.issues) # PydanticSemanticManifest subclasses pydantic.v1.BaseModel, whose JSON # serializer is .json(), not the pydantic v2 .model_dump_json(). diff --git a/converters/dbt/src/ossie_dbt/converter_issues.py b/converters/dbt/src/ossie_dbt/converter_issues.py index e1606fa1..6e2f9888 100644 --- a/converters/dbt/src/ossie_dbt/converter_issues.py +++ b/converters/dbt/src/ossie_dbt/converter_issues.py @@ -27,6 +27,7 @@ class ConverterIssueType(Enum): PRIVATE_METRIC_DROPPED = "PRIVATE_METRIC_DROPPED" NATURAL_ENTITY_DROPPED = "NATURAL_ENTITY_DROPPED" CUMULATIVE_SEMANTICS_LOSS = "CUMULATIVE_SEMANTICS_LOSS" + ROW_COUNT_METRIC_DROPPED = "ROW_COUNT_METRIC_DROPPED" @dataclass(frozen=True) diff --git a/converters/dbt/src/ossie_dbt/ossie_to_msi.py b/converters/dbt/src/ossie_dbt/ossie_to_msi.py index c4a50772..a19775c3 100644 --- a/converters/dbt/src/ossie_dbt/ossie_to_msi.py +++ b/converters/dbt/src/ossie_dbt/ossie_to_msi.py @@ -16,7 +16,7 @@ # under the License. from dataclasses import dataclass -from typing import List, Optional, Set +from typing import List, Optional, Set, Tuple from ossie import ( OssieDataset, @@ -26,7 +26,7 @@ OssieField, OssieSemanticModel, ) -from ossie_dbt.converter_issues import ConverterResult +from ossie_dbt.converter_issues import ConverterIssue, ConverterIssueType, ConverterResult from ossie_dbt.expression_utils import ( ROW_COUNT_EXPR, _extract_agg_info, @@ -75,6 +75,10 @@ class _KeySets: foreign: Set[str] +class _UnresolvedRowCountDataset(Exception): + """A ``COUNT(*)`` that does not identify exactly one dataset to count the rows of.""" + + class OssieToMSIConverter: """Converts an Ossie Document into a PydanticSemanticManifest. @@ -100,11 +104,10 @@ def __init__(self, dialect: OssieDialect = OssieDialect.ANSI_SQL) -> None: def convert(self, document: OssieDocument) -> ConverterResult[PydanticSemanticManifest]: semantic_models: List[PydanticSemanticModel] = [] - metrics: List[PydanticMetric] = [] for dataset in document.datasets: semantic_models.append(self._convert_dataset(dataset, document)) - metrics.extend(self._convert_metrics(document)) + metrics, issues = self._convert_metrics(document) return ConverterResult( output=PydanticSemanticManifest( @@ -112,7 +115,7 @@ def convert(self, document: OssieDocument) -> ConverterResult[PydanticSemanticMa metrics=metrics, project_configuration=PydanticProjectConfiguration(), ), - issues=[], + issues=issues, ) # ------------------------------------------------------------------ @@ -268,12 +271,19 @@ def _classify_field( # Metric conversion # ------------------------------------------------------------------ - def _convert_metrics(self, ossie_sm: OssieSemanticModel) -> List[PydanticMetric]: + def _convert_metrics(self, ossie_sm: OssieSemanticModel) -> Tuple[List[PydanticMetric], List[ConverterIssue]]: metrics: List[PydanticMetric] = [] + issues: List[ConverterIssue] = [] for metric in ossie_sm.metrics or []: expr_str = self._get_expression(metric.expression) - metrics.extend(self._convert_metric(metric.name, expr_str, metric.description, ossie_sm.datasets)) - return metrics + try: + metrics.extend(self._convert_metric(metric.name, expr_str, metric.description, ossie_sm.datasets)) + except _UnresolvedRowCountDataset: + # Also drops a ratio and its sub-metrics as a whole, so nothing references a missing metric. + issues.append( + ConverterIssue(issue_type=ConverterIssueType.ROW_COUNT_METRIC_DROPPED, element_name=metric.name) + ) + return metrics, issues def _convert_metric( self, @@ -296,7 +306,7 @@ def _convert_metric( agg, col, percentile, use_discrete = agg_result if agg is AggregationType.COUNT and col == ROW_COUNT_EXPR: # A constant, not a column: it must not go through the column → dataset lookup. - semantic_model_name = self._find_dataset_for_row_count(name, expr_str, datasets) + semantic_model_name = self._find_dataset_for_row_count(expr_str, datasets) else: semantic_model_name = self._find_dataset_for_col(expr_str, col, datasets) agg_params = ( @@ -410,25 +420,21 @@ def _find_dataset_for_col( return datasets[0].name if datasets else "" @staticmethod - def _find_dataset_for_row_count( - metric_name: str, - raw_expr_str: str, - datasets: List[OssieDataset], - ) -> str: + def _find_dataset_for_row_count(raw_expr_str: str, datasets: List[OssieDataset]) -> str: """Determine which dataset a ``COUNT(*)`` counts the rows of. Only the qualifier decides: ``COUNT(orders.*)`` names ``orders`` (a schema prefix such as ``db.orders.*`` is matched on its last segment). A bare ``COUNT(*)`` is only unambiguous when the document has a single dataset; otherwise guessing would count the rows of an unrelated table. + Raises ``_UnresolvedRowCountDataset`` when no single dataset can be determined. """ dataset_names = [dataset.name for dataset in datasets] qualifier = _get_dataset_qualifier(raw_expr_str) if qualifier is None: if len(dataset_names) == 1: return dataset_names[0] - raise ValueError( - f"Metric {metric_name!r}: 'COUNT(*)' is ambiguous with {len(dataset_names)} datasets " - f"({', '.join(dataset_names)}); qualify it as 'COUNT(.*)'" + raise _UnresolvedRowCountDataset( + f"'COUNT(*)' is ambiguous with {len(dataset_names)} datasets ({', '.join(dataset_names)})" ) if qualifier in dataset_names: @@ -436,9 +442,8 @@ def _find_dataset_for_row_count( by_last_segment = [name for name in dataset_names if _strip_qualifier(name) == _strip_qualifier(qualifier)] if len(by_last_segment) == 1: return by_last_segment[0] - raise ValueError( - f"Metric {metric_name!r}: 'COUNT({qualifier}.*)' does not match exactly one dataset " - f"(datasets: {', '.join(dataset_names)})" + raise _UnresolvedRowCountDataset( + f"'COUNT({qualifier}.*)' does not match exactly one dataset ({', '.join(dataset_names)})" ) def _get_expression(self, ossie_expr: OssieExpression) -> str: diff --git a/converters/dbt/tests/test_cli.py b/converters/dbt/tests/test_cli.py index 892e4a9d..b5ec07fb 100644 --- a/converters/dbt/tests/test_cli.py +++ b/converters/dbt/tests/test_cli.py @@ -59,3 +59,24 @@ def test_ossie_to_msi_writes_valid_manifest_json(tmp_path: Path, monkeypatch: py manifest = json.loads(output_path.read_text()) assert "semantic_models" in manifest assert "metrics" in manifest + + +def test_ossie_to_msi_warns_about_a_dropped_metric( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """A bare COUNT(*) over several datasets is skipped, and the CLI says so on stderr.""" + document = _ossie_doc( + datasets=[ + _ossie_dataset("customers", fields=[_ossie_field("customer_id")]), + _ossie_dataset("orders", fields=[_ossie_field("order_id")]), + ], + metrics=[_ossie_metric("order_count", "COUNT(*)")], + ) + input_path = tmp_path / "model.yaml" + output_path = tmp_path / "semantic_manifest.json" + input_path.write_text(document.to_ossie_yaml()) + + _run_cli(["ossie-to-msi", "-i", str(input_path), "-o", str(output_path)], monkeypatch) + + assert "ROW_COUNT_METRIC_DROPPED: order_count was dropped" in capsys.readouterr().err + assert json.loads(output_path.read_text())["metrics"] == [] diff --git a/converters/dbt/tests/test_ossie_to_msi.py b/converters/dbt/tests/test_ossie_to_msi.py index 8475aff4..056dbd63 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -21,6 +21,7 @@ from syrupy.assertion import SnapshotAssertion from ossie import OssieDataType, OssieDimension +from ossie_dbt.converter_issues import ConverterIssue, ConverterIssueType from ossie_dbt.msi_to_ossie import MSIToOssieConverter from ossie_dbt.ossie_to_msi import OssieToMSIConverter from metricflow_semantic_interfaces.implementations.elements.measure import ( @@ -355,20 +356,25 @@ def _customers_and_orders() -> list: ] @pytest.mark.parametrize("expression", ["COUNT(*)", "COUNT(1)"]) - def test_bare_row_count_with_multiple_datasets_is_ambiguous(self, expression: str) -> None: + def test_bare_row_count_with_multiple_datasets_is_dropped_with_a_warning(self, expression: str) -> None: doc = _ossie_doc(datasets=self._customers_and_orders(), metrics=[_ossie_metric("order_count", expression)]) + result = OssieToMSIConverter().convert(doc) - with pytest.raises(ValueError, match="ambiguous with 2 datasets"): - OssieToMSIConverter().convert(doc) + assert result.output.metrics == [] + assert result.issues == [ConverterIssue(ConverterIssueType.ROW_COUNT_METRIC_DROPPED, "order_count")] - def test_bare_count_star_in_ratio_with_multiple_datasets_is_ambiguous(self) -> None: + def test_ratio_with_a_bare_count_star_is_dropped_as_a_whole(self) -> None: doc = _ossie_doc( datasets=self._customers_and_orders(), - metrics=[_ossie_metric("avg_order_value", "(SUM(amount)) / (COUNT(*))")], + metrics=[ + _ossie_metric("revenue", "SUM(orders.amount)"), + _ossie_metric("avg_order_value", "(SUM(orders.amount)) / (COUNT(*))"), + ], ) + result = OssieToMSIConverter().convert(doc) - with pytest.raises(ValueError, match="ambiguous"): - OssieToMSIConverter().convert(doc) + assert [m.name for m in result.output.metrics] == ["revenue"] + assert result.issues == [ConverterIssue(ConverterIssueType.ROW_COUNT_METRIC_DROPPED, "avg_order_value")] def test_qualified_count_star_in_ratio_binds_both_sides_to_the_same_dataset(self) -> None: doc = _ossie_doc( @@ -394,16 +400,17 @@ def test_schema_qualified_count_star_matches_dataset_by_last_segment(self) -> No assert params is not None assert params.semantic_model == "orders" - def test_count_star_of_unknown_dataset_is_rejected(self) -> None: + def test_count_star_of_unknown_dataset_is_dropped_with_a_warning(self) -> None: doc = _ossie_doc( datasets=self._customers_and_orders(), metrics=[_ossie_metric("order_count", "COUNT(nope.*)")], ) + result = OssieToMSIConverter().convert(doc) - with pytest.raises(ValueError, match="does not match exactly one dataset"): - OssieToMSIConverter().convert(doc) + assert result.output.metrics == [] + assert [i.element_name for i in result.issues] == ["order_count"] - def test_count_star_matching_several_datasets_by_last_segment_is_rejected(self) -> None: + def test_count_star_matching_several_datasets_by_last_segment_is_dropped_with_a_warning(self) -> None: doc = _ossie_doc( datasets=[ _ossie_dataset("a.orders", fields=[_ossie_field("order_id")]), @@ -411,9 +418,10 @@ def test_count_star_matching_several_datasets_by_last_segment_is_rejected(self) ], metrics=[_ossie_metric("order_count", "COUNT(orders.*)")], ) + result = OssieToMSIConverter().convert(doc) - with pytest.raises(ValueError, match="does not match exactly one dataset"): - OssieToMSIConverter().convert(doc) + assert result.output.metrics == [] + assert [i.element_name for i in result.issues] == ["order_count"] @pytest.mark.parametrize( "expression",