Repository navigation
feat(validation): check metric references and field shadowing against Name Resolution - #608
Conversation
| anonymous = isinstance(parent, exp.Anonymous) and parent.name.upper() in DATE_PART_FUNCTIONS | ||
| if column.table or not (isinstance(parent, typed) or anonymous): | ||
| return False | ||
| units = exp.TimeUnit.UNABBREVIATED_UNIT_NAME |
There was a problem hiding this comment.
exp.TimeUnit.UNABBREVIATED_UNIT_NAME has no MONTH. Its keys are single letters (D, H, M, Q, ...) and its values are DAY, HOUR, MINUTE, QUARTER, WEEK, YEAR, etc. So DATEDIFF(month, orders.a, orders.b), DATEADD(month, 1, orders.a) and TIMESTAMPDIFF(MONTH, ...) all warn "'month' is not qualified". DATE_PART(month, ...) warns the same way on DATABRICKS, and so do dd and days.
sqlglot already has the full alias table in Dialect.DATE_PART_MAPPING:
| units = exp.TimeUnit.UNABBREVIATED_UNIT_NAME | |
| parts = Dialect.DATE_PART_MAPPING | |
| return column.name.upper() in parts or column.name.upper() in parts.values() |
This needs from sqlglot.dialects.dialect import Dialect next to the other sqlglot imports.
Could we also add tests for month, dd and days? The current tests only use day, MINUTE and year.
There was a problem hiding this comment.
Done in 8c29e26: Dialect.DATE_PART_MAPPING, and only in the unit position, since that table also has q, w and ms; the Snowflake argument order is handled too. month, days, dd and DATE_PART(month) are in the tests.
| tree = parse_sql(expr, dialect) | ||
| if tree is None: | ||
| continue | ||
| names = [c.name for c in tree.find_all(sqlglot.exp.Column) if not c.table] |
There was a problem hiding this comment.
This collects every unqualified column without the is_date_part filter that validate_metric_references applies.
Take a dataset with a field day (EXTRACT(DAY FROM order_date)) and a field days_open (DATEDIFF(day, opened_at, closed_at)). It warns "'day' is the source column, not the field 'day' ...". Date dimensions often have fields named day, month or year.
| names = [c.name for c in tree.find_all(sqlglot.exp.Column) if not c.table] | |
| names = [ | |
| c.name for c in tree.find_all(sqlglot.exp.Column) | |
| if not c.table and not is_date_part(c) | |
| ] |
For month it depends on the is_date_part fix above.
There was a problem hiding this comment.
Done in 8c29e26, with a day / days_open case.
| f"'{column.name}' is not qualified; metric expressions refer to fields " | ||
| f"as dataset.field" | ||
| ) | ||
| elif column.table not in fields: |
There was a problem hiding this comment.
Names are compared exactly here. The same happens in undeclared(), other_metrics, by_name and passes_through. But expression_language.md (in "Namespacing and Identifier Resolution") says regular, unquoted identifiers are case-insensitive: id matches Id and ID.
As written, SUM(ORDERS.TOTAL_USD) over dataset orders warns "references unknown dataset 'ORDERS'". And Total_USD * 2 next to a field total_usd misses the shadowing this check is meant to catch.
Could we compare normalized names everywhere? For example:
def fold(name: str) -> str:
"""Regular identifiers are case-insensitive (expression_language.md)."""
return name.upper()Then apply fold() to both sides of the dataset and field lookups, the metric-name set, by_name and passes_through. The spec leaves quoted versus unquoted comparison to each database, so folding everything seems the safer choice for a warning, and it keeps "amount" working as a pass-through.
There was a problem hiding this comment.
Done in 8c29e26: fold() on both sides of every lookup; messages keep the spelling as written.
| assert _field_reference_findings(document) == [] | ||
|
|
||
|
|
||
| def test_pass_through_compares_names_as_written() -> None: |
There was a problem hiding this comment.
Under the spec's case rule, a field amount defined as AMOUNT is a pass-through, so this test should expect no finding.
Could we flip it? Also we could add a case where the casing differs and a warning is still expected, such as Total_USD * 2 next to a field total_usd.
There was a problem hiding this comment.
Flipped in 8c29e26, and Total_USD * 0.9 next to total_usd is the case that still warns.
| return errors | ||
|
|
||
|
|
||
| def declared_fields(model: dict) -> dict[str, set[str]]: |
There was a problem hiding this comment.
I guess we have to merge #486 first to be consistent here.
|
Pushed a commit for the first four; the fifth waits for #486.
300 tests; the validation workflow passes locally on current main. |
|
Once this rebases onto #591, datasets:
- name: devices
source: devices
fields:
- name: last_contact_at
expression: {dialects: [{dialect: ANSI_SQL, expression: last_contact_at}]}
metrics:
- name: minutes_since_last_contact
expression: {dialects: [{dialect: ANSI_SQL, expression: "DATEDIFF(MINUTE, MAX(last_contact_at), CURRENT_TIMESTAMP)"}]}On main (7205e15) with sqlglot 30.24.0: With |
|
Reproduced on main, and it belongs here rather than in a follow-up: the scoping walk and the reference checks read the same columns, so they should skip the same units. With One thing the filter does not cover, in either check: under the default parser the third argument lands in |
8c29e26 to
ed11583
Compare
|
Rebased on main with #617 and #486 in; the two reviewed commits are unchanged apart from the renamed dialect sets, and two commits follow. The first moves the three checks onto The second is the fifth item from the first round and @ekut's report: From the sweep: the three |
|
I ran this branch (ed11583) against a model a code generator wrote rather than one written by hand: the ElastiCORE
Warnings first looks right from here: the one hit in a real generated model would have failed a CI that treated it as an error before the generator fix ships. |
… Name Resolution spec.md "Name Resolution" says a model-scoped metric names fields as dataset.field, and that a bare name in a field expression is a column of the dataset's source. validate_references already holds keys and relationship columns to declared fields; nothing checked the expressions. - validate_metric_references warns when a metric column is unqualified, names another metric, has more than two parts, or names an unknown dataset or an undeclared field, once per expression. - validate_field_shadowing warns when a field expression's bare name is another field of the same dataset, unless that field passes the same-named column through or has no SQL definition to compare. Both read expressions through read_expressions (the top-level `sql` as OSSIE_SQL_2026, a superseded entry skipped) and the SQL check's parser, so a dialect it does not parse is skipped, and read columns through iter_columns: `*` and the unit of a date function are no reference, and the unit slot the default parser fills with the last DATEDIFF or DATE_ADD argument is read back as that column. _expressions_in now skips shapes the schema rejects instead of failing on them, for direct calls. examples/renamed_fields.yaml is the spec's example as a complete model, validated in CI. Generated-by: Claude Code
ed11583 to
582a991
Compare
|
Rebuilt on main now that #649 is in. It covers keys and relationship columns as errors, so I dropped that check and kept the other two from #602, model-scoped metric references and field shadowing, in one commit that compares names with Both checks are still warnings. Now that keys are errors, should metric references follow? In this tree they would fail only the three |
jbonofre
left a comment
There was a problem hiding this comment.
LGTM!
I checked out the branch and ran validation/tests and test_validate.py.
renamed_fields.yaml and tpcds_semantic_model.yaml both validate cleanly.
I also tried a few likely false positive cases against the new reference check: DATE_TRUNC(month, ...) (Snowflake and ANSI), TIMESTAMP_TRUNC(..., month) (BigQuery), DATEDIFF(day, ...), and a Databricks lambda. None of them warned. A dataset scoped metric that references a source column (SUM(o_totalprice)) is still rejected by the existing [Scope] check.
A metric expression cannot refer to other metrics (core-spec/spec.md, Metric Scoping; expression_language.md), and since apache#608 validate.py warns when one does. M5's composite, avg_order_value = total_amount / order_count, referenced the two metrics by name. The "Referenced as metric_name" row in the Metric Scoping table is how a query addresses a metric, not what an expression may contain. M5 now carries the composition the way a converter has to emit it when the source tool composes measures: the parts inlined, over qualified dataset.field references, SUM(sales.amount) / NULLIF(COUNT(sales.order_id), 0), NULL when there are no orders. The two base metrics stay alongside it. The fixture validates with no warnings on main's validator, and its primary reference form is qualified again. Co-authored-by: Isaac <no-reply@databricks.com>
Picks up apache#595 and apache#608, which append tests to validation/tests/test_validate.py beside this branch's dialect tests; both sets kept.
…grade them Since apache#486 a model-scoped metric must reference fields as dataset.field, and since apache#608 validate.py warns on the bare variants of M0, M2 and M3 ("is not qualified"). Metric Scoping in core-spec/spec.md gives the bare form its place: a dataset-scoped metric (datasets[].metrics) references its own dataset's fields by single-part name. The four bare fixtures now carry their metrics there, and validate without warnings on main. The grader read only the model-root metrics, so a dataset-scoped metric was not graded at all, and the identity test still passed. It now collects datasets[].metrics too, keyed dataset.metric as the spec references them, and the identity test asserts that every metric in a fixture is graded. New tests: a dataset-scoped metric graded PORTABLE, dropped, and moved to the model (REWRITTEN, identity not preserved). All four fail without the grader change. Co-authored-by: Isaac <no-reply@databricks.com>
Summary
The Name Resolution section of spec.md says a model-scoped metric names fields as
dataset.fieldand a bare name in a field expression is a column of the dataset'ssource. Since #649,validate_referencesholds keys and relationship columns to declared fields; nothing checks the expressions, so a metric readingSUM(orders.o_totalprice)against a field calledtotal_amountstill passes. This adds the other two checks #602 asks for.validate_metric_referencesreports a metric column that is unqualified, names another metric, names an unknown dataset or names an undeclared field, each told apart in the message.validate_field_shadowingreports a field expression whose bare name is also the name of another field in the same dataset, where the author may have meant the field but the spec reads the source column. That one stays quiet when the other field is the same-named column passed through, read from its parsed definition, since both readings then agree, when it has no SQL definition to compare, and when a field names its own source column, as in the spec'samountexample. Dataset-scoped metrics are left tovalidate_metric_scoping, which #633 is changing. Names are compared case-insensitively withcasefold, likedataset_field_names.The checks read expressions through
iter_expressionsand_parse_sql_expression, likevalidate_sql: a top-levelsqlis checked as OSSIE_SQL_2026, an OSSIE_SQL_2026 dialect entry that asqlsupersedes is skipped, a dialect outsideSQLGLOT_DIALECTSis left to the unsupported-dialect warning, and an expression that does not parse is left to the SQL check. Columns are read throughiter_columns:*and the unit of a date function reference no field, sodayinDATEDIFF(day, a, b), which the default parser reads as a column, is not reported. The same parser leaves the lastDATEDIFForDATE_ADDargument inunit, as a Var when it is bare; when the written unit sits inthisas a column and that Var is an identifier that is not a date part,iter_columnsreads it back as the column it is.Both are warnings. The three portability fixtures written in the bare form (
m0_sum_bare,m2_aggregates_bare,m3_filtered_agg_bare) get one per bare metric reference, nine in all, and still pass;compliance/portability/README.mdcalls the bare form a sibling variant pending the name-resolution decisions. The shadowing check reports nothing in the tree.examples/renamed_fields.yamlis the spec example as a complete model: every field name differs from its source column, and validation-ci validates it.Verification
validation/tests/: 368 passed, 76 of them new (main: 292), also in random order, one of them through the CLI.validation/test_validate.pyOK. The validation workflow run locally against current main (89e2e17) on Python 3.11 to 3.14.DATE_ADDleft out of it, the date-part skip's qualified and two-argument guards dropped, empty definitions read, duplicates reported. The module imports without sqlglot on 3.11 and 3.12.examples/renamed_fields.yaml,examples/flights.semantic_model.yamlandexamples/tpcds_semantic_model.yamlpass clean; the YAML documents underexamples/andcompliance/compared with main differ only in the nine fixture warnings above.Related Issues
#602. Follows #486 (Name Resolution), #617 (
iter_expressions) and #649 (keys and relationship columns).Checklist
Validation
validation/are updated if the spec changedExamples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green)Compliance