Repository navigation
docs(core-spec): Let model-scoped metrics evaluate dataset-scoped metrics with AGG() - #703
justint-db wants to merge 1 commit into
Conversation
56fd48f to
e19b363
Compare
kayemkim
left a comment
There was a problem hiding this comment.
Two things from running the validator on the branch.
The header still pins sqlglot>=30.12.0, but exp.Agg only exists from sqlglot 30.19.0. With 30.18.0 installed, every document ends in AttributeError while validating …: module 'sqlglot.expressions' has no attribute 'Agg', examples/tpcds_semantic_model.yaml included, because validate_agg_placement and validate_metric_references look for exp.Agg whether or not the text calls it. CI installs the latest sqlglot, so it does not show there; bumping the header to >=30.19.0 closes it.
The checks also reach other dialects. A SNOWFLAKE field expression AGG(x) and a SNOWFLAKE dataset-scoped metric AGG(total_usd) now fail with the [Scope] error, and a SNOWFLAKE model-scoped metric gets the argument and FILTER warnings, while expression_language.md keeps "Expressions in other dialects are not affected" and the new MUST NOT is scoped to OSSIE_SQL_2026. Both places have expression.dialect in hand, so skipping the AGG-specific messages when it is not PORTABLE_DIALECT would match the text.
Two smaller notes. A dataset-scoped metric AGG(order_revenue) is reported twice, first by validate_metric_scoping as an undeclared field order_revenue and then by the new check, and the first message points the wrong way. And AGG(orders.total_usd), AGG(DISTINCT orders.order_revenue), AGG(*) and the FILTER form each get a [Reference] warning and the document passes, while AGG in a field expression is an error, although the spec says MUST NOT for both; an observation, not a request.
The accepted shapes behave as written: agg(...), AGG(Orders.ORDER_REVENUE), the quoted form, AGG inside CASE and COALESCE and the dialects entry form pass clean, and a bare orders.order_revenue gets the new "write AGG(...)" warning. The branch conflicts with #698 and #696 in validate.py, so whichever lands second needs a rebase.
e19b363 to
7b26364
Compare
…rics with AGG() A model-scoped metric may now evaluate a dataset-scoped metric with AGG(dataset.metric), as in AGG(orders.total_revenue) / COUNT(DISTINCT customers.customer_id). The argument must be a single dataset.metric reference, and the call takes no FILTER clause. Bare metric references, AGG of a field, and AGG of a model-scoped metric remain invalid. AGG stays disallowed everywhere else. Layer 3 lowers an AGG(D.m) call as one component with home dataset D. The validator accepts AGG(dataset.metric) in model-scoped metrics and reports a misused AGG call as an error, as it does AGG in a field expression or a dataset-scoped metric. No model written before AGG was allowed can contain one. A bare metric reference stays a warning, like the other metric reference findings, with a hint toward AGG. These AGG checks apply only to OSSIE_SQL_2026, as the spec does. They need sqlglot 30.19.0, the first release with exp.Agg, so the minimum version is raised. Co-authored-by: Isaac <no-reply@databricks.com>
7b26364 to
c9844ba
Compare
|
Thanks @kayemkim, this was a really useful review. I've pushed c9844ba:
|
kayemkim
left a comment
There was a problem hiding this comment.
All four are in as described, so the validation side is good from me. On the merged tree the suite passes on sqlglot 30.19.0, the Snowflake cases read exactly as they do on main, a dataset-scoped AGG(order_revenue) gives the one [Scope] line, and each misuse in a model-scoped metric now fails the document. Moving those to errors changes no existing document: the examples, the compliance cases and the converter fixtures all come out the same as on main.
On ordering, this conflicts with #696 and #704 in validate.py. With #696 it is the two column loops, where your id() skip has to move into the iter_columns(tree, expression.dialect) loop that PR introduces, so whichever lands second needs a small rebase.
christianeu-db
left a comment
There was a problem hiding this comment.
Thanks @justint-db! While the dev@ lazy consensus runs until Friday, I flagged one potential text update that would be nice to address before merging
| Ossie SQL reserves the function name `AGG` for evaluating a measure: a column that holds an aggregate calculation instead of a value for each row. The [Relational Query Interface](relational_semantics.md) defines measures (Section 4) and `AGG` (Section 6), including its valid arguments. In `OSSIE_SQL_2026` expressions, an implementation MUST NOT give `AGG` any other meaning. Expressions in other dialects are not affected. Because function names should be case insensitive, the reservation covers every capitalization, such as `agg` and `Agg`. | ||
|
|
||
| This version reserves the name only. `OSSIE_SQL_2026` expressions MUST NOT call `AGG` until a future version defines where model declarations may use it. | ||
| A model-scoped metric may use `AGG` to evaluate a dataset-scoped metric, as in `AGG(orders.total_revenue)`. The argument is a single `dataset.metric` reference, and the call takes no `FILTER (WHERE ...)` clause (see [Metric Scoping](spec.md#metric-scoping)). `OSSIE_SQL_2026` expressions MUST NOT call `AGG` anywhere else until a future version defines it. |
There was a problem hiding this comment.
Layer 2 Section 6 makes AGG an aggregate function, and the FILTER (WHERE ...) section above says every aggregate function supports FILTER, while rule 6 forbids FILTER on AGG. Could that section name AGG as the exception, or this paragraph say that it overrides that section? Not a blocker.
Summary
A model-scoped metric can now evaluate a dataset-scoped metric with
AGG(dataset.metric):This is the first place model declarations may use
AGG, which #662 reserved for a future version.Rules (
spec.mdMetric Scoping rule 6 and Name Resolution,expression_language.mdName Spaces and Measure Evaluation:AGG)dataset.metricreference to a dataset-scoped metric.FILTER (WHERE ...)clause.orders.total_revenue,AGGof a field (fields are never measures), andAGGof a model-scoped metric.AGGstays disallowed everywhere else, including field expressions and dataset-scoped metrics.Within a dataset, field and metric names share one namespace, so
orders.total_revenuealways names exactly one thing.Deferred: model-scoped metrics referring to other model-scoped metrics,
FILTERonAGG, andAGGover a source measure in dataset-scoped metrics.Layer 3 (
wide_table_semantics.md§5.2): one sentence saying that anAGG(D.m)call is one component with home datasetD, evaluated as the dataset-scoped metricD.m. This matches how §5.2 already treats a dataset-scoped metric queried directly.Validator (
validation/validate.py)AGG(dataset.metric). It warns on a bare metric reference (suggesting theAGGform),AGGof a field or a model-scoped metric, an argument that isn't a single qualified reference, an unknown dataset or metric, andAGGwithFILTER. These are warnings, like the rest of that check.AGGin a field expression or a dataset-scoped metric as an error, since the spec says MUST NOT.spec.yamlandossie-schema.jsondescriptions are updated to match.Related Issues
Follows #662 (reserve
AGG), #633 (source columns), and #653 (Layer 3).Checklist
Specification
core-spec/and follow the existing structureValidation
validation/are updated if the spec changedTests
validation/tests/: 579 passed;python/tests/: 269 passed)This pull request and its description were written by Isaac.