Repository navigation
Conversation
…te_metric_scoping validate_metric_scoping walked tree.find_all(exp.Column), so under sqlglot's default dialect the unit of DATEDIFF was reported as an undeclared field and the spec's own DATEDIFF(day, start_date, end_date) form was rejected. The check now walks iter_columns, the iterator validate_metric_references and validate_field_shadowing already use: the unit is skipped and an identifier the parser left in the unit slot is read as a column. iter_columns skips every `*`, so a qualified star is read on its own and is still a qualified reference. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| col for col in tree.find_all(exp.Column) | ||
| if isinstance(col.this, exp.Star) and col.table | ||
| ) | ||
| for col in (*iter_columns(tree), *qualified_stars): |
There was a problem hiding this comment.
Switching to iter_columns brings its unit-vs-column guessing into a check that fails validation. Until now it only fed validate_metric_references, which warns. For dialects whose parser knows the date function, it gets cases wrong in both directions:
- Valid metrics are now rejected.
is_date_part_nameonly knows sqlglotDATE_PART_MAPPING, so a real unit missing from it makes the field inthislook like the unit. BIGQUERYMAX(DATE_DIFF(day, opened_at, ISOWEEK))with fieldsdayandopened_atdropsdayand reportsISOWEEKas an undeclared field. SNOWFLAKEMAX(DATEDIFF(nanoseconds, opened_at, d))with fieldsdandopened_atreportsNANOSECONDS. Both pass onmain. It only triggers for a field named like a date part, butdis what the spec own examples use. - Undeclared fields are now accepted.
is_date_partdrops the column inthiswheneverunitis not a date partVar. BIGQUERYMAX(DATE_DIFF(day, opened_at, WEEK(MONDAY)))with onlyopened_atdeclared: the unit is aWeekStart, sodayis skipped and nothing is recovered.MAX(DATEDIFF(day, opened_at, "closed at")): the recovered name failsisidentifier(), so neither argument is checked. Main rejects both, this PR accepts them.
Only sqlglot default parser (OSSIE_SQL_2026, ANSI_SQL) moves DATEDIFF last argument into unit. Dialects that know the function already put the unit there, so the column in this is never a unit.
Could iter_columns take the sqlglot dialect and only do the swap when it's None? That removes the guessing instead of adding more special cases.
There was a problem hiding this comment.
Done in bbbe42d: iter_columns takes the dialect and reads the unit slot back only when SQLGLOT_DIALECTS maps it to None. Your BIGQUERY and SNOWFLAKE metrics with declared fields pass, and WEEK(MONDAY) with day undeclared reports 'day'. The reference checks share the iterator and change too where a dialect's parser reads a function like the default parser; the description lists those cases.
The quoted name is the one case I couldn't fix. Under the default parser "closed at" reaches the tree as the same Var as the literal '2024-01-01', so it passes. Main rejected it by reporting 'day', the unit, and accepts it once a field day is declared. Comparing that Var with the fields would reject it, but also a literal date there, which main accepts when day is a field. I left that out and can add it if you prefer the strict reading.
| {"name": "days_open", "expression": _ds_expr("MAX(DATEDIFF(day, opened_at, closed_at))")}, | ||
| {"name": "source_days_open", "expression": _ds_expr("MAX(DATEDIFF(day, source.opened_at, source.closed_at))")}, | ||
| {"name": "minutes_since_open", "expression": _ds_expr("DATEDIFF(MINUTE, MAX(opened_at), CURRENT_TIMESTAMP)")}, | ||
| {"name": "ansi_days_open", "expression": _ds_expr("MAX(DATEDIFF(day, opened_at, closed_at))", "ANSI_SQL")}, |
There was a problem hiding this comment.
These tests only cover OSSIE_SQL_2026 and ANSI_SQL, the two dialects that go through sqlglot default parser. The regressions above are in SNOWFLAKE and BIGQUERY, which aren't covered.
Could we add scoping cases for those two dialects? One case should declare a field named like a date part next to a unit sqlglot doesn't alias (ISOWEEK, NANOSECONDS). Another should use a non-Var unit like WEEK(MONDAY) with an undeclared field.
| assert errors[0].startswith("[Scope]") and "'minute'" in errors[0] | ||
|
|
||
|
|
||
| def test_dataset_scoped_metric_still_rejects_a_qualified_star() -> None: |
There was a problem hiding this comment.
The description lists this test as failing on main, but it passes there: main find_all(exp.Column) walk already reports customers.*. It's still a good guard for the switch to iter_columns.
Could we update the description so it doesn't read as reproducing a bug?
There was a problem hiding this comment.
Right. The description now lists it with the tests that pass on main.
iter_columns guessed whether a column in the unit slot of DATEDIFF, DATE_ADD or TIMESTAMPDIFF was the unit or a column by whether the Var in unit names a date part. Under a parser that knows the function that guess fails both ways: a field named like a date part next to a unit the alias table lacks (ISOWEEK, nanoseconds) was dropped and the unit reported, and a non-Var unit hid an undeclared field. The iterator now takes the expression's dialect and reads the unit slot back only under sqlglot's default parser, where the shift happens; is_date_part treats a column in this as the unit only there too. The SNOWFLAKE case of the DATE_ADD reference test now uses DATEADD: Snowflake has no DATE_ADD, and sqlglot reads it there like the default parser does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kayemkim
left a comment
There was a problem hiding this comment.
On the tree merged with current main the validation workflow passes on the four Python versions, validate.py agrees with main on the 90 model and ontology documents I could find, converter fixtures included, and the branch merges cleanly with #697, #698, #701, #647 and #491. Across the nine mapped dialects the scoping check moves only where the description says, plus one case of the same kind: where a parser leaves DATEADD anonymous (the default parser, BIGQUERY, TRINO, POSTGRES), MAX(DATEADD(day, 1, opened_at)) now passes in a dataset-scoped metric and DATEADD(day, 1, nope) reports 'nope' alone instead of 'day', 'nope', which is the reading validate_metric_references already had on main. The quoted-name reading in the thread is the only thing I see left open.
validate_metric_scopingrejects a dataset-scoped metric that usesDATEDIFFwith a unit. The validator parsesOSSIE_SQL_2026andANSI_SQLwith sqlglot's default dialect, which reads the unit as a column. On main (0a908aa, sqlglot 30.29.0) the form fromcore-spec/expression_language.md,DATEDIFF(day, start_date, end_date), fails with both fields declared:DATEDIFF(day, source.opened_at, source.closed_at), the form #633 added, fails the same way.The check now reads columns through
iter_columns, asvalidate_metric_referencesandvalidate_field_shadowingdo since #608.iter_columnsskips every*, so the check reads a qualified star such asCOUNT(customers.*)on its own.iter_columnsnow takes the expression's dialect. It reads a column back from the unit slot only under the default parser, which does not know the unit ofDATEDIFF,DATE_ADDandTIMESTAMPDIFF. A parser that knows the function puts the unit inunit, so every column it finds is a column. The three checks share the iterator, so all of them read the unit slot by dialect.With the change:
DATEDIFF(day, opened_at, closed_at)with both fields declared passes underOSSIE_SQL_2026andANSI_SQL, and so doesDATEDIFF(day, source.opened_at, source.closed_at).DATEDIFF(MINUTE, MAX(opened_at), CURRENT_TIMESTAMP)passes.closed_atundeclared,DATEDIFF(day, opened_at, closed_at)reports'CLOSED_AT'instead of'day', in the spelling the parser keeps.DATEDIFF(day, opened_at, '2024-01-01'), passes. Main reported'day'for it unless a fielddaywas declared.SUM(minute)with no fieldminuteandCOUNT(customers.*)are still reported, andCOUNT(source.*)still passes.SNOWFLAKEandBIGQUERYthe check reads as on main.DATEDIFF(nanoseconds, opened_at, d)andDATE_DIFF(day, opened_at, ISOWEEK)with their fields declared pass.DATE_DIFF(day, opened_at, WEEK(MONDAY))withdayundeclared reports'day'.SNOWFLAKE,validate_metric_referenceswarns ondinSUM(DATEDIFF(nanoseconds, orders.total_usd, d))instead of'NANOSECONDS'.Known limits:
DATEDIFF(day, opened_at, quarter), is read as the unit, asis_date_partdocuments.DATEDIFF(day, opened_at, "closed at"), is not checked. It reaches the tree as the sameVaras a string literal. Main reported'day'for it unless a fielddaywas declared, and the reference checks have skipped it since feat(validation): check metric references and field shadowing against Name Resolution #608.timestampDiff) or do not know it (DATEDIFFunderBIGQUERY,TRINOandPOSTGRES,DATE_ADDunderSNOWFLAKE). There the written unit is read as a column. The scoping check reports'day', as main does, and the two reference checks now warn onday.One existing test changes. The
SNOWFLAKEcase oftest_date_add_reads_its_unit_as_a_unit_and_its_date_as_a_columnusedDATE_ADD, which Snowflake does not have. sqlglot's Snowflake parser reads it the way the default parser does, so the case now usesDATEADD.test_dataset_scoped_metric_reads_a_date_part_as_a_unit_not_a_field,test_dataset_scoped_metric_reads_a_column_in_the_unit_slot_as_a_fieldand thenanosecondscase oftest_a_parser_that_knows_the_unit_leaves_the_date_arguments_as_columnsfail on main.test_dataset_scoped_metric_still_rejects_a_qualified_starand theSNOWFLAKEandBIGQUERYscoping tests pass on main and guard the switch toiter_columns.validation/tests: 536 passed, 1 skipped (530 passed, 1 skipped on main). The examples CI validates pass as before.Follow-up to #633, as asked in #633 (comment). The same false positive was first reported on #608 (#608 (comment)).