From 55baeced91301f6b604e10beef767e69786d68f7 Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Thu, 1 Oct 2026 23:59:20 +0200 Subject: [PATCH 1/4] fix(dbt): stop emitting COUNT(.*), SUM(1) is the row count 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(.*) 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(.*) isn't portable or spec-conformant anyway. Removed it. --- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 44 ++++++----------- converters/dbt/tests/test_msi_to_ossie.py | 52 +++++++++----------- converters/dbt/tests/test_ossie_to_msi.py | 27 +++++----- 3 files changed, 50 insertions(+), 73 deletions(-) diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index 9969dc38..aeee61bd 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, FrozenSet, List, Optional, Sequence, Tuple +from typing import Dict, List, Optional, Sequence, Tuple from ossie import ( OssieDataset, @@ -33,7 +33,6 @@ 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 @@ -94,16 +93,6 @@ def __init__(self, dialect: OssieDialect = OssieDialect.ANSI_SQL) -> None: 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(.*). - row_count_metrics: FrozenSet[str] = 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] = [] @@ -137,7 +126,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, row_count_metrics) + expr = self._resolve_metric_expression(metric, metric_index, expression_cache) except AmbiguousDerivedReferenceError: # Every other unsupported shape drops one metric and records an issue; # an ambiguous reference is no reason to fail the whole conversion. @@ -251,7 +240,6 @@ 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.""" @@ -263,13 +251,13 @@ def _resolve_metric_expression( return cache[cache_key] if metric.type is MetricType.SIMPLE: - expr = self._resolve_simple(metric, row_count_metrics, combined_filter) + expr = self._resolve_simple(metric, combined_filter) elif metric.type is MetricType.CUMULATIVE: - expr = self._resolve_cumulative(metric, metric_index, cache, row_count_metrics, combined_filter) + expr = self._resolve_cumulative(metric, metric_index, cache, combined_filter) elif metric.type is MetricType.RATIO: - expr = self._resolve_ratio(metric, metric_index, cache, row_count_metrics, combined_filter) + expr = self._resolve_ratio(metric, metric_index, cache, combined_filter) elif metric.type is MetricType.DERIVED: - expr = self._resolve_derived(metric, metric_index, cache, row_count_metrics, combined_filter) + expr = self._resolve_derived(metric, metric_index, cache, 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}") @@ -282,18 +270,20 @@ 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).""" + """Resolve a SIMPLE metric using metric_aggregation_params (always set after transformation). + + 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(.*)``, which several engines reject or interpret + differently). + """ agg_params_obj = metric.type_params.metric_aggregation_params if agg_params_obj is None: 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 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) @@ -315,7 +305,6 @@ 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. @@ -333,7 +322,6 @@ def _resolve_cumulative( self._lookup_metric(metric_index, sub_input.name, f"CUMULATIVE metric '{metric.name}'"), metric_index, cache, - row_count_metrics, sub_filter, ) @@ -342,7 +330,6 @@ 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.""" @@ -358,14 +345,12 @@ 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})" @@ -375,7 +360,6 @@ 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. @@ -405,7 +389,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, row_count_metrics, input_filter) + resolved = self._resolve_metric_expression(dep_metric, metric_index, cache, 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 d489102c..5e86f646 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -607,17 +607,34 @@ 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: + @pytest.mark.parametrize("agg", [AggregationType.COUNT, AggregationType.SUM]) + def test_a_row_count_always_comes_back_as_portable_sum_1(self, agg: AggregationType) -> None: + """COUNT(*) and SUM(1) mean the same thing in SQL, so both convert the same way. + + Emitting ``COUNT(.*)`` to keep the dataset through a round trip was tried and reverted: + it is not in the Ossie expression spec, several engines reject or misinterpret it, and the + sibling converters do not recognize it as a row count either. ``SUM(1)`` is what every engine + and converter agrees on, even though the dataset cannot be recovered on this leg of a round trip. + """ 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") + metric = _metric_with_agg("order_count", agg, "1", "orders") result = MSIToOssieConverter().convert(_manifest(semantic_models=[customers, orders], metrics=[metric])).output - assert _ossie_metrics(result)[0].expression.dialects[0].expression == "COUNT(orders.*)" + assert _ossie_metrics(result)[0].expression.dialects[0].expression == "SUM(1)" + + def test_legacy_measure_based_count_also_converts(self) -> None: + """A SIMPLE metric over a legacy Measure(agg=count, expr=1) is a row count too, not just the - 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") + newer metric_aggregation_params shape. It converts correctly with no special-casing, because + MetricFlow's own transform normalizes both shapes to the same agg=SUM, expr='1' before this + converter ever sees the metric. + """ + orders = semantic_model_with_guaranteed_meta( + name="orders", + measures=[_measure("order_count_measure", agg=AggregationType.COUNT, expr="1")], + ) + metric = _simple_metric("order_count", measure_name="order_count_measure") result = MSIToOssieConverter().convert(_manifest(semantic_models=[orders], metrics=[metric])).output assert _ossie_metrics(result)[0].expression.dialects[0].expression == "SUM(1)" @@ -631,29 +648,6 @@ 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 ebe6a637..7cf73a47 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -758,8 +758,17 @@ 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.""" + def test_count_star_round_trips_to_a_portable_sum_one(self) -> None: + """COUNT(orders.*) survives one round trip as the portable SUM(1), not the original invalid expr '*'. + + The dataset is not recovered on this leg: MSI has already collapsed the metric to agg=SUM, expr='1' + by the time it reaches MSIToOssieConverter (MetricFlow's own transform runs first), and SUM(1) has + no qualifier to recover a dataset from. Emitting COUNT(.*) instead, to carry the dataset + through, was tried and reverted: it is outside the Ossie expression spec, several engines reject or + misinterpret it, and no sibling converter recognizes it as a row count. SUM(1) is what every engine + agrees on. Converting SUM(1) forward again, with more than one dataset, still silently picks the + first dataset rather than refusing like a bare COUNT(*) does; closing that gap is tracked separately. + """ original = _ossie_doc( datasets=[ _ossie_dataset("customers", fields=[_ossie_field("customer_id")]), @@ -775,18 +784,8 @@ def test_count_star_keeps_its_dataset_across_round_trip(self) -> None: 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", - ) + assert expressions["order_count"] == "SUM(1)" + assert "SUM(1)" in expressions["avg_order_value"] def test_discrete_percentile_survives_round_trip(self) -> None: """A PERCENTILE_DISC metric keeps use_discrete_percentile through MSI -> Ossie -> MSI.""" From c39e158e8d5809bdacb22a826216828baf8d6873 Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Fri, 2 Oct 2026 00:11:58 +0200 Subject: [PATCH 2/4] fix(dbt): drop COUNT(DISTINCT ) instead of a nested fallback 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. --- converters/dbt/src/ossie_dbt/cli.py | 6 +++- .../dbt/src/ossie_dbt/expression_utils.py | 22 ++++++++++++ converters/dbt/src/ossie_dbt/ossie_to_msi.py | 14 +++++++- converters/dbt/tests/test_ossie_to_msi.py | 34 ++++++++++++++++--- 4 files changed, 70 insertions(+), 6 deletions(-) diff --git a/converters/dbt/src/ossie_dbt/cli.py b/converters/dbt/src/ossie_dbt/cli.py index ec910956..6055be67 100644 --- a/converters/dbt/src/ossie_dbt/cli.py +++ b/converters/dbt/src/ossie_dbt/cli.py @@ -41,7 +41,11 @@ 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(.*)", + ConverterIssueType.ROW_COUNT_METRIC_DROPPED: ( + "a row-count expression (COUNT(*) or similar) either did not identify exactly one dataset to " + "count rows of (qualify it as COUNT(.*)), or has no sensible translation at all, such " + "as COUNT(DISTINCT *)" + ), ConverterIssueType.AMBIGUOUS_REFERENCE_METRIC_DROPPED: ( "an input metric is listed more than once under one reference with differing filters, " "so the expression reference is ambiguous; give each occurrence a distinct alias" diff --git a/converters/dbt/src/ossie_dbt/expression_utils.py b/converters/dbt/src/ossie_dbt/expression_utils.py index 06c298ab..ed29b61a 100644 --- a/converters/dbt/src/ossie_dbt/expression_utils.py +++ b/converters/dbt/src/ossie_dbt/expression_utils.py @@ -54,6 +54,28 @@ def _is_row_count_argument(node: exp.Expression) -> bool: return False +def _is_unsupported_distinct_row_count(expression: str) -> bool: + """Return True for ``COUNT(DISTINCT )``, e.g. ``COUNT(DISTINCT *)`` or ``COUNT(DISTINCT 1)``. + + These parse and run as SQL, but counting distinct values of ``*`` or a constant is not a sensible + aggregation for a semantic layer: it answers whether any row exists (0 or 1), not a meaningful total, + and ``_extract_agg_info`` already declines to treat it as ``COUNT`` or ``COUNT_DISTINCT`` of a column. + The caller should drop the metric with an issue rather than fall back to a raw expression, which would + wrap this inside another aggregate (``SUM(COUNT(DISTINCT ...))``, not valid for MetricFlow to run) and + guess a dataset the way a row count must not. + """ + try: + tree = sqlglot.parse_one(expression.strip()) + except sqlglot.errors.ParseError: + return False + if not isinstance(tree, exp.Count) or tree.args.get("expressions"): + return False + argument = tree.this + if not isinstance(argument, exp.Distinct) or len(argument.expressions) != 1: + return False + return _is_row_count_argument(argument.expressions[0]) + + def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float], bool]]: """Parse a SQL aggregation expression using sqlglot. diff --git a/converters/dbt/src/ossie_dbt/ossie_to_msi.py b/converters/dbt/src/ossie_dbt/ossie_to_msi.py index c6bad108..bf79ad93 100644 --- a/converters/dbt/src/ossie_dbt/ossie_to_msi.py +++ b/converters/dbt/src/ossie_dbt/ossie_to_msi.py @@ -31,6 +31,7 @@ ROW_COUNT_EXPR, _extract_agg_info, _get_dataset_qualifier, + _is_unsupported_distinct_row_count, _strip_qualifier, _try_parse_ratio, ) @@ -76,7 +77,12 @@ class _KeySets: class _UnresolvedRowCountDataset(Exception): - """A ``COUNT(*)`` that does not identify exactly one dataset to count the rows of.""" + """A row-count expression that cannot be safely converted into a metric. + + Either it does not identify exactly one dataset to count the rows of (a bare ``COUNT(*)`` with + more than one dataset, or a qualifier matching none or several), or it is a form with no sensible + translation at all, such as ``COUNT(DISTINCT *)``. + """ class OssieToMSIConverter: @@ -360,6 +366,12 @@ def _convert_metric( ) return [*num_metrics, *den_metrics, ratio_metric] + # 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") + # --- Fallback: complex expression that can't be decomposed --- # Store the raw expression in `expr` with a best-guess aggregation type. # The caller is responsible for reviewing and correcting these metrics. diff --git a/converters/dbt/tests/test_ossie_to_msi.py b/converters/dbt/tests/test_ossie_to_msi.py index 7cf73a47..90d0d75e 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -385,6 +385,19 @@ def test_ratio_with_a_bare_count_star_is_dropped_as_a_whole(self) -> None: assert [m.name for m in result.output.metrics] == ["revenue"] assert result.issues == [ConverterIssue(ConverterIssueType.ROW_COUNT_METRIC_DROPPED, "avg_order_value")] + def test_ratio_with_a_distinct_row_count_is_dropped_as_a_whole(self) -> None: + doc = _ossie_doc( + datasets=self._customers_and_orders(), + metrics=[ + _ossie_metric("revenue", "SUM(orders.amount)"), + _ossie_metric("avg_order_value", "(SUM(orders.amount)) / (COUNT(DISTINCT *))"), + ], + ) + result = 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( datasets=self._customers_and_orders(), @@ -432,10 +445,7 @@ def test_count_star_matching_several_datasets_by_last_segment_is_dropped_with_a_ assert result.output.metrics == [] assert [i.element_name for i in result.issues] == ["order_count"] - @pytest.mark.parametrize( - "expression", - ["COUNT(DISTINCT *)", "COUNT(DISTINCT 1)", "COUNT(orders.*, amount)", "COUNT(db.orders, *)"], - ) + @pytest.mark.parametrize("expression", ["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")])], @@ -445,6 +455,22 @@ def test_unsupported_count_star_forms_fall_back_to_the_raw_expression(self, expr assert result.metrics[0].type_params.expr == expression + @pytest.mark.parametrize("expression", ["COUNT(DISTINCT *)", "COUNT(DISTINCT 1)", "COUNT(DISTINCT orders.*)"]) + def test_distinct_of_a_row_count_is_dropped_with_a_warning(self, expression: str) -> None: + """COUNT(DISTINCT *) and friends are valid SQL but answer 'does any row exist' (0 or 1), not a + + meaningful total. Falling back to a raw SUM of it would be a nested aggregate MetricFlow cannot + run, bound to a guessed dataset; dropped instead, the same as an unresolvable COUNT(*). + """ + 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) + + assert result.output.metrics == [] + assert [i.element_name for i in result.issues] == ["odd_count"] + def test_ratio_expression_produces_ratio_metric(self) -> None: doc = _ossie_doc( datasets=[ From fa59308ef0df6081f66d8ac514e7678d6d790e77 Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Fri, 2 Oct 2026 15:56:34 +0200 Subject: [PATCH 3/4] fix(dbt): refuse constant aggregates on ambiguous datasets, record the MSI loss - ossie-to-msi: SUM() 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 ) is dropped wherever it appears in an expression, including redundant parens and wrapped forms. --- converters/dbt/README.md | 3 + converters/dbt/src/ossie_dbt/cli.py | 10 ++- .../dbt/src/ossie_dbt/converter_issues.py | 1 + .../dbt/src/ossie_dbt/expression_utils.py | 65 +++++++++++++------ converters/dbt/src/ossie_dbt/msi_to_ossie.py | 25 ++++++- converters/dbt/src/ossie_dbt/ossie_to_msi.py | 11 ++-- converters/dbt/tests/test_msi_to_ossie.py | 38 ++++++++++- converters/dbt/tests/test_ossie_to_msi.py | 64 ++++++++++++++++-- 8 files changed, 180 insertions(+), 37 deletions(-) diff --git a/converters/dbt/README.md b/converters/dbt/README.md index fdeffb02..dd7ce9e7 100644 --- a/converters/dbt/README.md +++ b/converters/dbt/README.md @@ -125,12 +125,15 @@ manifest_json = result.output.model_dump_json(by_alias=True, exclude_none=True, | `NATURAL_ENTITY_DROPPED` | Ossie has no natural-key entity type | | `CUMULATIVE_SEMANTICS_LOSS` | Window/grain semantics cannot be expressed in an Ossie expression string; the base aggregation is preserved | | `AMBIGUOUS_REFERENCE_METRIC_DROPPED` | An input metric is listed more than once under one reference with differing filters, so the expression reference is ambiguous; give each occurrence a distinct alias | +| `CONSTANT_METRIC_SEMANTIC_MODEL_LOSS` | A metric over a constant, such as a row count (`SUM(1)`), has no column to carry its semantic model; recorded when the manifest has more than one, since converting it back will refuse it | **Ossie → MSI** reconstructs a best-effort MSI manifest from Ossie's simpler schema. Nothing is dropped for supported inputs, but Ossie carries less structural information than MSI, so the converter makes the following choices: - 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(*)`, or a qualifier that matches no dataset, is skipped with a `ROW_COUNT_METRIC_DROPPED` warning +- `SUM()` (e.g. `SUM(1)`) keeps its constant, but has no column to place it in a dataset: with more than one dataset it is skipped with `ROW_COUNT_METRIC_DROPPED` +- `COUNT(DISTINCT *)`, `COUNT(DISTINCT 1)` and the like, anywhere in an expression, are skipped with `ROW_COUNT_METRIC_DROPPED`: they count whether any row exists, not how many - `(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 6055be67..e00df91d 100644 --- a/converters/dbt/src/ossie_dbt/cli.py +++ b/converters/dbt/src/ossie_dbt/cli.py @@ -41,10 +41,14 @@ 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.CONSTANT_METRIC_SEMANTIC_MODEL_LOSS: ( + "its expression is a constant such as SUM(1), which has no column to say which semantic model it " + "counts; with more than one dataset, converting it back to dbt will refuse it" + ), ConverterIssueType.ROW_COUNT_METRIC_DROPPED: ( - "a row-count expression (COUNT(*) or similar) either did not identify exactly one dataset to " - "count rows of (qualify it as COUNT(.*)), or has no sensible translation at all, such " - "as COUNT(DISTINCT *)" + "a row count or constant aggregate (COUNT(*), SUM(1), ...) did not identify exactly one dataset " + "(qualify a COUNT(*) as COUNT(.*)), or has no sensible translation at all, such as " + "COUNT(DISTINCT *)" ), ConverterIssueType.AMBIGUOUS_REFERENCE_METRIC_DROPPED: ( "an input metric is listed more than once under one reference with differing filters, " diff --git a/converters/dbt/src/ossie_dbt/converter_issues.py b/converters/dbt/src/ossie_dbt/converter_issues.py index 8263c090..39922b61 100644 --- a/converters/dbt/src/ossie_dbt/converter_issues.py +++ b/converters/dbt/src/ossie_dbt/converter_issues.py @@ -29,6 +29,7 @@ class ConverterIssueType(Enum): CUMULATIVE_SEMANTICS_LOSS = "CUMULATIVE_SEMANTICS_LOSS" ROW_COUNT_METRIC_DROPPED = "ROW_COUNT_METRIC_DROPPED" AMBIGUOUS_REFERENCE_METRIC_DROPPED = "AMBIGUOUS_REFERENCE_METRIC_DROPPED" + CONSTANT_METRIC_SEMANTIC_MODEL_LOSS = "CONSTANT_METRIC_SEMANTIC_MODEL_LOSS" @dataclass(frozen=True) diff --git a/converters/dbt/src/ossie_dbt/expression_utils.py b/converters/dbt/src/ossie_dbt/expression_utils.py index ed29b61a..a5e29a58 100644 --- a/converters/dbt/src/ossie_dbt/expression_utils.py +++ b/converters/dbt/src/ossie_dbt/expression_utils.py @@ -54,26 +54,42 @@ def _is_row_count_argument(node: exp.Expression) -> bool: return False -def _is_unsupported_distinct_row_count(expression: str) -> bool: - """Return True for ``COUNT(DISTINCT )``, e.g. ``COUNT(DISTINCT *)`` or ``COUNT(DISTINCT 1)``. - - These parse and run as SQL, but counting distinct values of ``*`` or a constant is not a sensible - aggregation for a semantic layer: it answers whether any row exists (0 or 1), not a meaningful total, - and ``_extract_agg_info`` already declines to treat it as ``COUNT`` or ``COUNT_DISTINCT`` of a column. - The caller should drop the metric with an issue rather than fall back to a raw expression, which would - wrap this inside another aggregate (``SUM(COUNT(DISTINCT ...))``, not valid for MetricFlow to run) and - guess a dataset the way a row count must not. +def _is_constant_expr(expr: str) -> bool: + """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: + return False + return isinstance(node, exp.Boolean) or (isinstance(node, exp.Literal) and not node.is_string) + + +def _contains_distinct_row_count(expression: str) -> bool: + """Return True if ``expression`` contains ``COUNT(DISTINCT )`` anywhere in its tree. + + ``COUNT(DISTINCT *)`` / ``COUNT(DISTINCT 1)`` and friends parse and run as SQL, but counting distinct + values of ``*`` or a constant is not a sensible aggregation for a semantic layer: it answers whether + any row exists (0 or 1), not a meaningful total. The caller should drop the metric with an issue + rather than fall back to a raw expression, which would wrap this inside another aggregate + (``SUM(COUNT(DISTINCT ...))``, not valid for MetricFlow to run) and guess a dataset the way a row + count must not. + + Searches the whole tree, not just the top node, so a wrapped or combined form such as + ``(COUNT(DISTINCT *))``, ``COUNT(DISTINCT *) * 100`` or ``COALESCE(COUNT(DISTINCT 1), 0)`` is still + caught, not only a bare ``COUNT(DISTINCT *)`` as the entire expression. The DISTINCT operand is + unnested before the check, so ``COUNT(DISTINCT (*))`` is caught the same way as ``COUNT(DISTINCT *)``. """ try: tree = sqlglot.parse_one(expression.strip()) except sqlglot.errors.ParseError: return False - if not isinstance(tree, exp.Count) or tree.args.get("expressions"): - return False - argument = tree.this - if not isinstance(argument, exp.Distinct) or len(argument.expressions) != 1: - return False - return _is_row_count_argument(argument.expressions[0]) + for count in tree.find_all(exp.Count): + argument = count.this + if count.args.get("expressions") or not isinstance(argument, exp.Distinct): + continue + operands = argument.expressions + if len(operands) == 1 and _is_row_count_argument(operands[0].unnest()): + return True + return False def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float], bool]]: @@ -82,9 +98,10 @@ 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`` 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``. + 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; + ``SUM`` of a constant returns the constant itself (``SUM(2)`` → ``'2'``). ``COUNT(DISTINCT ...)`` of a + row-count argument, and multi-argument ``COUNT``, return ``None``. """ try: tree = sqlglot.parse_one(expression.strip()) @@ -101,8 +118,9 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O if len(operands) != 1: return None argument, distinct = operands[0], True - if _is_row_count_argument(argument): - # COUNT(*), COUNT(1), COUNT(TRUE), ... → count all rows; COUNT(DISTINCT ...) of one is not valid SQL + if _is_row_count_argument(argument.unnest()): + # COUNT(*), COUNT(1), COUNT(TRUE), ... → count all rows; COUNT(DISTINCT ...) of one is not valid SQL. + # Unnested so a redundant paren, e.g. COUNT(DISTINCT (*)), is still recognised. 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 @@ -121,8 +139,13 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O return AggregationType.SUM_BOOLEAN, ifs[0].this.sql(), None, False return None - # SUM(col) + # SUM(col), or SUM(). A constant keeps its own value (SUM(2) is twice the row count, + # not SUM(1)); the caller uses _is_constant_expr to send it through the same dataset check as + # COUNT(*), since a constant has no column to place it in a dataset. if isinstance(tree, exp.Sum): + argument = tree.this.unnest() + if _is_constant_expr(argument.sql()): + return AggregationType.SUM, argument.sql(), None, False return AggregationType.SUM, _col_name(tree.this), None, False if isinstance(tree, exp.Avg): diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index aeee61bd..f89b5c74 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -33,6 +33,7 @@ OssieRelationship, ) from ossie_dbt.converter_issues import ConverterIssue, ConverterIssueType, ConverterResult +from ossie_dbt.expression_utils import _is_constant_expr from ossie_dbt.filter_utils import _collect_filter_sql, _merge_filter_sqls from metricflow_semantic_interfaces.enum_extension import assert_values_exhausted @@ -144,6 +145,16 @@ def convert( description=metric.description, ) ) + if len(manifest.semantic_models) > 1 and self._aggregates_a_constant(metric): + # SUM(1) and the like carry no column, so the semantic model the metric belonged to + # cannot be written into the Ossie expression. Converting it back refuses rather than + # guesses (ROW_COUNT_METRIC_DROPPED); record the loss here so it is not silent. + issues.append( + ConverterIssue( + issue_type=ConverterIssueType.CONSTANT_METRIC_SEMANTIC_MODEL_LOSS, + element_name=metric.name, + ) + ) return ConverterResult( output=OssieDocument( @@ -275,9 +286,10 @@ def _resolve_simple( """Resolve a SIMPLE metric using metric_aggregation_params (always set after transformation). 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(.*)``, which several engines reject or interpret - differently). + through the generic path below. That matches ``COUNT(*)`` on any non-empty input (over zero + rows ``SUM(1)`` is NULL where ``COUNT(*)`` is 0, as with MetricFlow's own COUNT → SUM rewrite) + and is portable across engines, unlike ``COUNT(.*)``, which several engines reject or + interpret differently. """ agg_params_obj = metric.type_params.metric_aggregation_params if agg_params_obj is None: @@ -288,6 +300,13 @@ def _resolve_simple( 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) + @staticmethod + def _aggregates_a_constant(metric: Metric) -> bool: + """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) + @staticmethod def _qualify_col(col: str, semantic_model: str) -> str: """Qualify col with semantic_model if it is an unqualified identifier or a COUNT-converted expr.""" diff --git a/converters/dbt/src/ossie_dbt/ossie_to_msi.py b/converters/dbt/src/ossie_dbt/ossie_to_msi.py index bf79ad93..bcb69a6e 100644 --- a/converters/dbt/src/ossie_dbt/ossie_to_msi.py +++ b/converters/dbt/src/ossie_dbt/ossie_to_msi.py @@ -30,8 +30,9 @@ from ossie_dbt.expression_utils import ( ROW_COUNT_EXPR, _extract_agg_info, + _contains_distinct_row_count, _get_dataset_qualifier, - _is_unsupported_distinct_row_count, + _is_constant_expr, _strip_qualifier, _try_parse_ratio, ) @@ -310,7 +311,8 @@ def _convert_metric( agg_result = _extract_agg_info(expr_str) if agg_result is not None: agg, col, percentile, use_discrete = agg_result - if agg is AggregationType.COUNT and col == ROW_COUNT_EXPR: + is_row_count = agg is AggregationType.COUNT and col == ROW_COUNT_EXPR + if is_row_count or (agg is AggregationType.SUM and _is_constant_expr(col)): # A constant, not a column: it must not go through the column → dataset lookup. semantic_model_name = self._find_dataset_for_row_count(expr_str, datasets) else: @@ -366,10 +368,11 @@ def _convert_metric( ) return [*num_metrics, *den_metrics, ratio_metric] - # COUNT(DISTINCT *) / COUNT(DISTINCT 1) / ...: not a column count and not a row count either + # COUNT(DISTINCT *) / COUNT(DISTINCT 1) / ..., anywhere in the expression (bare, wrapped in + # parens, or combined with other operations): 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): + if _contains_distinct_row_count(expr_str): raise _UnresolvedRowCountDataset(f"{expr_str!r} has no sensible SIMPLE or RATIO translation") # --- Fallback: complex expression that can't be decomposed --- diff --git a/converters/dbt/tests/test_msi_to_ossie.py b/converters/dbt/tests/test_msi_to_ossie.py index 5e86f646..ad797047 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -22,10 +22,11 @@ import pytest from syrupy.assertion import SnapshotAssertion -from ossie_dbt.converter_issues import ConverterIssueType +from ossie_dbt.converter_issues import ConverterIssue, ConverterIssueType from ossie_dbt.filter_utils import _render_filter_template from ossie import OssieDialect, OssieDocument from ossie_dbt.msi_to_ossie import MSIToOssieConverter +from metricflow_semantics.model.dbt_manifest_parser import parse_manifest_from_dbt_generated_manifest from metricflow_semantic_interfaces.implementations.metric import ( PydanticConversionTypeParams, PydanticCumulativeTypeParams, @@ -609,7 +610,10 @@ def test_simple_metric_with_metric_aggregation_params(self) -> None: @pytest.mark.parametrize("agg", [AggregationType.COUNT, AggregationType.SUM]) def test_a_row_count_always_comes_back_as_portable_sum_1(self, agg: AggregationType) -> None: - """COUNT(*) and SUM(1) mean the same thing in SQL, so both convert the same way. + """A COUNT/expr=1 metric comes back as SUM(1), like a SUM/expr=1 one. + + MetricFlow's own transform has already turned the count into a sum by the time this converter + sees it. SUM(1) matches COUNT(*) on any non-empty input; over zero rows it is NULL, not 0. Emitting ``COUNT(.*)`` to keep the dataset through a round trip was tried and reverted: it is not in the Ossie expression spec, several engines reject or misinterpret it, and the @@ -623,6 +627,36 @@ def test_a_row_count_always_comes_back_as_portable_sum_1(self, agg: AggregationT assert _ossie_metrics(result)[0].expression.dialects[0].expression == "SUM(1)" + def test_constant_metric_records_the_lost_semantic_model_on_the_cli_path(self) -> None: + """Routed through the real dbt loader, as the CLI does: the count is already SUM(1) by then.""" + 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") + manifest_json = _manifest(semantic_models=[customers, orders], metrics=[metric]).json( + by_alias=True, exclude_none=True + ) + result = MSIToOssieConverter().convert(parse_manifest_from_dbt_generated_manifest(manifest_json)) + + assert _ossie_metrics(result.output)[0].expression.dialects[0].expression == "SUM(1)" + assert result.issues == [ + ConverterIssue(ConverterIssueType.CONSTANT_METRIC_SEMANTIC_MODEL_LOSS, "order_count") + ] + + def test_constant_metric_with_a_single_semantic_model_loses_nothing(self) -> None: + orders = semantic_model_with_guaranteed_meta(name="orders") + metric = _metric_with_agg("order_count", AggregationType.COUNT, "1", "orders") + result = MSIToOssieConverter().convert(_manifest(semantic_models=[orders], metrics=[metric])) + + assert result.issues == [] + + def test_column_metric_with_several_semantic_models_loses_nothing(self) -> None: + customers = semantic_model_with_guaranteed_meta(name="customers") + orders = semantic_model_with_guaranteed_meta(name="orders") + metric = _metric_with_agg("revenue", AggregationType.SUM, "amount", "orders") + result = MSIToOssieConverter().convert(_manifest(semantic_models=[customers, orders], metrics=[metric])) + + assert result.issues == [] + def test_legacy_measure_based_count_also_converts(self) -> None: """A SIMPLE metric over a legacy Measure(agg=count, expr=1) is a row count too, not just the diff --git a/converters/dbt/tests/test_ossie_to_msi.py b/converters/dbt/tests/test_ossie_to_msi.py index 90d0d75e..f9eafd5f 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -342,6 +342,30 @@ def test_count_of_a_constant_normalizes_to_row_count_expr(self, expression: str) assert m.type_params.metric_aggregation_params.agg == AggregationType.COUNT assert m.type_params.expr == "1" + @pytest.mark.parametrize(("expression", "expr"), [("SUM(1)", "1"), ("SUM(2)", "2"), ("SUM(TRUE)", "TRUE")]) + def test_sum_of_a_constant_keeps_its_value(self, expression: str, expr: str) -> None: + """SUM adds the constant up, so SUM(2) is twice the row count and must not become SUM(1).""" + doc = _ossie_doc( + datasets=[_ossie_dataset("orders", fields=[_ossie_field("order_id")])], + metrics=[_ossie_metric("total", 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.SUM + assert m.type_params.metric_aggregation_params.semantic_model == "orders" + assert m.type_params.expr == expr + + @pytest.mark.parametrize("expression", ["SUM(1)", "SUM(2)", "SUM(0)"]) + def test_sum_of_a_constant_with_multiple_datasets_is_dropped_with_a_warning(self, expression: str) -> None: + """A constant has no column to place it in a dataset, so with several it is refused, not guessed.""" + doc = _ossie_doc(datasets=self._customers_and_orders(), metrics=[_ossie_metric("total", expression)]) + result = OssieToMSIConverter().convert(doc) + + assert result.output.metrics == [] + assert [i.element_name for i in result.issues] == ["total"] + def test_qualified_count_star_uses_dataset_qualifier(self) -> None: doc = _ossie_doc( datasets=[ @@ -455,12 +479,26 @@ def test_unsupported_count_star_forms_fall_back_to_the_raw_expression(self, expr assert result.metrics[0].type_params.expr == expression - @pytest.mark.parametrize("expression", ["COUNT(DISTINCT *)", "COUNT(DISTINCT 1)", "COUNT(DISTINCT orders.*)"]) + @pytest.mark.parametrize( + "expression", + [ + "COUNT(DISTINCT *)", + "COUNT(DISTINCT 1)", + "COUNT(DISTINCT orders.*)", + "COUNT(DISTINCT (*))", + "COUNT(DISTINCT (1))", + "(COUNT(DISTINCT *))", + "COUNT(DISTINCT *) * 100", + "COALESCE(COUNT(DISTINCT 1), 0)", + "CAST(COUNT(DISTINCT *) AS INT)", + ], + ) def test_distinct_of_a_row_count_is_dropped_with_a_warning(self, expression: str) -> None: """COUNT(DISTINCT *) and friends are valid SQL but answer 'does any row exist' (0 or 1), not a meaningful total. Falling back to a raw SUM of it would be a nested aggregate MetricFlow cannot - run, bound to a guessed dataset; dropped instead, the same as an unresolvable COUNT(*). + run, bound to a guessed dataset; dropped instead, the same as an unresolvable COUNT(*). Caught + whether it is the whole expression, redundantly parenthesized, or wrapped in another function. """ doc = _ossie_doc( datasets=[_ossie_dataset("orders", fields=[_ossie_field("order_id"), _ossie_field("amount")])], @@ -792,8 +830,13 @@ def test_count_star_round_trips_to_a_portable_sum_one(self) -> None: no qualifier to recover a dataset from. Emitting COUNT(.*) instead, to carry the dataset through, was tried and reverted: it is outside the Ossie expression spec, several engines reject or misinterpret it, and no sibling converter recognizes it as a row count. SUM(1) is what every engine - agrees on. Converting SUM(1) forward again, with more than one dataset, still silently picks the - first dataset rather than refusing like a bare COUNT(*) does; closing that gap is tracked separately. + agrees on. + + Converting SUM(1) forward again, with more than one dataset, now refuses rather than silently + picking the first dataset: SUM of a constant has no column to place it in a dataset, so it goes + through the same ambiguity check as COUNT(*) and is dropped with ROW_COUNT_METRIC_DROPPED instead of + a plausible but wrong semantic_model. The dataset is genuinely lost by this point, not recoverable; refusing is + the honest outcome, not a bug to fix on the next leg. """ original = _ossie_doc( datasets=[ @@ -813,6 +856,19 @@ def test_count_star_round_trips_to_a_portable_sum_one(self) -> None: assert expressions["order_count"] == "SUM(1)" assert "SUM(1)" in expressions["avg_order_value"] + # The first round trip already flattened the ratio into independent metrics (numerator, + # denominator, and the ratio's own expression string). Only the ones that are still row + # counts drop here: the denominator (SUM(1), ambiguous) and the ratio itself, which depends + # on it. The numerator (SUM(orders.amount), not a row count) is a valid metric on its own + # and survives, same as it would have before any of this. + again = OssieToMSIConverter().convert(ossie_doc) + assert [m.name for m in again.output.metrics] == ["avg_order_value__numerator"] + assert {i.element_name for i in again.issues} == { + "order_count", + "avg_order_value", + "avg_order_value__denominator", + } + 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 ef964f76abc6932621701600cc7dbb58ddb21f14 Mon Sep 17 00:00:00 2001 From: LukasSchwarzlmueller Date: Fri, 2 Oct 2026 23:26:03 +0200 Subject: [PATCH 4/4] fix(dbt): catch tokenizer errors when parsing, only flag SUM constants 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. --- .../dbt/src/ossie_dbt/expression_utils.py | 10 +++++----- converters/dbt/src/ossie_dbt/msi_to_ossie.py | 10 ++++++++-- converters/dbt/tests/test_msi_to_ossie.py | 20 +++++++++++++++++++ converters/dbt/tests/test_ossie_to_msi.py | 11 ++++++++++ 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/converters/dbt/src/ossie_dbt/expression_utils.py b/converters/dbt/src/ossie_dbt/expression_utils.py index a5e29a58..340e883f 100644 --- a/converters/dbt/src/ossie_dbt/expression_utils.py +++ b/converters/dbt/src/ossie_dbt/expression_utils.py @@ -58,7 +58,7 @@ def _is_constant_expr(expr: str) -> bool: """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: + except sqlglot.errors.SqlglotError: return False return isinstance(node, exp.Boolean) or (isinstance(node, exp.Literal) and not node.is_string) @@ -80,7 +80,7 @@ def _contains_distinct_row_count(expression: str) -> bool: """ try: tree = sqlglot.parse_one(expression.strip()) - except sqlglot.errors.ParseError: + except sqlglot.errors.SqlglotError: return False for count in tree.find_all(exp.Count): argument = count.this @@ -105,7 +105,7 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O """ try: tree = sqlglot.parse_one(expression.strip()) - except sqlglot.errors.ParseError: + except sqlglot.errors.SqlglotError: return None if isinstance(tree, exp.Count): @@ -185,7 +185,7 @@ def _try_parse_ratio(expr_str: str) -> Optional[Tuple[str, str]]: """Try to parse ``(expr_a) / (expr_b)`` using sqlglot, returning ``(num_expr, den_expr)`` or None.""" try: tree = sqlglot.parse_one(expr_str.strip()) - except sqlglot.errors.ParseError: + except sqlglot.errors.SqlglotError: return None if not isinstance(tree, exp.Div): @@ -207,7 +207,7 @@ def _get_dataset_qualifier(expression: str) -> Optional[str]: """Return the sole dataset qualifier referenced by an expression, if present.""" try: tree = sqlglot.parse_one(expression.strip()) - except sqlglot.errors.ParseError: + except sqlglot.errors.SqlglotError: return None qualifiers = { diff --git a/converters/dbt/src/ossie_dbt/msi_to_ossie.py b/converters/dbt/src/ossie_dbt/msi_to_ossie.py index f89b5c74..fcfdf6ef 100644 --- a/converters/dbt/src/ossie_dbt/msi_to_ossie.py +++ b/converters/dbt/src/ossie_dbt/msi_to_ossie.py @@ -302,10 +302,16 @@ def _resolve_simple( @staticmethod def _aggregates_a_constant(metric: Metric) -> bool: - """Return True for a SIMPLE metric over a constant expr, e.g. a row count rewritten to SUM(1).""" + """Return True for a SIMPLE SUM over a constant expr, e.g. a row count rewritten to SUM(1). + + Only SUM: it is the one aggregation the Ossie → MSI side resolves as a constant (and refuses on + ambiguous datasets). MAX(1), AVG(1) and the like go through the ordinary column lookup there. + """ 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) + if metric.type is not MetricType.SIMPLE or params is None or expr is None: + return False + return params.agg is AggregationType.SUM and _is_constant_expr(expr) @staticmethod def _qualify_col(col: str, semantic_model: str) -> str: diff --git a/converters/dbt/tests/test_msi_to_ossie.py b/converters/dbt/tests/test_msi_to_ossie.py index ad797047..8c3b640f 100644 --- a/converters/dbt/tests/test_msi_to_ossie.py +++ b/converters/dbt/tests/test_msi_to_ossie.py @@ -649,6 +649,26 @@ def test_constant_metric_with_a_single_semantic_model_loses_nothing(self) -> Non assert result.issues == [] + @pytest.mark.parametrize("agg", [AggregationType.MAX, AggregationType.AVERAGE]) + def test_non_sum_constant_metric_is_not_flagged(self, agg: AggregationType) -> None: + """Only SUM of a constant is refused on the way back, so only SUM gets the loss warning.""" + customers = semantic_model_with_guaranteed_meta(name="customers") + orders = semantic_model_with_guaranteed_meta(name="orders") + metric = _metric_with_agg("m", agg, "1", "orders") + result = MSIToOssieConverter().convert(_manifest(semantic_models=[customers, orders], metrics=[metric])) + + assert result.issues == [] + + def test_unparseable_expr_does_not_abort_the_conversion(self) -> None: + """An unterminated quote is a sqlglot TokenError, not a ParseError; it must not crash the run.""" + customers = semantic_model_with_guaranteed_meta(name="customers") + orders = semantic_model_with_guaranteed_meta(name="orders") + metric = _metric_with_agg("m", AggregationType.SUM, "CASE WHEN status = 'paid THEN amount END", "orders") + result = MSIToOssieConverter().convert(_manifest(semantic_models=[customers, orders], metrics=[metric])) + + assert [m.name for m in _ossie_metrics(result.output)] == ["m"] + assert result.issues == [] + def test_column_metric_with_several_semantic_models_loses_nothing(self) -> None: customers = semantic_model_with_guaranteed_meta(name="customers") orders = semantic_model_with_guaranteed_meta(name="orders") diff --git a/converters/dbt/tests/test_ossie_to_msi.py b/converters/dbt/tests/test_ossie_to_msi.py index f9eafd5f..099bb9c5 100644 --- a/converters/dbt/tests/test_ossie_to_msi.py +++ b/converters/dbt/tests/test_ossie_to_msi.py @@ -366,6 +366,17 @@ def test_sum_of_a_constant_with_multiple_datasets_is_dropped_with_a_warning(self assert result.output.metrics == [] assert [i.element_name for i in result.issues] == ["total"] + def test_unparseable_expression_does_not_abort_the_conversion(self) -> None: + """An unterminated quote is a sqlglot TokenError, not a ParseError; it falls back, it doesn't crash.""" + expression = "SUM(CASE WHEN status = 'paid THEN amount END)" + doc = _ossie_doc( + datasets=[_ossie_dataset("orders", fields=[_ossie_field("amount")])], + metrics=[_ossie_metric("paid", expression)], + ) + result = OssieToMSIConverter().convert(doc).output + + assert result.metrics[0].type_params.expr == expression + def test_qualified_count_star_uses_dataset_qualifier(self) -> None: doc = _ossie_doc( datasets=[