diff --git a/converters/dbt/src/ossie_dbt/expression_utils.py b/converters/dbt/src/ossie_dbt/expression_utils.py index d499925d..4e553da9 100644 --- a/converters/dbt/src/ossie_dbt/expression_utils.py +++ b/converters/dbt/src/ossie_dbt/expression_utils.py @@ -36,11 +36,12 @@ def _col_name(node: exp.Expression) -> str: return _strip_qualifier(rendered) -def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, Optional[float]]]: +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)`` for recognised patterns, ``None`` otherwise. - ``percentile`` is only set for ``PERCENTILE`` aggregations; it is ``None`` for all others. + 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. """ try: @@ -52,12 +53,12 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O 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 + return AggregationType.COUNT_DISTINCT, _col_name(cols[0]), None, False return None # COUNT(col) if isinstance(tree, exp.Count): - return AggregationType.COUNT, _col_name(tree.this), None + return AggregationType.COUNT, _col_name(tree.this), None, False # SUM(CASE WHEN col THEN 1 ELSE 0 END) → SUM_BOOLEAN if isinstance(tree, exp.Sum) and isinstance(tree.this, exp.Case): @@ -71,21 +72,21 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O and isinstance(ifs[0].args.get("true"), exp.Literal) and ifs[0].args["true"].name == "1" ): - return AggregationType.SUM_BOOLEAN, ifs[0].this.sql(), None + return AggregationType.SUM_BOOLEAN, ifs[0].this.sql(), None, False return None # SUM(col) if isinstance(tree, exp.Sum): - return AggregationType.SUM, _col_name(tree.this), None + return AggregationType.SUM, _col_name(tree.this), None, False if isinstance(tree, exp.Avg): - return AggregationType.AVERAGE, _col_name(tree.this), None + return AggregationType.AVERAGE, _col_name(tree.this), None, False if isinstance(tree, exp.Min): - return AggregationType.MIN, _col_name(tree.this), None + return AggregationType.MIN, _col_name(tree.this), None, False if isinstance(tree, exp.Max): - return AggregationType.MAX, _col_name(tree.this), None + return AggregationType.MAX, _col_name(tree.this), None, False # PERCENTILE_CONT(p) WITHIN GROUP (ORDER BY col) # sqlglot parses this as WithinGroup(this=PercentileCont(...), expression=Order(...)) @@ -105,8 +106,8 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O except (AttributeError, ValueError): return None if p == 0.5 and isinstance(inner, exp.PercentileCont): - return AggregationType.MEDIAN, col, None - return AggregationType.PERCENTILE, col, p + return AggregationType.MEDIAN, col, None, False + return AggregationType.PERCENTILE, col, p, isinstance(inner, exp.PercentileDisc) return None diff --git a/converters/dbt/src/ossie_dbt/osi_to_msi.py b/converters/dbt/src/ossie_dbt/osi_to_msi.py index c8f155fc..28ce3b16 100644 --- a/converters/dbt/src/ossie_dbt/osi_to_msi.py +++ b/converters/dbt/src/ossie_dbt/osi_to_msi.py @@ -282,9 +282,16 @@ def _convert_metric( # --- SIMPLE: single aggregation --- agg_result = _extract_agg_info(expr_str) if agg_result is not None: - agg, col, percentile = agg_result + agg, col, percentile, use_discrete = agg_result semantic_model_name = self._find_dataset_for_col(expr_str, col, datasets) - agg_params = PydanticMeasureAggregationParameters(percentile=percentile) if percentile is not None else None + agg_params = ( + PydanticMeasureAggregationParameters( + percentile=percentile, + use_discrete_percentile=use_discrete, + ) + if percentile is not None + else None + ) return [ PydanticMetric( name=name, diff --git a/converters/dbt/tests/test_osi_to_msi.py b/converters/dbt/tests/test_osi_to_msi.py index dfed1e42..50db4fa2 100644 --- a/converters/dbt/tests/test_osi_to_msi.py +++ b/converters/dbt/tests/test_osi_to_msi.py @@ -23,12 +23,23 @@ from ossie import OSIDataType, OSIDimension from ossie_dbt.msi_to_osi import MSIToOSIConverter from ossie_dbt.osi_to_msi import OSIToMSIConverter +from metricflow_semantic_interfaces.implementations.elements.measure import ( + PydanticMeasureAggregationParameters, +) +from metricflow_semantic_interfaces.implementations.metric import ( + PydanticMetric, + PydanticMetricAggregationParams, + PydanticMetricTypeParams, +) +from metricflow_semantic_interfaces.test_utils import default_meta, semantic_model_with_guaranteed_meta from metricflow_semantic_interfaces.type_enums import ( AggregationType, DimensionType, MetricType, ) from tests.helpers import ( + _manifest, + _measure, _osi_dataset, _osi_doc, _osi_field, @@ -445,3 +456,46 @@ def test_osi_to_msi_to_osi_preserves_structure(self, snapshot: SnapshotAssertion assert metrics[0].name == "revenue" assert metrics[0].expression.dialects[0].expression == "SUM(orders.amount)" assert osi_doc.to_osi_yaml() == snapshot + + def test_discrete_percentile_survives_round_trip(self) -> None: + """A PERCENTILE_DISC metric keeps use_discrete_percentile through MSI -> OSI -> MSI.""" + orders = semantic_model_with_guaranteed_meta( + name="orders", + measures=[_measure("amount", agg=AggregationType.SUM, expr="amount")], + ) + metric = PydanticMetric( + name="p95_amount", + description=None, + type=MetricType.SIMPLE, + type_params=PydanticMetricTypeParams( + expr="amount", + metric_aggregation_params=PydanticMetricAggregationParams( + semantic_model="orders", + agg=AggregationType.PERCENTILE, + agg_params=PydanticMeasureAggregationParameters( + percentile=0.95, + use_discrete_percentile=True, + ), + agg_time_dimension=None, + non_additive_dimension=None, + ), + ), + filter=None, + metadata=default_meta(), + config=None, + ) + + osi_doc = MSIToOSIConverter().convert( + _manifest(semantic_models=[orders], metrics=[metric]) + ).output + + osi_expr = osi_doc.semantic_model[0].metrics[0].expression.dialects[0].expression + assert osi_expr == "PERCENTILE_DISC(0.95) WITHIN GROUP (ORDER BY orders.amount)" + + back = OSIToMSIConverter().convert(osi_doc).output + m = back.metrics[0] + + assert m.type_params.metric_aggregation_params is not None + assert m.type_params.metric_aggregation_params.agg == AggregationType.PERCENTILE + assert m.type_params.metric_aggregation_params.agg_params is not None + assert m.type_params.metric_aggregation_params.agg_params.use_discrete_percentile is True