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.
There was a problem hiding this comment.
Proposal for data set metrics -- SUM(col_name) references a source column. SUM(dataset.field_name) can reference a field in the same data set.
There was a problem hiding this comment.
Having differentiation makes a lot sense. Let's chat at the working group as well. This one is quite tricky to get right. For metric views, we have:
- source.field: from the table
- field: a dimension (if it exists), otherwise it's from source
The wrinkle is that you could have a struct-typed column called source so that source.field is ambiguous. This been a tricky area for development.
To be honest, 1 pager on the name resolution precedence rules (e.g. source, dataset, struct, model) could be valuable in of itself.
| **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
Rule 1 originally read "a dataset-scoped metric's expression MUST NOT reference a field of another dataset", justified on the grounds that such a metric could then be exchanged without resolving the surrounding join graph. That justification is a claim about evaluation, and this specification does not define evaluation semantics: there is nothing in core-spec/ about how a metric is computed, how grain is resolved, or how fan-out is handled. Constraining authors on the strength of a benefit the spec never specifies overreaches. Raised by willpugh in review. Restating it as a placement rule removed the overreach but left the rule redundant. It is entailed by the unqualified-reference rule: an expression restricted to unqualified field names cannot reach another dataset, because a cross-dataset reference requires a qualifier. The rule stated a consequence and then needed two paragraphs to walk back the evaluation reading it invited, so it is now dropped and the consequence folded into rule 1 as a single clause. The remaining rules are renumbered 1 to 5. Also drops the aggregation grain paragraph and the corresponding comparison table row, for the same reason the rule 1 rationale went: grain is an evaluation concept, and this spec defines structure and naming rather than evaluation semantics. No check changes behaviour. Both previously documented INVALID snippets are still rejected; they are now both rule 1 violations. Assisted-by: Cortex Code <noreply@snowflake.com>
Matches the rest of spec.md, which uses one em dash in total. Assisted-by: Cortex Code <noreply@snowflake.com>
Rule 1 said a dataset-scoped metric's expression MUST reference fields of its declaring dataset. That forces an author to declare a field for a column they only want to aggregate, and then hide it. The expression is written against the dataset's source and references its columns, so nothing needs declaring first. This is not a new rule: it is how a field's own expression already works. The spec declares a field customer_id with expression customer_id, which refers to the source column rather than to the field itself, since referring to the field would be circular. That also settles a resolution question the old wording left open. A declared field does not shadow a source column of the same name, so if a dataset declares a field foo whose expression is not simply foo, then SUM(foo) in a metric of that dataset still refers to the source column. Reusing a declared field's expression inside a metric is the same composability question as a metric referencing another metric, and is left to a separate proposal. No check changes behaviour. validate_metric_scoping keys on dataset names and never on whether a leaf is a declared field, so permitting undeclared columns costs nothing mechanically. Two tests lock the documented behaviour in; both also pass against the previous validator, because it did not check field declarations either. Assisted-by: Cortex Code <noreply@snowflake.com>
Rule 1 said a dataset-scoped metric's expression references fields of its dataset. That forces an author to declare a field for a column they only want to aggregate, and then hide it. It also narrowed something the spec already states: the Fields section describes fields as "row-level attributes that can be used for grouping, filtering, and in metric expressions". The expression now reaches both namespaces, with a distinct spelling for each. A declared field is written dataset_name.field_name, which is how a metric reuses a field's expression instead of repeating it. A column of the source is written unqualified. Two spellings rather than one shared namespace avoids a shadowing rule. A field and a source column may share a name without ambiguity, so declaring a field named after an existing column does not change the meaning of an expression already using the bare name. This replaces the hard error on self-qualification, since orders.amount now means the declared field amount, with a check that a qualified reference names a declared field of the declaring dataset. That is verifiable from the model, so SUM(orders.tax) is reported and points the author at SUM(tax). Whether a bare name is a real column stays unchecked, because it needs catalog metadata the model does not carry, and nothing else in validate.py checks a field's expression against real columns either. validate.py: _leading_qualifiers becomes _qualified_references, returning (qualifier, name) pairs so the name a qualifier introduces can be resolved against the dataset's field list. spec.md: the worked example now demonstrates both spellings on a derived field rather than two identity fields, and the second INVALID case is a qualified reference to an undeclared name instead of a self-qualifier. Tests: 21 functions added overall, collecting 32 cases with the 10 from apache#330. Reverting only validate.py to 592db69 fails 11 of the added functions and none of the 10 pre-existing ones. Assisted-by: Cortex Code <noreply@snowflake.com>
|
Replying to each review comment against the current revision. The PR description is rewritten too. @christianeu-db
@jbonofre
@khush-bhatia
@willpughYour three points are the open design questions, so they are in the comment below. |
|
@willpugh, your three points. 1. Why not remove the no-aggregation restriction on fields?Depends on whether the aggregate is marked, and I think only the marked version is viable. Unmarked. If a field may hold A fields:
- name: total_amount
expression:
dialects:
- dialect: ANSI_SQL
expression: SUM(amount)
metric: {}Two things favor it. It reuses the existing pattern for declaring a field's role rather than adding a node. And it makes rule 3 unnecessary, since fields and metrics would share one list and a collision becomes structurally impossible rather than something a validator catches. Declaration and query surface are separate questions, which is a distinction the Metrics Working Group has been drawing too. A marker on a projected expression answers how a consumer queries an aggregate. A separate list answers how the model declares it. Systems that mark aggregates in SQL still keep dimensions and measures as separate lists in their metadata. Ossie's Two against.
The bigger one is the eleven existing converters, because it inverts the failure mode. With a separate Silently wrong is worse than silently absent. That is the point I would want resolved before choosing the marker shape, and if you think the converter risk is manageable I would rather hear the argument than treat it as decisive. 2. Semantics first, particularly relationship traversalI do not think this blocks, because the proposal adds no evaluation semantics to a spec that defines none. There is nothing in Traversal specifically: nothing here restricts how a metric may be queried. A dataset-scoped metric is joined and grouped like any other through the model's relationships. The unsettled question is that Ossie has no mechanism for choosing between multiple relationship paths when more than one connects the same pair of datasets. That affects model-scoped expressions and consumer queries today, independently of this change, and I raised it in #342. Where the door does close is the naming and namespace rules, since tightening a name rule later invalidates models that were valid before. That is where most of the review has gone, and rule 4 is deliberately a warning rather than an error for exactly that reason. If you see a way this forecloses an evaluation design, that is worth blocking on and I would rather hear it now than after it merges. 3. Grain lockingNot covered. Pinning a metric to a grain independent of the query would be a property of a metric, applying identically to both placements since they share the On your opening point about spec before code: the |
RE 1: Josh - those are good points around the limit of parsing for resolving types / performing validation. This could be important if we extend Ossie to consume metrics from inside a source (e.g. an Ossie dataset directly points at a semantic view which meets the single-table invariants, or a metric view). Then, an expression: measure_name could more easily differentiated as someone correctly importing in a measure v. someone accidentally trying to use a measure-typed column as a dimension. |
5 participants
Summary
A metric may now be declared on a dataset (
datasets[].metrics) as well as on the model (semantic_model.metrics). Both placements use the sameMetricstructure; only name resolution differs.datasets[].metricsis optional and absent from every existing model, andsemantic_model.metricsis unchanged. Nothing breaks.Discussed on
dev@ossie.apache.orgas[DISCUSS] Dataset-scoped metrics. Related: #287, #342.What this adds
Today every metric lives at the model level, so an aggregation over a single dataset has to be written as though it could span several, and named uniquely across the entire model.
Declaring it on the dataset gives three things the current spec cannot express:
revenue. Today the second one has to be renamed.Scoping rules
semantic_model.metrics)datasets[].metrics)dataset.fielddataset.field; columns of itssource, unqualified:columnmetric_namedataset_name.metric_namesource. A declared field is writtendataset_name.field_name; a source column is written unqualified. A qualified reference MUST name a declared field of the declaring dataset. An expression that references another dataset MUST be declared insemantic_model.metrics.dataset_name.metric_name.Two spellings, one per namespace.
SUM(orders.net_amount)references the declared field, which is how a metric reuses a field's expression instead of repeating it.SUM(tax)references a column of thesourcethat is not declared as a field. Because the spellings differ, a field and a column may share a name without ambiguity, so declaring a field named after an existing column does not change the meaning of an expression already using the bare name.Placement constrains the expression, not the query. A dataset-scoped metric is joined and grouped like any other, using the model's relationships, so it can be sliced by dimensions of any dataset the model connects.
The spec defines structure and name resolution only. It says nothing about evaluation, grain or traversal, and this proposal does not change that.
Errors, warnings, and what is left alone
Hard error, where one string resolves two ways or a reference cannot resolve at all:
dataset.nameresolves two waysorders.amountresolves two wayssemantic_model.metricsorders.taxasks for a field the model does not declareWarning, exit code 0: a model-scoped metric reusing a dataset-scoped metric's name. 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.
Not checked, and stated in spec.md so implementations do not add divergent constraints: a model-scoped metric sharing a name with a field or a dataset, since those are spelled differently; whether a bare name is a real column, since that needs catalog metadata the model does not carry; and a qualifier naming neither a dataset nor a field, since it is a local alias, CTE or subquery source.
Open questions
SUM(orders.tax)is valid SQL iftaxis a column, so erroring enforces Ossie's reading of the qualifier over SQL's.Prior art
Most semantic layers already distinguish the two placements. Listed alphabetically.
metricbound to one datasetmetric_calccube.memberMEASURE(name)metricstables[].metricsmetricstable.metricScoped uniqueness with qualified references, rather than the flat global namespace AtScale SML and dbt MetricFlow use, follows Ossie's existing convention that field names are scoped to a dataset.
Whether some metrics belong at the dataset level rather than above datasets and relationships is an open question in the Ossie Metrics Working Group as well, argued there from the evaluation side: a metric over one dataset has a fixed dimension domain, while one spanning datasets is evaluated on different join paths depending on the filters and dimensions in play. That reasoning lands on the same boundary this proposal draws structurally.
Not included
The TPC-DS example is unchanged. No converter reads
datasets[].metricsyet, so moving the example's metrics would silently drop three of them on conversion. Shipping a lossy flagship example is worse than shipping the placement without one; the worked examples live in theMetric Scopingsection of spec.md meanwhile. Proposed as follow-ups: a sharediter_metrics(model)helper so converters get both placements right by default, then updating the converters, after which the example can move.converters/polariscarries the same gap the Pydantic model did: itsDatasetclass has nometricsfield.No CI workflow covers the paths this PR touches. Every workflow is filtered to
cli/**orconverters/<name>/**, so nothing runs againstcore-spec/,validation/,python/,docs/orexamples/, and a green check here means no jobs ran. This also affects the validator suite added in #330. Left alone so this PR stays a specification change.Run locally instead: the validator suite (31 test functions, the 10 from #330 plus 21 added here), the
pythonsuite, and 9 of the 11 converter suites. Two Java converters need a JRE I do not have. The onedatabricksfailure is a pre-existing Hypothesis case reproducible on unmodifiedmain. Both examples validate, every fenced YAML block in spec.md parses, and the two snippets spec.md marks INVALID are confirmed rejected by the validator.Changes
core-spec/spec.md:Metric Scopingpromoted to##so the TOC entry resolves; rules, namespace model and worked examples addedcore-spec/spec.yaml,core-spec/ossie-schema.json:Dataset.metricsadded and both metric descriptions documentedvalidation/validate.py: scoping and field/metric collision checks added; one shared expression-parsing helper with caching replaces two duplicated parse pathsvalidation/tests/test_validate.py: 21 test functions added to the suite from Warn when relationship to_columns does not cover a declared key #330python/src/ossie/models.py:OssieDataset.metricsaddedpython/tests/test_models.py: test walking schema$defsto catch schema/model driftdocs/index.md,converters/README.md: corrected, both stated that metrics are model-level onlyChecklist
Specification
core-spec/and follow the existing structureOntology
ontology.jsonreferencesSemanticModelby$ref, so it picks upDataset.metricswithout changeConverters
datasets[].metrics; because the example is unchanged, none regressesValidation
validation/are updatedDocumentation
docs/is updated to reflect user-facing changesCONTRIBUTING.mdupdated if the process changed. Not applicableExamples
examples/updated for new spec constructs. Deliberately not done, so the flagship example is not lossy through converters that do not yet read the new placementTests
Compliance
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 were produced by running the suites rather than asserted.