feat: support dataset-scoped metrics - #343
Conversation
| "items": { | ||
| "$ref": "#/$defs/Metric" | ||
| }, | ||
| "description": "Dataset-scoped metrics. Expressions must resolve entirely within this dataset and must not traverse relationships or reference fields of another dataset. Names must be unique within the dataset and must not collide with any model-scoped metric name. Referenced from outside the dataset as dataset_name.metric_name. Metrics that span datasets belong in semantic_model.metrics." |
There was a problem hiding this comment.
How about clarifying here that the dataset scoped metrics are computed at the grain of the dataset's primary key. Also why restrict relationship traversal ?
There was a problem hiding this comment.
- Agree that dataset-scoped metrics are computed at the dataset's grain. However, this grain should be safely computable without requiring a PK.
- Agree with the relationship traversal restriction. To follow on to the layering framing, dataset-scoped metrics belong to the relational / metric layer. In that layer, each dataset acts as a self-contained table that can be independently queried / composed in SQL. If databases supported this layer natively, Ossie datasets could be pushed down to the database layer & used by multiple Ossie models. Relationship traversal is more tied to the multi-table semantics within a Ossie model (i.e. it depends on the graph shape), which would mean that datasets now need to reason about each other in the context of some graph
| # Represents key calculations like sums, averages, ratios, etc. | ||
| # | ||
| # The same structure is used in two placements: | ||
| # - semantic_model.metrics (model-scoped): may span datasets via relationships |
There was a problem hiding this comment.
Let's add here that semantic_model.metrics can be used to combine multiple dataset scoped metrics ?
There was a problem hiding this comment.
To Josh's PR review questions, this fits with the question on if / how to build metrics from other metrics. That's probably worth a followup PR / discussion since it touches on how to extend the expression language
christianeu-db
left a comment
christianeu-db
left a comment
There was a problem hiding this comment.
Thanks for putting this PR together - definitely agree with adding dataset-scoped metrics. Conceptually, this fits in nicely with the layered model from the working group.
Most of my questions are around making dataset-scoped metrics consistent with fields in terms of name-uniqueness, how they reference fields in their expressions, and the requirement to always be called with a two-level name. Another question is around what can be an input to a dataset-scoped metric.
With respect to your open questions:
- Relationship traversal information seems like a model-level concept since it spans across datasets.
- Agreed that defining metric / other reference rules should be a separate discussion since that's a pretty fundamental part of the spec
|
|
||
| **Rules** | ||
|
|
||
| 1. A dataset-scoped metric's expression MUST only reference fields of the dataset that declares it. It MUST NOT traverse relationships or reference fields belonging to another dataset. A metric that needs to span datasets MUST be model-scoped. |
There was a problem hiding this comment.
Does the metric's expression reference the dataset's fields, the dataset's source's columns or both (with some disambiguation mechanism)?
In the example below, COUNT(order_id) references a column in the source, not a field in the containing dataset.
| **Rules** | ||
|
|
||
| 1. A dataset-scoped metric's expression MUST only reference fields of the dataset that declares it. It MUST NOT traverse relationships or reference fields belonging to another dataset. A metric that needs to span datasets MUST be model-scoped. | ||
| 2. Dataset-scoped metric names MUST be unique within their dataset. Two different datasets MAY each declare a metric with the same name (e.g. `orders.item_count` and `shipments.item_count`). |
There was a problem hiding this comment.
Should uniqueness apply across both fields & metrics (e.g. can a metric and a field in a dataset have the same name)?
Uniqueness would guarantee that dataset.name referred to exactly one of a field or metric, not both.
|
|
||
| Rule 1 constrains only what a metric's *expression* may reference. It does not restrict how the metric may be queried. A dataset-scoped metric can still be grouped by, or filtered on, dimensions from other datasets reached through relationships — grouping dimensions are supplied by the consumer at query time and are not part of the metric definition. | ||
|
|
||
| For example, a metric declared on `store_sales` as `SUM(store_sales.ss_ext_sales_price)` is dataset-scoped because its expression touches only `store_sales`, yet it remains valid to group that metric by `item.i_brand` or `store.s_state` via the model's relationships. Only a metric whose own expression must reach into another dataset — such as `SUM(store_sales.amount) / COUNT(DISTINCT customer.id)` — needs to be model-scoped. |
There was a problem hiding this comment.
Should dataset-scoped metric expressions use a single-layer namespace since they can only reference fields from the dataset and/or source (depending on this discussion)?
For example, store_sales-scoped metric would have the expression SUM(ss_ext_sales_price) instead of SUM(store_sales.ss_ext_sales_price). This would be more consistent with fields and lines up with some of the layering framing (i.e. dataset-scoped objects don't reason about other datasets)
If a single-dataset metric references the two level namespace, that seems to be relational metrics layer being aware of concepts (two-layer names) from the layer above.
| "items": { | ||
| "$ref": "#/$defs/Metric" | ||
| }, | ||
| "description": "Dataset-scoped metrics. Expressions must resolve entirely within this dataset and must not traverse relationships or reference fields of another dataset. Names must be unique within the dataset and must not collide with any model-scoped metric name. Referenced from outside the dataset as dataset_name.metric_name. Metrics that span datasets belong in semantic_model.metrics." |
There was a problem hiding this comment.
- Agree that dataset-scoped metrics are computed at the dataset's grain. However, this grain should be safely computable without requiring a PK.
- Agree with the relationship traversal restriction. To follow on to the layering framing, dataset-scoped metrics belong to the relational / metric layer. In that layer, each dataset acts as a self-contained table that can be independently queried / composed in SQL. If databases supported this layer natively, Ossie datasets could be pushed down to the database layer & used by multiple Ossie models. Relationship traversal is more tied to the multi-table semantics within a Ossie model (i.e. it depends on the graph shape), which would mean that datasets now need to reason about each other in the context of some graph
| # Represents key calculations like sums, averages, ratios, etc. | ||
| # | ||
| # The same structure is used in two placements: | ||
| # - semantic_model.metrics (model-scoped): may span datasets via relationships |
There was a problem hiding this comment.
To Josh's PR review questions, this fits with the question on if / how to build metrics from other metrics. That's probably worth a followup PR / discussion since it touches on how to extend the expression language
|
|
||
| 1. A dataset-scoped metric's expression MUST only reference fields of the dataset that declares it. It MUST NOT traverse relationships or reference fields belonging to another dataset. A metric that needs to span datasets MUST be model-scoped. | ||
| 2. Dataset-scoped metric names MUST be unique within their dataset. Two different datasets MAY each declare a metric with the same name (e.g. `orders.item_count` and `shipments.item_count`). | ||
| 3. A dataset-scoped metric name MUST NOT collide with the name of any model-scoped metric in the same semantic model. This keeps an unqualified metric reference unambiguous. |
There was a problem hiding this comment.
Could the rule be that only model-scoped metrics can use unqualified names?
The requirement that unqualified metric references must be unambiguous seems to conflict with rule 2, which could allow for two datasets to have metrics with the same name. Rule 2 seems more consistent with field behavior / having datasets not reason about name uniqueness across each other.
Then, the model could be:
- Unscoped names: model-level
- Scoped names: dataset-level
Some tools, such as Tableau, support cross-dataset dimensions, which also live in the global namespace. In their model, they have both dataset-scoped metrics/dimensions and model-scoped metrics/dimensions
| | **AtScale SML** | Standalone `metric` object bound to one `dataset` + `column` | Separate `metric_calc` object type | Global across all repositories | Bare `unique_name` | | ||
| | **dbt MetricFlow** (v1.12+) | Metrics inside a semantic model | Top-level `metrics` | Global across the project | Bare name | | ||
| | **Cube** | Measures within cubes | Calculated measures referencing other measures | Per cube | Qualified — `cube.member` | | ||
| | **Databricks UC metric views** | Measures in the view (one flat scope) | Joins declared inside the view | Per metric view | `MEASURE(name)` | |
There was a problem hiding this comment.
For metric views, it's close to named subqueries when building a view. Each source has a name so within a metric view, field names are <source_name>.<field_name> but the metric view itself acts like a table so its schema has a flat scope
|
|
||
| **Scope restricts the expression, not the query** | ||
|
|
||
| Rule 1 constrains only what a metric's *expression* may reference. It does not restrict how the metric may be queried. A dataset-scoped metric can still be grouped by, or filtered on, dimensions from other datasets reached through relationships — grouping dimensions are supplied by the consumer at query time and are not part of the metric definition. |
There was a problem hiding this comment.
Probably worth clarifying that within the layering proposal, the ability to natively query across datasets from a single interface would be in what is called the dimensional layer (Will was also proposing "presentation layer" as an alternate name)
|
|
||
| **Choosing a placement** | ||
|
|
||
| Prefer dataset-scoped for simple aggregations that belong conceptually to one entity — they keep the metric next to the fields it depends on and make the dataset independently interpretable. Use model-scoped for anything requiring a join. |
There was a problem hiding this comment.
(nit) The aggregations themselves don't necessarily need to be simple. More complex aggregates (e.g. later window calculation extensions) could be dataset-scoped
| # store_sales. These may still be grouped by dimensions of other | ||
| # datasets (for example item.i_brand or store.s_state) through the | ||
| # model's relationships. Referenced as store_sales.<metric_name>. | ||
| metrics: |
There was a problem hiding this comment.
Two tests read this file directly and assert on model-level metrics:
converters/omni/tests/test_ossie_to_omni.pytest_tpcds_export_matches_expectedloads this file and compare the export againsttests/fixtures/tpcds_omniconverters/omni/tests/test_roundtrip.pytest_ossie_roundtrip_up_to_documented_normalizations
Both fail due to this change (cd converters/omni && uv run pytest).
I suggest to update the omni converter to host dataset-scoped metrics (updating the fixture in this PR). That would be my preference.
| "$ref": "#/$defs/Field" | ||
| } | ||
| }, | ||
| "metrics": { |
There was a problem hiding this comment.
OssieDataset in python/src/ossie/models.py (the model shipped as the apache-ossie package) has no metrics field. Pydantic's default extra='ignore' means dataset-scoped metrics are discarded on load with no error:
doc = OssieDocument.model_validate(yaml.safe_load(open('examples/tpcds_semantic_model.yaml')))
# store_sales has no `metrics` key; only customer_lifetime_value and
# store_productivity survive to_ossie_yaml(). Three metrics vanish silently.Adding metrics: list[OssieMetric] | None = None to OssieDataset fixes it.
Worth noting the reason nothing caught this: python/tests/test_models.py:75 only cross-checks the DataType enum against the JSON Schema, so structural drift between the schema and the pydantic model is invisible. A test that walks $defs properties and asserts each one exists on the corresponding model would have failed here.
|
|
||
| **Consumer guidance: flattening to a single metric namespace** | ||
|
|
||
| Consumers whose native model has only model-level metrics do not need to represent the two placements separately. Because a dataset-scoped metric's expression resolves entirely within its declaring dataset, that expression is already valid as a model-scoped metric — hoisting requires no expression rewriting. |
There was a problem hiding this comment.
Every converter reads model-level metrics only:
| Converter | Location |
|---|---|
| omni | converters/omni/src/ossie_omni/ossie_to_omni.py:187 |
| honeydew | converters/honeydew/.../converter.py:136 |
| snowflake | converters/snowflake/.../converter.py:169 |
| nvidia | converters/nvidia/.../native_converter.py:322 |
| orionbelt | converters/orionbelt/.../ossie_to_obml.py:170 |
| databricks | converters/databricks/python/.../ossie_to_metric_view.py:168 |
| polaris | converters/polaris/.../OssieYamlGenerator.java:72 |
Convert the new TPC-DS example to any target today and three metrics disappear with no warning. Compare converters/honeydew, which at least warnings.warns when it has to guess.
"Consumers that read only semantic_model.metrics remain valid" is a reasonable spec position, but combined with moving the reference example it means the flagship model is lossy through the entire hub-and-spoke on day one.
Two things would help:
- A shared hoist helper in the spec tooling (
iter_metrics(model)yielding both placements with qualified names), so converters get this right by default rather than each reimplementing it. - Stronger wording here: a consumer that ignores
datasets[].metricssilently produces an incomplete model, which is a lossy conversion, not a valid one. At minimum it SHOULD warn.
| if qualifiers is None: | ||
| continue | ||
|
|
||
| foreign = sorted(q for q in qualifiers if q != dataset_name) |
There was a problem hiding this comment.
SQL identifiers are case-insensitive, and sqlglot preserves the raw text of unquoted identifiers, so q != dataset_name is comparing raw casing against the YAML name.
To reproduce, use dataset orders, dialect SNOWFLAKE, expression SUM(ORDERS.AMOUNT):
[Scope] Dataset-scoped metric 'orders.total' in model 'm' (SNOWFLAKE) references dataset(s) 'ORDERS'.
So a correct model is rejected, and the error names a dataset that does not exist. Uppercase is the natural casing for Snowflake semantic views, which the PR body names as the motivating consumer.
Fold case before comparing (q.casefold() != dataset_name.casefold()). Strictly correct handling also depends on whether the identifier was quoted, but case-folding unquoted identifiers covers the realistic cases and is a clear improvement on the current behavior.
| continue | ||
| if tree is None: | ||
| continue | ||
| return {col.table for col in tree.find_all(exp.Column) if col.table} |
There was a problem hiding this comment.
Verified with sqlglot:
qualifiers("SUM(orders.payload.amount)") # -> {'payload'} (not 'orders')
qualifiers("SUM(payload.amount)") # -> {'payload'}For a three-part path sqlglot puts the middle part in col.table, so a dataset-scoped metric over a STRUCT/VARIANT column on orders is rejected with references dataset(s) payload'`.
There is no way to express such a metric at dataset scope at all, the only escape is moving it to model scope, where the rule does not apply.
exp.Column has catalog/db/table/this parts, for the three-part case you want to look at col.db (or col.parts[0]) as the potential dataset qualifier, and treat a bare two-part path whose first element is a declared field name as intra-dataset.
|
|
||
| 1. A dataset-scoped metric's expression MUST only reference fields of the dataset that declares it. It MUST NOT traverse relationships or reference fields belonging to another dataset. A metric that needs to span datasets MUST be model-scoped. | ||
| 2. Dataset-scoped metric names MUST be unique within their dataset. Two different datasets MAY each declare a metric with the same name (e.g. `orders.item_count` and `shipments.item_count`). | ||
| 3. A dataset-scoped metric name MUST NOT collide with the name of any model-scoped metric in the same semantic model. This keeps an unqualified metric reference unambiguous. |
There was a problem hiding this comment.
Rule 4 makes a dataset-scoped metric reachable as dataset_name.metric_name, which is exactly the form a field already uses. Nothing forbids a metric from taking a field's name in the same dataset, and the validator accepts it:
- name: orders
fields:
- name: amount # ...
metrics:
- name: amount # expression: SUM(orders.amount)→ Validation PASSED.
A consumer resolving orders.amount now cannot tell whether it means the row-level field or the aggregate, and the metric's own expression SUM(orders.amount) becomes self-referential under rule 4. Suggest a rule 5: a dataset-scoped metric name MUST NOT collide with the name of any field of the same dataset: plus the corresponding check in validate_unique_names, which already has both name lists in scope.
| | `description` | string | No | Human-readable description | | ||
| | `ai_context` | string/object | No | Additional context for AI tools (e.g., synonyms, common terms) | | ||
| | `fields` | array | No | Row-level attributes for grouping, filtering, and metric expressions | | ||
| | `metrics` | array | No | Dataset-scoped metrics whose expressions resolve entirely within this dataset. See [Metric Scoping](#metric-scoping). | |
There was a problem hiding this comment.
docs/index.md:251(FAQ) — "Metrics are defined at the semantic model level (not within a dataset)"docs/index.md:58anddocs/index.md:321(glossary) — same claimconverters/README.md:169— "Metrics are aggregate measures defined at the semantic model level"converters/README.md:97— the dataset property table omitsmetrics
The converter guide one matters most: an author following it writes a converter that drops dataset-scoped metrics, which is exactly the failure mode already present in all the existing converters.
|
|
||
| metrics: | ||
| - name: total_revenue | ||
| # Model-scoped: spans orders and customers via the relationship |
There was a problem hiding this comment.
The comment says "spans orders and customers via the relationship", but it applies to a metrics: list whose second entry (customer_count, line 722) is COUNT(DISTINCT customers.id), single-dataset. By the guidance at line 453 ("prefer dataset-scoped for simple aggregations that belong conceptually to one entity") it belongs on customers.metrics.
Commit 4 moved all three single-dataset metrics in the TPC-DS example for exactly this reason, so the two examples in the PR now teach opposite things. Either move customer_count onto customers here too, or scope the comment to revenue_per_customer alone.
| expression: | ||
| dialects: | ||
| - dialect: ANSI_SQL | ||
| expression: COUNT(order_id) |
There was a problem hiding this comment.
The orders dataset above declares exactly one field, amount; order_id appears only in primary_key. So the metric introduced as "unqualified reference to a field of the declaring dataset" references something that is not among the declaring dataset's fields, the example undercuts the point it is making.
Either add order_id to fields, or use COUNT(amount).
| 5. [Fields](#fields) | ||
| 6. [Metrics](#metrics) | ||
| 7. [Examples](#examples) | ||
| 7. [Metric Scoping](#metric-scoping) |
There was a problem hiding this comment.
nit: every other numbered entry (1–6, 8) maps to a ## heading, but Metric Scoping is authored as ### Metric Scoping at line 427, nested under ## Metrics. Rendered docs get a top-level entry that jumps into the middle of the Metrics section.
Either promote it to ## Metric Scoping, or drop the TOC entry and keep the in-section link at line 370.
| "items": { | ||
| "$ref": "#/$defs/Metric" | ||
| }, | ||
| "description": "Dataset-scoped metrics. Expressions must resolve entirely within this dataset and must not traverse relationships or reference fields of another dataset. Names must be unique within the dataset and must not collide with any model-scoped metric name. Referenced from outside the dataset as dataset_name.metric_name. Metrics that span datasets belong in semantic_model.metrics." |
There was a problem hiding this comment.
Let's add here that metrics and fields in the dataset share the same namespace. So the names should be unique across fields and metrics in dataset.
willpugh
left a comment
willpugh
left a comment
There was a problem hiding this comment.
I would like us to focus on the spec, before the code changes. A few issues:
1). Why not just remove the no-aggregation restriction on fields?
2) We should probably work on the semantics first. Particularly around relationship traversal
3) I'm assuming we are going to want grain locking here, but don't think you covered it.
Allow metrics to be declared on an individual dataset (datasets[].metrics) in addition to the semantic model (semantic_model.metrics), using the same metric structure in both placements. Dataset-scoped metrics are for aggregations that resolve entirely within a single dataset. They keep a metric next to the fields it depends on and make a dataset independently interpretable. Metrics that span datasets via relationships remain model-scoped. Scoping rules: - A dataset-scoped metric expression must only reference fields of its own dataset; it must not traverse relationships. - Names must be unique within their dataset. Two datasets may each declare a metric with the same local name. - A dataset-scoped name must not collide with any model-scoped metric name, keeping unqualified metric references unambiguous. - Referenced from outside the dataset as dataset_name.metric_name, mirroring how a dataset's fields are already referenced in metric expressions. Prior art: dbt MetricFlow (v1.12+) supports the same two-placement split, reserving in-model metrics for single-semantic-model metrics and top-level metrics for cross-model ones. Cube scopes measures to cubes with qualified cube_name.member references. Ossie follows Cube's scoped-uniqueness and qualified-reference convention because it matches how Ossie already treats fields. Dataset-scoped metrics reuse the existing Metric schema definition, so they inherit any future additions to the metric shape automatically. validate.py gains three checks that JSON Schema cannot express: duplicate metric names within a dataset, collisions with model-scoped metric names, and cross-dataset references in dataset-scoped expressions. Scope checking degrades gracefully for dialects sqlglot cannot parse (MDX, TABLEAU, MAQL) rather than reporting false positives. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
Add Snowflake semantic views and Databricks Unity Catalog metric views to the prior-art comparison, alongside dbt MetricFlow and Cube. Snowflake is the closest analogue: table-level metrics scoped to a logical table plus top-level derived metrics that combine metrics across tables, with qualified table.metric references. Databricks takes a different approach, with a single flat scope per metric view and joins declared inside the view. Also records that this proposal is deliberately stricter than Snowflake on scope enforcement: Snowflake permits table-level metrics to traverse relationships via using_relationships, whereas dataset-scoped metrics here must resolve within their own dataset. Notes the rationale (a strict boundary is legible and can be relaxed compatibly later) and leaves the question open for the community. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
Add AtScale SML as a fifth reference point. SML demonstrates a third placement pattern: a standalone, globally-named metric object that declares its binding by property (dataset and column are both required), with cross-entity calculations as a separate metric_calc object type. This positions the proposal between the extremes rather than at one end: SML binds a plain metric to a single column with a single aggregation method; Snowflake permits table-level metrics to traverse relationships; Ossie permits an arbitrary expression over the declaring dataset but no traversal. Also adds consumer guidance on flattening to a single metric namespace. Because a dataset-scoped expression resolves within its declaring dataset, it is already valid as a model-scoped metric, so consumers that support only model-level metrics can hoist without rewriting expressions. Notes the two real caveats: flattening requires name qualification, and consumers reading only semantic_model.metrics will not observe dataset-scoped metrics. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
Move the three single-dataset metrics in the TPC-DS example into store_sales.metrics, leaving the two that genuinely span datasets at model level. The example now demonstrates the placement decision rather than contradicting the guidance in spec.md. Dataset-scoped (expressions resolve within store_sales): - total_sales, total_profit, sales_by_brand Model-scoped (span datasets via relationships): - customer_lifetime_value (store_sales + customer) - store_productivity (store_sales + store) Referencing these three changes from total_sales to store_sales.total_sales. Also clarifies in spec.md and spec.yaml that the scoping rule constrains a metric's expression, not how it may be queried. A dataset-scoped metric can still be grouped by or filtered on dimensions of other datasets reached through relationships, since grouping dimensions are supplied by the consumer at query time. The existing sales_by_brand metric is exactly this case: its expression touches only store_sales, but its description notes it "requires grouping by item.i_brand". Without this clarification the rule is easy to misread as forbidding cross-dataset grouping, which would make the feature appear far more limited than it is. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
Incorporates review from jbonofre, christianeu-db and khush-bhatia on apache#343. Two substantive changes: - Dataset-scoped metric expressions now reference fields by unqualified name (SUM(amount), not SUM(orders.amount)), matching how a field's own expression is written. Raised by christianeu-db. - Name-collision rules are split into errors and warnings. A model-scoped metric reusing a dataset-scoped metric's name now warns instead of failing, and the warning is attributed to the model rather than the dataset, since a dataset may be authored independently and reused across models. Also reframes the placement throughout: a dataset-scoped metric aggregates data held by its dataset, and is still joined and grouped through the model's relationships like any other metric. The earlier "no traversal" wording implied a restriction on how the metric could be queried, which was wrong. Validator fixes, each reported with a reproduction: - Case-fold qualifier comparison, so SUM(ORDERS.AMOUNT) on dataset orders is no longer rejected against a dataset that does not exist - Use Column.parts[0] rather than Column.table, so STRUCT and VARIANT paths are not read as dataset references - Cross-check qualifiers against declared dataset names, so local aliases, CTEs and subquery sources are not reported as cross-dataset references - Extract a single cached _parse_expression helper, replacing two duplicated parse paths that had begun to diverge - Sort collision output for determinism under hash randomisation - Handle an explicitly null expression without raising AttributeError Spec and docs: - Metric Scoping promoted to a top-level section so its TOC entry resolves - Aggregation grain stated, and explicitly not dependent on primary_key - New rule: a dataset-scoped metric name must not collide with a field name of the same dataset - Namespace model documented, so the permitted repetitions are stated rather than left for implementations to constrain differently - docs/index.md and converters/README.md corrected; both said metrics are model-level only - Consumer guidance strengthened: ignoring datasets[].metrics is a lossy conversion, not a valid one The TPC-DS example is reverted to its state on main. No converter reads datasets[].metrics yet, so a flagship example using the placement would be lossy through every converter. Assisted-by: Cortex Code <noreply@snowflake.com>
Extends the validator test suite added in apache#330 with cases for the metric scoping and metric name checks. Each test under "reported in review" corresponds to a defect found in review of apache#343 and fails against the validator as it stood before that review: raw-cased qualifier comparison, three-part STRUCT paths read as dataset references, local aliases and subquery sources reported as cross-dataset references, a traceback on an explicitly null expression, non-deterministic collision output, and the missing field/metric name collision check. Also covers the deliberately permitted cases, so a later change does not constrain them by accident: two datasets may reuse a metric name, and a model-scoped metric may take the name of a field or of a dataset. Follows the module-loading and importorskip pattern established by the existing tests. sqlglot is skipped rather than asserted, since the scoping checks no-op without it and would otherwise pass without asserting anything. Assisted-by: Cortex Code <noreply@snowflake.com>
df8819d to
75118a7
Compare
|
Thanks all, this was a genuinely useful round. Two things changed materially as a result: the expression namespace is now unqualified, and the name-collision rules are split into errors and warnings rather than all being errors. The branch is also rebased onto current Every comment is answered below, grouped by reviewer. Verification for each claim is at the end. @jbonofreSix of these were bugs with working reproductions. All six are fixed, and each has a test that fails before the change and passes after.
Your diagnosis of why nothing caught it was the more valuable part. Added a test walking every One note on how the fix landed: adding the field needed
The
The reasoning is your own point about the flagship model being lossy through the whole hub-and-spoke on day one. Rather than fix one converter and leave six lossy, the example stays as-is until converters support the placement, which makes converter work a prerequisite for moving the example rather than a race against it. spec.md carries the worked examples meanwhile. If you would still rather see omni updated here, say so and I will do it. Unrelated, found while running the suites: @christianeu-db
It also simplified the validator, which no longer compares a qualifier against the declaring dataset's name to decide whether a reference is local. One correction it forced: the first revision claimed hoisting to a flat namespace needed no expression rewriting. It now does, and the consumer guidance says so.
This had a knock-on effect. The old rule forbidding a dataset-scoped name from colliding with a model-scoped one existed only to keep unqualified references unambiguous. Under rule 6 that justification disappears, so the constraint had to be re-argued, which is how it became a warning. The spec's prior-art table now records reference form per system, so the Tableau comparison is visible.
On the expression boundary, your layering rationale is stronger than the one I had, so I added it: letting an expression reach through relationships makes it depend on the shape of the relationship graph, which would require a dataset to reason about the datasets around it. The schema description carries the grain point too.
Your review summary also asked what can be an input to a dataset-scoped metric. Two halves: fields rather than source columns, answered above; and whether another metric can be an input, deferred as agreed. @khush-bhatia
On grain: added, with @christianeu-db's qualification that it must be computable without requiring a PK. So the spec states the grain is that of the rows the On the expression boundary: I had this framed badly in the first revision, and the wording is now fixed throughout. A dataset-scoped metric simply aggregates data held by its dataset. It is then joined and grouped exactly like any other metric, using the relationships declared in the model, so it can be sliced by dimensions of any dataset the model connects. The placement records where an aggregation is anchored; it does not fence off part of the model from the metric, and nothing that Ossie models express today becomes unexpressible. What rule 1 constrains is only the expression: it must aggregate fields of its own dataset rather than combine fields from several. An expression that genuinely needs to combine columns across datasets, such as As for why the boundary sits there, the honest answer is that it is the conservative choice rather than the obviously correct one. It makes the anchor point checkable, so a dataset and its metrics can be reasoned about or exchanged without resolving the join graph. And the asymmetry matters: relaxing this later is backward compatible, tightening it is not. Snowflake semantic views do let a table-scoped metric's own expression reach through a relationship, so the boundary between their two placements is softer than the one proposed here. If you think that is the better model, this is the place to push, and the spec records it as an open question rather than settling it. One thing I should separate out, since I previously ran these together: choosing between multiple relationship paths when more than one connects the same pair of datasets is a different problem entirely. Ossie has no mechanism for it, it affects model-scoped metric expressions and consumer queries regardless of this PR, and nothing here addresses it. I raised it separately in #342.
Not silent about it, though. The spec.yaml comment now states that composing model-scoped metrics from dataset-scoped ones is a frequently requested capability being handled separately, and the PR body records the direction rules so whichever proposal lands second does not have to reopen this one. Worth noting the gap is ambiguous rather than absent: Warn vs error, and where I would like feedbackReview made clear that not every constraint deserves the same severity, so there are now three levels. The full table is in the PR body; the short version and the open questions: Hard error where one string resolves two ways, or the expression is not self-contained: duplicate metric name in a dataset; metric name equal to a field name of the same dataset (rule 4); expression referencing another dataset (rule 1); expression qualifying a field with its own dataset name (rule 2). Warning, exit 0 for one case: a model-scoped metric reusing a dataset-scoped metric's name (rule 5). References stay unambiguous, and a dataset may be authored independently and reused across models, so it must not become invalid because of a name the surrounding model introduces. The warning is attributed to the model, and names the model-scoped metric as the thing to rename. The first revision reported it against the dataset, which asks the wrong party to change. Deliberately unchecked, now stated in spec.md rather than left silent: a model-scoped metric name equal to a field name, or to a dataset name; whether a bare name is a declared field; and qualifiers that are local aliases or CTEs. The rule 4 versus rule 5 split is the part I most want checked. The line I drew is that rule 4 prevents one string resolving two ways, while rule 5 guards against confusion between names that remain separately addressable. Four specific questions:
VerificationThe reproductions are now committed. #330 landed a validator test suite while this branch was in review, so these extend it rather than introduce one:
Four things found while verifying that are unrelated to this change and not addressed here. Each looks worth its own issue, and I am happy to open them:
One process note for anyone re-running the validator tests: they must run under |
5 participants
Summary
Adds dataset-scoped metrics: a metric may be declared on an individual dataset (
datasets[].metrics) when its expression aggregates data held by that dataset, alongside the existing model-scoped placement (semantic_model.metrics). Both placements use the identical metric structure; only the resolution rules differ.Backward compatible.
datasets[].metricsis optional and absent from every existing model, andsemantic_model.metricsis unchanged. No breaking changes to the spec.This revision incorporates review from @jbonofre, @christianeu-db and @khush-bhatia. Two things changed materially as a result:
SUM(amount), notSUM(orders.amount), matching how a field's own expression is written. Adopted from @christianeu-db.Detailed responses to every review comment are in a separate comment on this PR.
Related Issues
Discussed on
dev@ossie.apache.org([DISCUSS] Dataset-scoped metrics). Related: #287 (extended metadata, proposeshidden), #342 (shared filters, dimensions and metric references, where metric-to-metric references arise from a different direction).Scoping rules
semantic_model.metrics)datasets[].metrics)dataset.fieldfieldmetric_namedataset_name.metric_namedataset_name.metric_name.Scope describes the aggregation, not the query. A dataset-scoped metric aggregates data held by its own dataset. That is all the placement asserts. It does not limit how the metric may be queried: it is joined and grouped like any other metric, using the relationships declared in the model, so it can be sliced by dimensions of any dataset the model connects.
Fields, not source columns. Rule 1 means declared fields, not any column the
sourceexposes. Rule 1 places no limit on the complexity of the aggregation.Warn vs error, and a request for feedback
Not every constraint here deserves the same severity, so the validator distinguishes three levels.
Hard error, where one string resolves two ways or the expression is not anchored to one dataset:
dataset.nameresolves two waysorders.amountresolves two waysWarning, exit code 0:
The rule 5 warning is attributed to the model, and names the model-scoped metric as the thing to rename.
Deliberately not checked, and now stated in spec.md so implementations do not add divergent constraints:
revenuevsorders.revenue). Also the common case, since a model-scoped metric usually takes the name of the column it aggregatesFour questions I would like answered:
SUM(orders.amount)inside datasetordersis valid, unambiguous SQL, so erroring enforces a spelling. The case for erroring is that it makes the expression depend on the dataset's name in the model.Prior art
Systems differ in where a single-entity aggregation lives, how names resolve, and how strictly scope is enforced. Listed alphabetically.
metricbound to onedataset+columnmetric_calcobjectunique_namecube.memberMEASURE(name)metricstables[].metricsmetrics(derived)table.metricFour of the five distinguish the two structurally. Ossie has so far provided only the model-level placement, which is the gap this addresses.
Databricks UC metric views are closest to the namespace rules here: one
sourceplus optionaljoins, sources named so a column issource_name.column_name, with the view itself exposing a flat schema. Snowflake semantic views let a table-scoped metric's own expression reach through a relationship, so the boundary between their placements is softer than rule 1.Scoped uniqueness with qualified references, rather than the flat global namespace used by AtScale SML and dbt MetricFlow, follows Ossie's existing convention: field names are already scoped to a dataset. Rule 2 follows the same convention.
Rule 1 is the conservative choice: relaxing it later is backward compatible, tightening it is not. Whether a dataset-scoped expression should be permitted to reach through an explicitly declared path is left open. Choosing between multiple relationship paths, where more than one connects the same pair of datasets, is a separate gap Ossie does not currently address.
Related open question: can a metric expression reference another metric?
Raised by @khush-bhatia and seconded by @christianeu-db as belonging in a separate proposal, which I agree with: it affects the expression language rather than metric placement.
Recorded here so the two stay compatible. The gap is ambiguous rather than absent: because expressions are raw SQL strings,
SUM(orders.amount) / total_ordersparses today withtotal_ordersread as an unqualified column. If metric references are added, the direction rules follow from rule 6:A dataset-scoped metric referencing a model-scoped one would break rules 1 and 2, so that case should be disallowed.
The TPC-DS example, and converter support
The first revision moved three single-dataset metrics in the flagship example to
store_sales.metrics. @jbonofre showed this broke two omni converter tests, and that all converters read model-level metrics only, so converting the example would silently drop three metrics.The example is reverted here, byte-identical to
main, and no converter is touched. Shipping a flagship example that is lossy through the whole hub-and-spoke is worse than shipping the placement without a flagship demonstration of it, and it makes converter support a prerequisite for moving the example rather than a race against it. TheMetric Scopingsection of spec.md carries the worked examples meanwhile.Directly implied by this change, and proposed as follow-ups rather than done here:
iter_metrics(model)helper in the spec tooling yielding both placements with qualified names, so converters get this right by default rather than each reimplementing itdatasets[].metrics, after which the example can move.converters/polariscarries the same latent gap the Pydantic model did: itsDatasetclass has nometricsfield and its parser reads model-level metrics onlyOut of scope: findings noted while verifying
None of the following is addressed in this PR, and none of it is caused by this change. Recording it because it came up while verifying, and each looks worth its own issue.
No CI workflow covers the paths this PR touches. Every workflow is path-filtered to
cli/**orconverters/<name>/**. Nothing watchescore-spec/,validation/,python/,docs/orexamples/, so a PR touching only those paths runs no jobs and still shows a green check. This now also affects the validator suite introduced in #330, which no workflow executes. A workflow covering those paths would want to run the validator againstexamples/, thepythonsuite, andvalidation/tests/. I have deliberately not added one here so this PR stays a specification change.converters/databrickshas a pre-existing test failure.test_roundtrip_properties.py::TestMetricViewRoundTrip::test_mv_to_ossie_to_mv. Hypothesis generates a metric view whosesourcetable and one of its joins share the namej0, which trips the converter's own duplicate-name guard. Confirmed reproducible on unmodifiedmainby stashing this branch's changes.converters/wisdomhas no CI workflow and no committeduv.lock, unlike every other converter, so its tests never run in CI.Changes
Spec:
core-spec/spec.md:Metric Scopingpromoted to a##section so the TOC entry resolves; rules rewritten; grain, namespace and fields-vs-columns stated; examples corrected; prior art and consumer guidance updatedcore-spec/spec.yaml: dataset and model metric blocks documentedcore-spec/ossie-schema.json:Dataset.metricsadded; both metric descriptions rewrittenTooling:
validation/validate.py: one shared_parse_expressionhelper replacing two duplicated parse paths, with caching; scoping check rewritten; field/metric collision check added; rule 5 emitted as a warningvalidation/tests/test_validate.py: 18 cases added to the suite introduced in Warn when relationship to_columns does not cover a declared key #330, covering the metric scoping and metric name rules. Follows the existing module-loading andimportorskippattern; no new packaging filespython/src/ossie/models.py:OssieDataset.metricsaddedpython/tests/test_models.py: new test walking schema$defsto catch schema/model driftDocs:
docs/index.md(3 places) andconverters/README.md(2 places, including the dataset property table): all previously stated that metrics are model-level onlyExample:
examples/tpcds_semantic_model.yaml: unchanged frommainValidation
Rules 1 to 4 are semantic constraints JSON Schema cannot express and are enforced in
validate.py. Rule 5 is emitted as a warning. Rule 6 is a resolution rule for consumers rather than a check.Verified locally, since no CI workflow covers the paths this PR touches:
validation/tests/test_validate.py: 28 tests, the 10 from Warn when relationship to_columns does not cover a declared key #330 plus 18 added here. Confirmed non-vacuous by reverting onlyvalidate.pyto the pre-review commit, where 10 of the 18 new cases fail and all 10 pre-existing ones still passpython: 10 tests. The schema/model drift test was likewise confirmed to fail against a simulated pre-fix modelmainexamples/tpcds_semantic_model.yamlvalidates against the core schema;examples/flights.yamlagainst the ontology schemaChecklist
Specification
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changes. None required:ontology.jsonreferencesSemanticModelby$refto the published schema, so it picks upDataset.metricswithout changeConverters
converters/is updated to reflect spec or ontology changes. Deliberately not done, see "The TPC-DS example, and converter support" above. No converter readsdatasets[].metrics; because the example is unchanged, no converter regresses, and updating them is proposed as a follow-upValidation
validation/are updated if the spec changedvalidation/tests/test_validate.py, 28 total in that fileDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changed. Not applicableExamples
examples/are added or updated for any new spec constructs or converter support. Deliberately not done, so the flagship example is not lossy through converters that do not yet read the new placement. Worked examples live in theMetric Scopingsection of spec.mdTests
pytest/ CI green). Verified locally; note that no CI workflow covers the paths this PR touches, so a green check on this PR would mean no jobs ranCompliance
AI disclosure
Per the ASF Generative Tooling Guidance, this contribution was prepared with AI assistance. All specification decisions, the scoping rules, and the responses to review are mine. I have reviewed and verified every change, and the verification results above were produced by running the suites rather than asserted.