From 94085aff55a8ea15b68669b066afa9c946e0b912 Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Sat, 19 Sep 2026 23:35:57 +0200 Subject: [PATCH 1/4] 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/4] 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/4] 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", From ab8513fb3ff983314f20049809dfe7496dcbbca3 Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Tue, 22 Sep 2026 20:39:14 +0200 Subject: [PATCH 4/4] fix(dbt): treat any non-null constant as a row count, drop instance state --- .../dbt/src/ossie_dbt/expression_utils.py | 25 ++++++++++----- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 31 +++++++++++++------ converters/dbt/tests/test_msi_to_ossie.py | 23 ++++++++++++++ converters/dbt/tests/test_ossie_to_msi.py | 8 ++--- 4 files changed, 66 insertions(+), 21 deletions(-) diff --git a/converters/dbt/src/ossie_dbt/expression_utils.py b/converters/dbt/src/ossie_dbt/expression_utils.py index 9d82e38a..06c298ab 100644 --- a/converters/dbt/src/ossie_dbt/expression_utils.py +++ b/converters/dbt/src/ossie_dbt/expression_utils.py @@ -39,9 +39,19 @@ 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 _is_row_count_argument(node: exp.Expression) -> bool: + """Return True for ``*`` (bare or qualified) and for any non-null constant. + + None of these can ever be NULL, so ``COUNT()`` of one counts every row. A string literal is left + alone, since ``COUNT('x')`` is not a row-count idiom anyone writes on purpose. + """ + if isinstance(node, exp.Star) or (isinstance(node, exp.Column) and isinstance(node.this, exp.Star)): + return True + if isinstance(node, exp.Boolean): + return True + if isinstance(node, exp.Literal) and not node.is_string: + return True + return False def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float], bool]]: @@ -50,8 +60,9 @@ 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(*)`` and ``COUNT(dataset.*)`` return - ``ROW_COUNT_EXPR`` instead of a column name; ``COUNT(DISTINCT *)`` and multi-argument ``COUNT`` return ``None``. + The returned column name has any dataset qualifier stripped. ``COUNT`` of ``*`` or of any non-null constant + (``COUNT(1)``, ``COUNT(TRUE)``, ...) returns ``ROW_COUNT_EXPR`` instead of a column name; + ``COUNT(DISTINCT ...)`` of one of those, and multi-argument ``COUNT``, return ``None``. """ try: tree = sqlglot.parse_one(expression.strip()) @@ -68,8 +79,8 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O 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 + if _is_row_count_argument(argument): + # COUNT(*), COUNT(1), COUNT(TRUE), ... → count all rows; COUNT(DISTINCT ...) of one 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 diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index e1bd27cb..cf1684c9 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -82,18 +82,21 @@ class AmbiguousDerivedReferenceError(Exception): class MSIToOssieConverter: - """Converts an MSI SemanticManifest into an Ossie Document.""" + """Converts an MSI SemanticManifest into an Ossie Document. + + Holds no per-call state on the instance, so one instance may be reused, including concurrently + from multiple threads, across any number of ``convert()`` calls. + """ 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( + row_count_metrics: FrozenSet[str] = frozenset( metric.name for metric in manifest.metrics if metric.type is MetricType.SIMPLE @@ -130,7 +133,7 @@ def convert( ConverterIssue(issue_type=ConverterIssueType.CUMULATIVE_SEMANTICS_LOSS, element_name=metric.name) ) try: - expr = self._resolve_metric_expression(metric, metric_index, expression_cache) + expr = self._resolve_metric_expression(metric, metric_index, expression_cache, row_count_metrics) except AmbiguousDerivedReferenceError: # Every other unsupported shape drops one metric and records an issue; # an ambiguous reference is no reason to fail the whole conversion. @@ -244,6 +247,7 @@ def _resolve_metric_expression( metric: Metric, metric_index: Dict[str, Metric], cache: Dict[Tuple[str, Optional[str]], str], + row_count_metrics: FrozenSet[str], parent_filter: Optional[str] = None, ) -> str: """Recursively resolve a metric to a fully-inlined SQL expression string.""" @@ -255,13 +259,13 @@ def _resolve_metric_expression( return cache[cache_key] if metric.type is MetricType.SIMPLE: - expr = self._resolve_simple(metric, combined_filter) + expr = self._resolve_simple(metric, row_count_metrics, combined_filter) elif metric.type is MetricType.CUMULATIVE: - expr = self._resolve_cumulative(metric, metric_index, cache, combined_filter) + expr = self._resolve_cumulative(metric, metric_index, cache, row_count_metrics, combined_filter) elif metric.type is MetricType.RATIO: - expr = self._resolve_ratio(metric, metric_index, cache, combined_filter) + expr = self._resolve_ratio(metric, metric_index, cache, row_count_metrics, combined_filter) elif metric.type is MetricType.DERIVED: - expr = self._resolve_derived(metric, metric_index, cache, combined_filter) + expr = self._resolve_derived(metric, metric_index, cache, row_count_metrics, combined_filter) elif metric.type is MetricType.CONVERSION: # CONVERSION metrics are skipped in convert(); this branch should never be reached. raise RuntimeError(f"Unexpected CONVERSION metric in expression resolver: metric_name={metric.name!r}") @@ -274,6 +278,7 @@ def _resolve_metric_expression( def _resolve_simple( self, metric: Metric, + row_count_metrics: FrozenSet[str], filter_sql: Optional[str] = None, ) -> str: """Resolve a SIMPLE metric using metric_aggregation_params (always set after transformation).""" @@ -283,7 +288,7 @@ def _resolve_simple( 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: + if metric.name in 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) @@ -306,6 +311,7 @@ def _resolve_cumulative( metric: Metric, metric_index: Dict[str, Metric], cache: Dict[Tuple[str, Optional[str]], str], + row_count_metrics: FrozenSet[str], filter_sql: Optional[str] = None, ) -> str: """Resolve a CUMULATIVE metric to its base aggregation expression. @@ -323,6 +329,7 @@ def _resolve_cumulative( self._lookup_metric(metric_index, sub_input.name, f"CUMULATIVE metric '{metric.name}'"), metric_index, cache, + row_count_metrics, sub_filter, ) @@ -331,6 +338,7 @@ def _resolve_ratio( metric: Metric, metric_index: Dict[str, Metric], cache: Dict[Tuple[str, Optional[str]], str], + row_count_metrics: FrozenSet[str], filter_sql: Optional[str] = None, ) -> str: """Resolve a RATIO metric as (numerator) / (denominator), both fully inlined.""" @@ -346,12 +354,14 @@ def _resolve_ratio( self._lookup_metric(metric_index, num_input.name, f"RATIO metric '{metric.name}' numerator"), metric_index, cache, + row_count_metrics, num_filter, ) den_expr = self._resolve_metric_expression( self._lookup_metric(metric_index, den_input.name, f"RATIO metric '{metric.name}' denominator"), metric_index, cache, + row_count_metrics, den_filter, ) return f"({num_expr}) / ({den_expr})" @@ -361,6 +371,7 @@ def _resolve_derived( metric: Metric, metric_index: Dict[str, Metric], cache: Dict[Tuple[str, Optional[str]], str], + row_count_metrics: FrozenSet[str], filter_sql: Optional[str] = None, ) -> str: """Resolve a DERIVED metric by substituting each input metric's expression into the expr string. @@ -390,7 +401,7 @@ def _resolve_derived( ref = input_metric.alias if input_metric.alias else input_metric.name dep_metric = self._lookup_metric(metric_index, input_metric.name, f"DERIVED metric '{metric.name}'") input_filter = _merge_filter_sqls(filter_sql, _collect_filter_sql(input_metric.filter)) - resolved = self._resolve_metric_expression(dep_metric, metric_index, cache, input_filter) + resolved = self._resolve_metric_expression(dep_metric, metric_index, cache, row_count_metrics, input_filter) if dep_metric.type in (MetricType.DERIVED, MetricType.RATIO): resolved = f"({resolved})" distinct = resolutions.setdefault(ref, []) diff --git a/converters/dbt/tests/test_msi_to_ossie.py b/converters/dbt/tests/test_msi_to_ossie.py index a7eb2960..f0c58868 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -636,6 +636,29 @@ def test_filtered_count_of_all_rows_keeps_the_filter(self) -> None: _ossie_metrics(result)[0].expression.dialects[0].expression == "SUM(CASE WHEN status = 'paid' THEN 1 END)" ) + def test_a_reused_converter_instance_does_not_leak_state_between_calls(self) -> None: + """Sequential reuse of one converter instance must not mix up which metrics are row counts. + + This does not reproduce a bug: sequential reuse worked even when that state lived on ``self``, + since each ``convert()`` call overwrote it before resolving any metric. Only two calls actually + overlapping (e.g. on separate threads) could corrupt it, which is why the state is no longer + stored on the instance at all: there is nothing left for a second call to overwrite. + """ + orders = semantic_model_with_guaranteed_meta(name="orders") + row_count = _manifest( + semantic_models=[orders], metrics=[_metric_with_agg("order_count", AggregationType.COUNT, "1", "orders")] + ) + plain_sum = _manifest( + semantic_models=[orders], metrics=[_metric_with_agg("total", AggregationType.SUM, "1", "orders")] + ) + converter = MSIToOssieConverter() + + converter.convert(row_count) + converter.convert(plain_sum) + result = converter.convert(row_count).output + + assert _ossie_metrics(result)[0].expression.dialects[0].expression == "COUNT(orders.*)" + # --- 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 056dbd63..0e6d4487 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -320,8 +320,8 @@ def test_count_distinct_expression(self) -> None: sm = result.semantic_models[0] assert len(sm.measures) == 0 - @pytest.mark.parametrize("expression", ["COUNT(*)", "COUNT(orders.*)"]) - def test_count_star_uses_constant_expr(self, expression: str) -> None: + @pytest.mark.parametrize("expression", ["COUNT(*)", "COUNT(orders.*)", "COUNT(2)", "COUNT(TRUE)", "COUNT(0)"]) + def test_count_of_a_constant_normalizes_to_row_count_expr(self, expression: str) -> None: doc = _ossie_doc( datasets=[_ossie_dataset("orders", fields=[_ossie_field("order_id")])], metrics=[_ossie_metric("order_count", expression)], @@ -355,7 +355,7 @@ def _customers_and_orders() -> list: _ossie_dataset("orders", fields=[_ossie_field("order_id"), _ossie_field("amount")]), ] - @pytest.mark.parametrize("expression", ["COUNT(*)", "COUNT(1)"]) + @pytest.mark.parametrize("expression", ["COUNT(*)", "COUNT(1)", "COUNT(2)", "COUNT(0)", "COUNT(TRUE)"]) 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) @@ -425,7 +425,7 @@ def test_count_star_matching_several_datasets_by_last_segment_is_dropped_with_a_ @pytest.mark.parametrize( "expression", - ["COUNT(DISTINCT *)", "COUNT(orders.*, amount)", "COUNT(db.orders, *)"], + ["COUNT(DISTINCT *)", "COUNT(DISTINCT 1)", "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(