Support latest MetricFlow YAML spec, conversion metrics, and saved queries#213
Support latest MetricFlow YAML spec, conversion metrics, and saved queries#213nicosuave wants to merge 9 commits into
Conversation
…aved queries Extend the MetricFlow adapter to parse, in addition to the legacy spec: - Latest spec (dbt Core 1.12 / Fusion): top-level models: with a nested semantic_model: block, column-based entity:/dimension: under columns:, measures folded into type: simple metrics with agg/expr, and promoted top-level metric keys (input_metrics, input_metric, numerator/denominator, base_metric/conversion_metric, window, grain_to_date, granularity). - Conversion metrics via type_params.conversion_type_params (and promoted keys), mapped onto Sidemantic conversion metrics with calculation flavor and constant_properties retained in metadata. - Top-level saved_queries (query_params + exports), exposed on graph.metadata['saved_queries']. - Derived per-input offset_window/offset_to_grain modifiers and cumulative period_agg, retained in metric metadata. Legacy-format parsing is unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8949d78a1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…saved queries Latest-spec type:simple metrics fold a model measure but were registered as graph-level metrics with unqualified SQL (e.g. amount) or no SQL (a bare count). Selecting only such a metric raised No models found for query because the planner could not infer the owning model. Qualify the folded expression with the model name, and anchor a bare count to the model primary key, so the metric carries model context while staying queryable by dependent ratio and derived metrics. Also reset per-parse saved_queries state at the start of parse() so reusing a MetricFlowAdapter does not leak a prior source's saved queries into a later graph's metadata.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd15a4186
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ain conversion metrics as metadata Derived metrics referencing inputs by a plain (non-offset, unfiltered) alias now rewrite the alias back to the real input metric so dependency resolution sees real metrics and the metric is queryable. Aliases carrying offset_window/offset_to_grain or a per-input filter are preserved as round-trip metadata. Inline latest-spec count metrics with a constant expr (count with expr: 1 or '*') are anchored to the model primary key, the same as a bare count, so selecting them no longer raises No models found for query. MetricFlow conversion metrics reference base/conversion measures, which have no faithful mapping to Sidemantic's event-filter conversion funnel; they are retained on graph.metadata[metricflow_conversion_metrics] instead of being registered as broken queryable conversion metrics that would generate wrong SQL. Conversion specs are reset per parse to avoid leaking across reuse.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8180ae98b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… own column An inline latest-spec simple metric with an aggregation but no expr (e.g. agg: sum on a customers model) was anchored to the model primary key, so it generated SUM(primary_key) and silently aggregated the wrong column. Only count-style measures may use the primary key anchor (COUNT(pk) == COUNT(*)); other expr-less aggregations now default to the measure's own column, qualified with the model (e.g. customers.lifetime_spend_pretax).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2107c1c396
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A latest-spec model with semantic_model.enabled: false yields no model, but its inline model-local metrics were still registered. Those metrics fold a measure on the absent model, so they carry no SQL/model context and a CLI query for one raised No models found for query. Skip inline metrics whenever the owning semantic model is not produced so the graph never exposes an unqueryable metric.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9519e6c57a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…entable aggs Two correctness bugs in the latest-spec inline metric path: - A simple metric with a filter registered the filter on the graph-level metric but the aggregation path ignored Metric.filters, so a filtered metric such as completed_revenue aggregated every row and silently dropped the MetricFlow filter. Render metric-level filters via CASE WHEN in the graph-level aggregate path (mirroring the model-scoped measure path) and project the filter columns into the CTE for graph-level simple aggregates. - agg: percentile (and any other aggregation Sidemantic cannot represent) fell through the agg mapper's default and was coerced to sum, registering SUM(amount) under the percentile metric's name. Map unrepresentable aggs to None and skip those measures/metrics instead. Also normalize aggregation names case-insensitively so uppercase aggs like SUM map correctly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47b9ab4aa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… CTE The CASE WHEN rendered for a filtered inline simple metric left filter columns unqualified (e.g. status = 'completed'). When the metric was queried alongside a joined model whose CTE also exposed a same-named column, the predicate became ambiguous and the query failed to bind. Infer the metric's owning model from its SQL (or model context) and qualify the filter's columns with that model's CTE before rendering the aggregation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b89f5c656
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A filtered graph-level count metric with an expression counted a constant 1 for every matching row, so rows with a NULL counted expression were included even though the unfiltered COUNT(expr) path skips them. Count the expression inside the CASE WHEN (skipping NULLs); only a bare row count (*) counts every matching row.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775c084eb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ut metrics A latest-spec cumulative metric built from a promoted input_metric is a graph-level metric, so default-time-dimension resolution skipped it and a query raised 'requires a time dimension for ordering' even when the underlying model declares an agg_time_dimension. Resolve the cumulative metric's owning model by following its base input reference (through intermediate graph-level metrics) and apply that model's default time dimension.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd289155bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
What changed
Extends
sidemantic/adapters/metricflow.pyto keep pace with dbt MetricFlow / the dbt Semantic Layer. All additions are backward compatible: the legacy YAML spec continues to parse unchanged and existing tests stay green.Latest YAML spec (dbt Core 1.12 / Fusion engine)
The adapter now detects new-vs-legacy shape and parses both. For the latest spec it handles:
models:with a nestedsemantic_model:block (honorsenabled: false, andsemantic_model.namedefaulting to the model name).entity:/dimension:declared undercolumns:, including theentity: primaryshorthand and column-levelgranularity:for time dimensions.type: simplemetrics carryingagg/exprat the top level.type_params:numerator/denominator(ratio),input_metrics(derived),input_metric+window+grain_to_date+period_agg(cumulative), andbase_metric/conversion_metric/entity(conversion).Conversion metrics
Previously skipped with a "conversion not yet supported" comment. Now parsed from
type_params.conversion_type_params(legacy) and the promoted top-level keys (latest), mappingbase_measure/conversion_measureto base/conversion events,entity, andwindowtoconversion_window. Thecalculationflavor (conversion_rate/conversion/conversions) andconstant_propertiesare retained in metric metadata.Saved queries
Top-level
saved_queries(list or mapping form) are parsed with theirquery_params(metrics, group_by, where, order_by, limit) andexports, and exposed ongraph.metadata["saved_queries"]. They are intentionally kept out ofgraph.metrics.Derived/cumulative modifiers
Per-input
offset_window/offset_to_grain/aliason derived inputs and cumulativeperiod_aggare retained in metric metadata.Tests
conversion_metrics.yml,simple_manifest_metrics.yaml,simple_manifest_saved_queries.yaml,saved_queries_example.yml).tests/fixtures/metricflow/latest_spec_models.yml) and a parsing test covering embedded semantic models, column-based entities/dimensions, folded simple metrics, promoted ratio/derived/cumulative/conversion keys, and offset/period_agg metadata, and wired it into the shared added-fixture coverage suite.Known limitations
calculationflavor /fill_nulls_with/constant_propertiesare preserved in metadata rather than driving execution semantics.semantic_models:shape.