From 7260e48fe206271eaca7d65c5e7ac60522fab03d Mon Sep 17 00:00:00 2001 From: Kyoung Min Kim Date: Sun, 18 Oct 2026 19:05:38 +0900 Subject: [PATCH 1/4] feat(validation): warn when keys, relationship columns or metric references do not name a declared field The spec's Name Resolution section says primary_key, unique_keys, relationship from_columns/to_columns and metric expressions refer to a dataset's declared fields, not to columns of its source. validate.py did not check any of that, so a document whose keys name source columns, or whose metric reads SUM(orders.o_totalprice) against a field called total_amount, passed. Three checks report each such reference, one per rule. Keys and relationship columns have to be declared fields of their dataset. Metric expressions are parsed with sqlglot and every column has to be a declared field written as dataset.field: an unqualified name, a name that is another metric, a dataset the model does not declare and a field the dataset does not are each told apart. Dialects sqlglot cannot parse are skipped, an expression that fails to parse is left to the SQL check, and a bare date part such as the day in DATEDIFF(day, a, b), which the default parser reads as a column, is not reported. The same section makes a bare name in a field expression a source column. When another field of the same dataset carries that name, the author may have meant the field, so that is reported too, unless the other field is the same-named column passed through, read from its parsed definition, in which case both readings agree. The findings are warnings for now: at least one converter still emits a metric over a name that is not a field and runs validate.py in its tests, so an error would turn that suite red before the converter has caught up. examples/renamed_fields.yaml is a small model whose field names differ from their source columns throughout, and validation-ci validates it. The test fixtures now declare the fields their keys refer to. Generated-by: Claude Code --- .github/workflows/validation-ci.yml | 5 + examples/renamed_fields.yaml | 120 +++++++ validation/test_validate.py | 10 +- validation/tests/test_validate.py | 476 +++++++++++++++++++++++++++- validation/validate.py | 239 +++++++++++++- 5 files changed, 846 insertions(+), 4 deletions(-) create mode 100644 examples/renamed_fields.yaml diff --git a/.github/workflows/validation-ci.yml b/.github/workflows/validation-ci.yml index 9e3c45aa..7473c2c1 100644 --- a/.github/workflows/validation-ci.yml +++ b/.github/workflows/validation-ci.yml @@ -29,6 +29,7 @@ on: - "ontology/**" - "examples/flights*.yaml" - "examples/tpcds_semantic_model.yaml" + - "examples/renamed_fields.yaml" - ".github/workflows/validation-ci.yml" - "scripts/generate-spec-types.py" pull_request: @@ -40,6 +41,7 @@ on: - "ontology/**" - "examples/flights*.yaml" - "examples/tpcds_semantic_model.yaml" + - "examples/renamed_fields.yaml" - ".github/workflows/validation-ci.yml" - "scripts/generate-spec-types.py" @@ -89,3 +91,6 @@ jobs: uv run validation/validate.py examples/flights.ontology.yaml --schema ontology/ontology.json uv run validation/validate.py examples/flights.semantic_model.yaml uv run validation/validate.py examples/flights.mapping.yaml --schema ontology/mapping.json + + - name: Validate renamed-fields example + run: uv run validation/validate.py examples/renamed_fields.yaml diff --git a/examples/renamed_fields.yaml b/examples/renamed_fields.yaml new file mode 100644 index 00000000..4e61bef6 --- /dev/null +++ b/examples/renamed_fields.yaml @@ -0,0 +1,120 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# A small model whose field names deliberately differ from the columns of +# their source tables. Keys, relationship columns and metric expressions +# all name the fields, never the source columns, as described in +# core-spec/spec.md "Name Resolution". Converters that compile to SQL +# against the source tables have to substitute each field's expression. +version: "0.2.0.dev0" +name: renamed_fields +description: Orders and customers whose field names differ from their source columns + +datasets: + - name: orders + source: sales.public.orders # columns: o_orderkey, o_custkey, o_orderdate, o_totalprice, total_cents + primary_key: [order_id] + description: One row per order + fields: + - name: order_id + expression: + dialects: + - dialect: ANSI_SQL + expression: o_orderkey + datatype: Integer + - name: customer_id + expression: + dialects: + - dialect: ANSI_SQL + expression: o_custkey + datatype: Integer + - name: order_date + expression: + dialects: + - dialect: ANSI_SQL + expression: o_orderdate + datatype: Date + dimension: + is_time: true + - name: total_amount + expression: + dialects: + - dialect: ANSI_SQL + expression: o_totalprice + datatype: Decimal + - name: total_usd + expression: + dialects: + - dialect: ANSI_SQL + expression: total_cents / 100.0 + datatype: Decimal + + - name: customers + source: sales.public.customers # columns: c_custkey, c_name, c_mktsegment + primary_key: [id] + description: One row per customer + fields: + - name: id + expression: + dialects: + - dialect: ANSI_SQL + expression: c_custkey + datatype: Integer + - name: name + expression: + dialects: + - dialect: ANSI_SQL + expression: c_name + datatype: String + - name: segment + expression: + dialects: + - dialect: ANSI_SQL + expression: c_mktsegment + datatype: String + +relationships: + - name: orders_to_customers + from: orders + to: customers + from_columns: [customer_id] # field of orders, not o_custkey + to_columns: [id] # field of customers, not c_custkey + +metrics: + - name: total_revenue + description: Sum of order totals in dollars + expression: + dialects: + - dialect: ANSI_SQL + expression: SUM(orders.total_usd) + - name: order_count + description: Number of orders + expression: + dialects: + - dialect: ANSI_SQL + expression: COUNT(DISTINCT orders.order_id) + - name: customer_count + description: Number of customers with at least one order + expression: + dialects: + - dialect: ANSI_SQL + expression: COUNT(DISTINCT orders.customer_id) + - name: gross_revenue + description: Sum of order totals as the source records them, through the renamed field + expression: + dialects: + - dialect: ANSI_SQL + expression: SUM(orders.total_amount) diff --git a/validation/test_validate.py b/validation/test_validate.py index 5386dd8a..886cd7c2 100644 --- a/validation/test_validate.py +++ b/validation/test_validate.py @@ -535,6 +535,9 @@ def test_valid_model_still_passes(self): self.assertIn("Validation PASSED", result.stdout) def test_relationship_column_counts_are_checked_in_flat_documents(self): + def column(name): + return {"name": name, "expression": {"dialects": [{"dialect": "ANSI_SQL", "expression": name}]}} + cases = ( (["customer_id"], ["id"], 0), (["customer_id", "region_id"], ["id"], 1), @@ -546,11 +549,16 @@ def test_relationship_column_counts_are_checked_in_flat_documents(self): "version": "0.2.0.dev0", "name": "sales", "datasets": [ - {"name": "orders", "source": "analytics.orders"}, + { + "name": "orders", + "source": "analytics.orders", + "fields": [column("customer_id"), column("region_id")], + }, { "name": "customers", "source": "analytics.customers", "primary_key": ["id"], + "fields": [column("id"), column("region_id")], }, ], "relationships": [ diff --git a/validation/tests/test_validate.py b/validation/tests/test_validate.py index ece083f1..3914bb3c 100644 --- a/validation/tests/test_validate.py +++ b/validation/tests/test_validate.py @@ -55,14 +55,25 @@ def _document(datasets: list[dict], relationships: list[dict]) -> dict: } +def _column(name: str) -> dict: + return {"name": name, "expression": {"dialects": [{"dialect": "ANSI_SQL", "expression": name}]}} + + +# Keys and relationship columns name declared fields (core-spec/spec.md +# "Name Resolution"), so the fixtures declare the fields they refer to. _CUSTOMERS = { "name": "customers", "source": "db.s.customers", "primary_key": ["id"], "unique_keys": [["email"]], + "fields": [_column("id"), _column("email"), _column("region")], } -_ORDERS = {"name": "orders", "source": "db.s.orders"} +_ORDERS = { + "name": "orders", + "source": "db.s.orders", + "fields": [_column("customer_id"), _column("amount")], +} def test_accepts_a_single_root_model(core_schema: dict) -> None: @@ -919,7 +930,9 @@ def test_metric_expressed_only_in_ossie_sql_2026_validates(run_validator): document = _document([_ORDERS], []) document["metrics"] = [{ "name": "total_amount", - "expression": {"dialects": [{"dialect": "OSSIE_SQL_2026", "expression": "SUM(amount)"}]}, + "expression": { + "dialects": [{"dialect": "OSSIE_SQL_2026", "expression": "SUM(orders.amount)"}] + }, }] exit_code, output = run_validator(document) @@ -1625,3 +1638,462 @@ def test_superseded_ossie_sql_2026_dialect_entry_is_not_scope_checked() -> None: ) assert _VALIDATE.validate_metric_scoping(doc) == [] +# --- field references (core-spec/spec.md "Name Resolution") --- + +ValidationWarning = _VALIDATE.ValidationWarning + + +def _field_reference_findings(document: dict) -> list[str]: + """The three Name Resolution checks together, in the order validate_document runs them.""" + return ( + _VALIDATE.validate_key_references(document) + + _VALIDATE.validate_metric_references(document) + + _VALIDATE.validate_field_shadowing(document) + ) + + +def _field(name: str, expression: str | None = None, dialect: str = "ANSI_SQL") -> dict: + """A field defined by one expression; by default the same-named source column.""" + sql = name if expression is None else expression + return {"name": name, "expression": {"dialects": [{"dialect": dialect, "expression": sql}]}} + + +def _metric(name: str, expression: str, dialect: str = "ANSI_SQL") -> dict: + """A metric carries its expression the way a field does.""" + return _field(name, expression, dialect) + + +def _renamed_model(**overrides) -> dict: + document = { + "version": "0.2.0.dev0", + "name": "m", + "datasets": [ + { + "name": "orders", + "source": "db.s.orders", + "primary_key": ["order_id"], + "fields": [ + _field("order_id", "o_orderkey"), + _field("customer_id", "o_custkey"), + _field("total_usd", "total_cents / 100.0"), + ], + }, + { + "name": "customers", + "source": "db.s.customers", + "primary_key": ["id"], + "unique_keys": [["email"]], + "fields": [_field("id", "c_custkey"), _field("email", "c_email")], + }, + ], + "relationships": [ + { + "name": "orders_to_customers", + "from": "orders", + "to": "customers", + "from_columns": ["customer_id"], + "to_columns": ["id"], + }, + ], + "metrics": [ + _metric("total_revenue", "SUM(orders.total_usd)"), + ], + } + document.update(overrides) + return document + + +def test_accepts_keys_relationships_and_metrics_that_name_declared_fields() -> None: + assert _field_reference_findings(_renamed_model()) == [] + + +def test_field_reference_findings_are_warnings_for_now() -> None: + document = _renamed_model() + document["datasets"][0]["primary_key"] = ["o_orderkey"] + document["metrics"][0]["expression"]["dialects"][0]["expression"] = "SUM(total_usd)" + findings = _field_reference_findings(document) + assert len(findings) == 2 + assert all(isinstance(f, ValidationWarning) for f in findings) + + +def test_rejects_primary_key_that_names_a_source_column() -> None: + document = _renamed_model() + document["datasets"][0]["primary_key"] = ["o_orderkey"] + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "primary_key names 'o_orderkey'" in errors[0] + + +def test_rejects_unique_key_that_names_a_source_column() -> None: + document = _renamed_model() + document["datasets"][1]["unique_keys"] = [["c_email"]] + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "unique_keys names 'c_email'" in errors[0] + + +def test_rejects_relationship_columns_that_name_source_columns() -> None: + document = _renamed_model() + document["relationships"][0]["from_columns"] = ["o_custkey"] + document["relationships"][0]["to_columns"] = ["c_custkey"] + errors = _field_reference_findings(document) + assert len(errors) == 2 + assert "from_columns names 'o_custkey'" in errors[0] + assert "to_columns names 'c_custkey'" in errors[1] + + +@pytest.mark.parametrize("fields", [[], None], ids=["empty", "absent"]) +def test_rejects_keys_on_a_dataset_that_declares_no_fields(fields) -> None: + document = _renamed_model() + if fields is None: + del document["datasets"][1]["fields"] + else: + document["datasets"][1]["fields"] = fields + errors = _field_reference_findings(document) + assert len(errors) == 3 # primary_key, unique_keys, to_columns + assert all("'customers'" in e for e in errors) + + +def test_rejects_metric_that_references_a_source_column() -> None: + document = _renamed_model() + document["metrics"][0]["expression"]["dialects"][0]["expression"] = "SUM(orders.o_totalprice)" + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "'orders.o_totalprice' is not a declared field of dataset 'orders'" in errors[0] + + +def test_rejects_metric_with_unqualified_field_reference() -> None: + document = _renamed_model() + document["metrics"][0]["expression"]["dialects"][0]["expression"] = "SUM(total_usd)" + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "'total_usd' is not qualified" in errors[0] + + +def test_rejects_metric_that_references_an_unknown_dataset() -> None: + document = _renamed_model() + document["metrics"][0]["expression"]["dialects"][0]["expression"] = "SUM(order.total_usd)" + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "references unknown dataset 'order'" in errors[0] + + +def test_metric_check_skips_dialects_that_are_not_sql() -> None: + document = _renamed_model() + dax = {"dialect": "DAX", "expression": "SUM(Orders[Total])"} + document["metrics"][0]["expression"]["dialects"].append(dax) + assert _field_reference_findings(document) == [] + + +def test_metric_check_leaves_unparseable_sql_to_the_sql_check() -> None: + document = _renamed_model() + document["metrics"][0]["expression"]["dialects"][0]["expression"] = "SUM(CASE WHEN" + assert _field_reference_findings(document) == [] + + +def test_field_reference_check_skips_relationships_to_unknown_datasets() -> None: + document = _renamed_model() + document["relationships"][0]["to"] = "nobody" + assert _field_reference_findings(document) == [] + + +def test_warns_when_a_field_expression_names_another_field_of_its_dataset() -> None: + document = _renamed_model() + document["datasets"][0]["fields"].append(_field("total_with_tax", "total_usd * 1.1")) + errors = _field_reference_findings(document) + assert errors == [ + "[Reference] Warning: Field 'orders.total_with_tax' in model 'm' (ANSI_SQL): " + "'total_usd' is the source column, not the field 'total_usd' defined as " + "'total_cents / 100.0'" + ] + assert isinstance(errors[0], ValidationWarning) + + +def test_field_expression_may_name_a_field_that_passes_its_column_through() -> None: + document = _renamed_model() + document["datasets"][0]["fields"].append(_field("amount")) + document["datasets"][0]["fields"].append(_field("amount_with_tax", "amount * 1.1")) + assert _field_reference_findings(document) == [] + + +def test_field_expression_may_name_its_own_source_column() -> None: + document = _renamed_model() + document["datasets"][0]["fields"].append(_field("amount", "amount * 100")) + assert _field_reference_findings(document) == [] + + +def test_field_shadowing_check_reads_the_same_dialect_as_the_expression() -> None: + document = _renamed_model() + amount = {"name": "amount", "expression": {"dialects": [ + {"dialect": "ANSI_SQL", "expression": "amount"}, + {"dialect": "SNOWFLAKE", "expression": "AMT"}, + ]}} + taxed = {"name": "amount_with_tax", "expression": {"dialects": [ + {"dialect": "ANSI_SQL", "expression": "amount * 1.1"}, + {"dialect": "SNOWFLAKE", "expression": "amount * 1.1"}, + ]}} + document["datasets"][0]["fields"] += [amount, taxed] + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert errors[0].startswith( + "[Reference] Warning: Field 'orders.amount_with_tax' in model 'm' (SNOWFLAKE): 'amount'" + ) + assert "defined as 'AMT'" in errors[0] + + +def test_field_shadowing_check_skips_dialects_that_are_not_sql_and_unparseable_sql() -> None: + document = _renamed_model() + document["datasets"][0]["fields"].append( + {"name": "x", "expression": {"dialects": [ + {"dialect": "DAX", "expression": "[total_usd] * 2"}, + {"dialect": "ANSI_SQL", "expression": "total_usd * ("}, + ]}} + ) + assert _field_reference_findings(document) == [] + + +@pytest.mark.parametrize( + "definition", ["(amount)", '"amount"', "amount -- the amount", "((amount))"] +) +def test_pass_through_is_read_from_the_parsed_definition(definition: str) -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("amount", definition), + _field("amount_cents", "amount * 100"), + ] + assert _field_reference_findings(document) == [] + + +def test_pass_through_compares_names_as_written() -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("amount", "AMOUNT"), + _field("amount_cents", "amount * 100"), + ] + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "defined as 'AMOUNT'" in errors[0] + + +def test_field_shadowing_check_needs_a_sql_definition_to_compare() -> None: + document = _renamed_model() + dax_only = _field("amount", "[amount]", dialect="DAX") + document["datasets"][0]["fields"] += [dax_only, _field("amount_cents", "amount * 100")] + assert _field_reference_findings(document) == [] + + +def test_field_shadowing_check_falls_back_to_any_dialect_of_the_other_field() -> None: + document = _renamed_model() + snowflake_only = _field("amount", dialect="SNOWFLAKE") + document["datasets"][0]["fields"] += [snowflake_only, _field("amount_cents", "amount * 100")] + assert _field_reference_findings(document) == [] + + +def test_field_shadowing_check_ignores_qualified_names_and_reports_a_name_once() -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("total_usd2", "orders.total_usd * 2"), + _field("total_usd3", "total_usd + total_usd"), + ] + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "Field 'orders.total_usd3'" in errors[0] + + +def test_metric_that_names_another_metric_is_told_so() -> None: + document = _renamed_model() + document["metrics"].append( + {"name": "revenue_share", "expression": {"dialects": [ + {"dialect": "ANSI_SQL", "expression": "total_revenue / SUM(orders.total_usd)"} + ]}} + ) + errors = _field_reference_findings(document) + assert errors == [ + "[Reference] Warning: Metric 'revenue_share' in model 'm' (ANSI_SQL): " + "'total_revenue' names the metric 'total_revenue'; metric expressions cannot refer " + "to other metrics" + ] + + +def test_metric_check_skips_star_and_reports_more_than_two_parts() -> None: + document = _renamed_model() + expression = "COUNT(orders.*) + SUM(db.orders.total_usd)" + document["metrics"][0]["expression"]["dialects"][0]["expression"] = expression + errors = _field_reference_findings(document) + assert errors == [ + "[Reference] Warning: Metric 'total_revenue' in model 'm' (ANSI_SQL): " + "'db.orders.total_usd' has more than two parts; metric expressions refer to fields " + "as dataset.field" + ] + + +def test_field_references_are_looked_up_on_the_named_dataset_only() -> None: + document = _renamed_model() + # id and email are fields of customers, not of orders. + document["datasets"][0]["primary_key"] = ["id"] + document["relationships"][0]["from_columns"] = ["email"] + document["metrics"][0]["expression"]["dialects"][0]["expression"] = "SUM(orders.id)" + errors = _field_reference_findings(document) + assert len(errors) == 3 + assert "primary_key names 'id'" in errors[0] + assert "from_columns names 'email'" in errors[1] + assert "'orders.id' is not a declared field of dataset 'orders'" in errors[2] + + +def test_every_undeclared_name_is_reported_and_each_only_once() -> None: + document = _renamed_model() + document["datasets"][0]["primary_key"] = ["order_id", "nope", "nada", "nope"] + document["datasets"][1]["unique_keys"] = [["email"], ["c_name"], ["c_phone"]] + document["relationships"][0]["from_columns"] = ["o_custkey", "o_x"] + document["metrics"][0]["expression"]["dialects"] = [ + {"dialect": "ANSI_SQL", "expression": "SUM(orders.a) + SUM(orders.b)"}, + {"dialect": "SNOWFLAKE", "expression": "SUM(orders.c)"}, + ] + errors = _field_reference_findings(document) + named = [e.split("names ")[1].split(",")[0] for e in errors[:6]] + assert named == ["'nope'", "'nada'", "'c_name'", "'c_phone'", "'o_custkey'", "'o_x'"] + assert [e.split("'")[5] for e in errors[6:]] == ["orders.a", "orders.b", "orders.c"] + + +def test_field_shadowing_check_reports_the_other_dialect_definition_it_fell_back_to() -> None: + document = _renamed_model() + snowflake_only = _field("amount", "AMT", dialect="SNOWFLAKE") + document["datasets"][0]["fields"] += [snowflake_only, _field("amount_cents", "amount * 100")] + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "(ANSI_SQL): 'amount' is the source column" in errors[0] + assert "defined as 'AMT'" in errors[0] + + +def test_field_shadowing_check_looks_at_the_same_dataset_only() -> None: + document = _renamed_model() + # email is a field of customers, so in an orders expression it is only a source column. + document["datasets"][0]["fields"].append(_field("email_domain", "SPLIT_PART(email, '@', 2)")) + assert _field_reference_findings(document) == [] + + +@pytest.mark.parametrize("definition", ["t.amount", "CAST(amount AS INT)"]) +def test_pass_through_is_the_bare_column_and_nothing_else(definition: str) -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("amount", definition), + _field("amount_cents", "amount * 100"), + ] + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert f"defined as '{definition}'" in errors[0] + + +def test_pass_through_may_carry_an_alias() -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("amount", "amount AS a"), + _field("amount_cents", "amount * 100"), + ] + assert _field_reference_findings(document) == [] + + +def test_field_shadowing_check_needs_a_definition_it_can_parse() -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("amount", "amount * ("), + _field("amount_cents", "amount * 100"), + ] + assert _field_reference_findings(document) == [] + + +def test_qualified_metric_name_is_reported_as_a_missing_field() -> None: + document = _renamed_model() + document["metrics"].append( + _metric("share", "SUM(orders.total_revenue)"), + ) + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "'orders.total_revenue' is not a declared field of dataset 'orders'" in errors[0] + + +@pytest.mark.parametrize( + "name, expression", + [ + ("total_revenue", "total_revenue + 1"), # the metric's own name + ("total_usd_sum", "SUM(total_usd)"), # a field somewhere in the model + ], +) +def test_bare_name_of_this_metric_or_a_field_is_reported_as_unqualified(name, expression) -> None: + document = _renamed_model() + document["metrics"] = [_metric(name, expression)] + if name == "total_usd_sum": + document["metrics"].append(_renamed_model()["metrics"][0] | {"name": "total_usd"}) + errors = _field_reference_findings(document) + assert len(errors) == 1 + assert "is not qualified" in errors[0] + + +def test_renamed_fields_example_passes_the_field_reference_check() -> None: + import yaml + + example = Path(__file__).parents[2] / "examples" / "renamed_fields.yaml" + assert _field_reference_findings(yaml.safe_load(example.read_text())) == [] + + +@pytest.mark.parametrize("expression", [ + "DATEDIFF(day, orders.shipped, orders.ordered)", + "TIMESTAMPDIFF(MINUTE, orders.shipped, orders.ordered)", + "DATEADD(day, 1, orders.ordered)", + "DATE_PART(year, orders.ordered)", +]) +def test_metric_check_reads_a_date_part_as_a_unit_not_a_column(expression) -> None: + document = _renamed_model(metrics=[_metric("age", expression)]) + document["datasets"][0]["fields"] += [_field("shipped"), _field("ordered")] + + assert _VALIDATE.validate_metric_references(document) == [] + + +def test_metric_check_still_reports_a_bare_column_named_like_a_unit() -> None: + document = _renamed_model(metrics=[_metric("days", "SUM(day)")]) + + errors = _VALIDATE.validate_metric_references(document) + + assert len(errors) == 1 + assert "'day' is not qualified" in errors[0] + + +@pytest.mark.parametrize("document", [ + { + "version": "0.2.0.dev0", + "name": "m", + "datasets": None, + "metrics": None, + "relationships": None, + }, + { + "version": "0.2.0.dev0", + "name": "m", + "datasets": [ + { + "name": "orders", + "primary_key": [["a"]], + "fields": [{"name": "a", "expression": "a"}], + }, + {"name": None, "fields": [{"name": "b", "expression": {"dialects": None}}]}, + ], + "metrics": [{"name": "m", "expression": "SUM(orders.a)"}, "not a metric"], + "relationships": [{"name": "r", "from": ["orders"], "to": "orders", "to_columns": "a"}], + }, +]) +def test_field_reference_checks_tolerate_shapes_the_schema_rejects(document) -> None: + assert _field_reference_findings(document) == [] + + +def test_cli_prints_reference_warnings_and_still_passes(run_validator) -> None: + document = _renamed_model() + document["datasets"][0]["primary_key"] = ["o_orderkey"] + + exit_code, output = run_validator(document) + + assert exit_code == 0 + assert "Validation PASSED" in output + assert ( + "[Reference] Warning: Dataset 'orders' in model 'm': primary_key names 'o_orderkey', " + "which is not a declared field" + ) in output diff --git a/validation/validate.py b/validation/validate.py index 8d705e85..378b840f 100644 --- a/validation/validate.py +++ b/validation/validate.py @@ -34,7 +34,9 @@ 2. Unique names (datasets, fields, metrics, relationships) 3. Valid relationship references 4. Relationship column arity (from_columns and to_columns lengths match) -5. SQL syntax (using sqlglot) +5. Field references: keys, relationship columns and metric expressions name declared + fields, and a field expression that names another field is flagged (warnings) +6. SQL syntax (using sqlglot) Ontology documents (--schema ontology/ontology.json) are checked for unique concept and relationship names, and for extends, identify_by, roles and QName @@ -330,6 +332,238 @@ def validate_relationship_column_arity(data: dict) -> list[str]: return errors +def declared_fields(model: dict) -> dict[str, set[str]]: + """The names of the fields each dataset declares, by dataset name. + + A dataset without a fields list declares none: model-level constructs do + not reach through a dataset to its source (spec.md, Name Resolution). + """ + return { + d["name"]: { + f.get("name") for f in as_list(d.get("fields")) if isinstance(f, dict) and f.get("name") + } + for d in as_list(model.get("datasets")) + if isinstance(d, dict) and d.get("name") + } + + +def sql_dialect_expressions(node: dict, dialect: str | None = None) -> list[tuple[str, str]]: + """The (dialect, expression) pairs of a field or metric that sqlglot can + parse, or only those in one dialect when it is given.""" + expression = node.get("expression") + if not isinstance(expression, dict): + return [] + found = [] + for dialect_expr in as_list(expression.get("dialects")): + if not isinstance(dialect_expr, dict): + continue + d = dialect_expr.get("dialect", "ANSI_SQL") + expr = dialect_expr.get("expression") + if not isinstance(expr, str) or not expr or d not in SQLGLOT_DIALECTS: + continue + if dialect is None or d == dialect: + found.append((d, expr)) + return found + + +def parse_sql(expr: str, dialect: str): + """The sqlglot tree of expr, or None when it is not one expression. + + The SQL check reports what does not parse at all. What parses only as a + SELECT item, such as ``DISTINCT x``, passes that check and is skipped here. + """ + try: + return sqlglot.parse_one(expr, dialect=SQLGLOT_DIALECTS.get(dialect)) + except (ParseError, TokenError, RecursionError): + return None + + +# Functions sqlglot leaves untyped under the default dialect whose first argument +# is a date part; the typed ones (DateDiff, DateAdd, ...) are matched by class. +DATE_PART_FUNCTIONS = {"DATEADD", "DATE_ADD", "DATEDIFF", "DATE_DIFF", "DATE_PART", "DATEPART"} + + +def is_date_part(column) -> bool: + """Whether a bare name is the unit argument of a date function. + + The default parser reads ``day`` in ``DATEDIFF(day, a, b)`` as a column, + while the Snowflake one reads it as a unit. + """ + exp = sqlglot.exp + parent = column.parent + typed = (exp.DateDiff, exp.TimestampDiff, exp.DateAdd, exp.DateSub, exp.TimestampAdd, + exp.TimestampSub, exp.DateTrunc, exp.TimestampTrunc) + 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 + return column.name.upper() in units or column.name.upper() in units.values() + + +def validate_key_references(data: dict) -> list[str]: + """Warn when primary_key, unique_keys or relationship from_columns/to_columns + name something other than a declared field of their dataset (spec.md, Name + Resolution). Warnings while converters catch up with the rule.""" + if not isinstance(data, dict) or "datasets" not in data: + return [] + model_name = data.get("name", "") + fields = declared_fields(data) + warnings = [] + + def undeclared(dataset_name: str, columns) -> list[str]: + names = [c for c in as_list(columns) if isinstance(c, str)] + return [c for c in dict.fromkeys(names) if c not in fields.get(dataset_name, set())] + + for dataset in as_list(data.get("datasets")): + if not isinstance(dataset, dict) or dataset.get("name") not in fields: + continue + name = dataset["name"] + context = f"Dataset '{name}' in model '{model_name}'" + for col in undeclared(name, dataset.get("primary_key")): + warnings.append(ValidationWarning( + f"[Reference] Warning: {context}: primary_key names '{col}', which is not a " + f"declared field" + )) + for key in as_list(dataset.get("unique_keys")): + for col in undeclared(name, key): + warnings.append(ValidationWarning( + f"[Reference] Warning: {context}: unique_keys names '{col}', which is not a " + f"declared field" + )) + + for rel in as_list(data.get("relationships")): + if not isinstance(rel, dict): + continue + context = f"Relationship '{rel.get('name', '')}' in model '{model_name}'" + for side, endpoint in (("from_columns", "from"), ("to_columns", "to")): + dataset_name = rel.get(endpoint) + if not isinstance(dataset_name, str) or dataset_name not in fields: + continue # reported by validate_references + for col in undeclared(dataset_name, rel.get(side)): + warnings.append(ValidationWarning( + f"[Reference] Warning: {context}: {side} names '{col}', which is not a " + f"declared field of dataset '{dataset_name}'" + )) + return warnings + + +def validate_metric_references(data: dict) -> list[str]: + """Warn when a metric expression refers to anything but a declared field + written as dataset.field (spec.md, Name Resolution): an unqualified name, + another metric, a dataset the model does not declare or a field the dataset + does not. Dialects sqlglot cannot parse are skipped and an expression that + does not parse is left to the SQL check.""" + if not SQLGLOT_AVAILABLE or not isinstance(data, dict) or "datasets" not in data: + return [] + model_name = data.get("name", "") + fields = declared_fields(data) + field_names = set().union(*fields.values()) if fields else set() + metrics = [m for m in as_list(data.get("metrics")) if isinstance(m, dict)] + metric_names = {m.get("name") for m in metrics} + warnings = [] + + for metric in metrics: + metric_name = metric.get("name", "") + # A bare name that is another metric, and not also a field or this + # metric itself, was most likely meant as that metric. + other_metrics = metric_names - field_names - {metric_name} + for dialect, expr in sql_dialect_expressions(metric): + tree = parse_sql(expr, dialect) + if tree is None: + continue + context = f"Metric '{metric_name}' in model '{model_name}' ({dialect})" + for column in tree.find_all(sqlglot.exp.Column): + if isinstance(column.this, sqlglot.exp.Star) or is_date_part(column): + continue + if column.db: + written = ".".join(part.name for part in column.parts) + message = ( + f"'{written}' has more than two parts; metric expressions refer to " + f"fields as dataset.field" + ) + elif not column.table and column.name in other_metrics: + message = ( + f"'{column.name}' names the metric '{column.name}'; metric expressions " + f"cannot refer to other metrics" + ) + elif not column.table: + message = ( + f"'{column.name}' is not qualified; metric expressions refer to fields " + f"as dataset.field" + ) + elif column.table not in fields: + message = ( + f"'{column.table}.{column.name}' references unknown dataset " + f"'{column.table}'" + ) + elif column.name not in fields[column.table]: + message = ( + f"'{column.table}.{column.name}' is not a declared field of dataset " + f"'{column.table}'" + ) + else: + continue + warnings.append(ValidationWarning(f"[Reference] Warning: {context}: {message}")) + return warnings + + +def validate_field_shadowing(data: dict) -> list[str]: + """Warn when a field expression names another field of the same dataset. + + A bare name in a field expression is a column of the dataset's source + (spec.md, Name Resolution), so when another field carries that name the + author may have meant the field. Nothing is reported when that field is + the same-named column passed through, since both readings then agree, when + it has no SQL definition the validator can read, or when a field names its + own column. + """ + if not SQLGLOT_AVAILABLE or not isinstance(data, dict) or "datasets" not in data: + return [] + model_name = data.get("name", "") + warnings = [] + + def passes_through(tree, name: str) -> bool: + while isinstance(tree, (sqlglot.exp.Paren, sqlglot.exp.Alias)): + tree = tree.this + return isinstance(tree, sqlglot.exp.Column) and not tree.table and tree.name == name + + for dataset in as_list(data.get("datasets")): + if not isinstance(dataset, dict): + continue + dataset_name = dataset.get("name", "") + declared = [ + f for f in as_list(dataset.get("fields")) + if isinstance(f, dict) and isinstance(f.get("name"), str) + ] + by_name = {f["name"]: f for f in declared} + for field in declared: + for dialect, expr in sql_dialect_expressions(field): + 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] + for name in dict.fromkeys(names): + other = by_name.get(name) + if other is None or other is field: + continue + definitions = ( + sql_dialect_expressions(other, dialect) or sql_dialect_expressions(other) + ) + # A definition that does not parse gives nothing to compare. + parsed = [(e, t) for d, e in definitions if (t := parse_sql(e, d)) is not None] + if not parsed or any(passes_through(t, name) for _, t in parsed): + continue + context = ( + f"Field '{dataset_name}.{field['name']}' in model '{model_name}' " + f"({dialect})" + ) + warnings.append(ValidationWarning( + f"[Reference] Warning: {context}: '{name}' is the source column, not the " + f"field '{name}' defined as '{parsed[0][0]}'" + )) + return warnings + + def _parse_sql_expression( expr: str, dialect: str, context: str ) -> tuple[object | None, str | None]: @@ -844,6 +1078,9 @@ def validate_document(data, schema: dict) -> list[str]: errors.extend(validate_unique_names(data)) errors.extend(validate_references(data)) errors.extend(validate_relationship_column_arity(data)) + errors.extend(validate_key_references(data)) + errors.extend(validate_metric_references(data)) + errors.extend(validate_field_shadowing(data)) errors.extend(validate_metric_scoping(data)) errors.extend(validate_sql(data)) if not errors and isinstance(data, dict) and "ontology" in data: From c2b33c6b462d1c96531b81a7f9c4359b22d0bd00 Mon Sep 17 00:00:00 2001 From: Kyoung Min Kim Date: Wed, 21 Oct 2026 16:36:29 +0900 Subject: [PATCH 2/4] fix(validation): compare names case-insensitively and read every date part as a unit The three Name Resolution checks compared names as written, while expression_language.md makes regular identifiers case-insensitive. So SUM(ORDERS.TOTAL_USD) over dataset orders warned about an unknown dataset, and Total_USD * 2 next to a field total_usd escaped the shadowing check. fold() now upper-cases both sides of every lookup: the declared fields, the metric names and the pass-through test. is_date_part read sqlglot's UNABBREVIATED_UNIT_NAME, which has no MONTH and none of the aliases, so month, dd and days in DATEDIFF, DATEADD and DATE_PART warned as unqualified columns. It now reads Dialect.DATE_PART_MAPPING, and the shadowing check applies the same filter, so a field named day next to DATEDIFF(day, ...) is quiet. Generated-by: Claude Code --- validation/tests/test_validate.py | 112 +++++++++++++++++++++++++++- validation/validate.py | 118 +++++++++++++++++++++--------- 2 files changed, 193 insertions(+), 37 deletions(-) diff --git a/validation/tests/test_validate.py b/validation/tests/test_validate.py index 3914bb3c..6dbbf747 100644 --- a/validation/tests/test_validate.py +++ b/validation/tests/test_validate.py @@ -1864,15 +1864,34 @@ def test_pass_through_is_read_from_the_parsed_definition(definition: str) -> Non assert _field_reference_findings(document) == [] -def test_pass_through_compares_names_as_written() -> None: +def test_pass_through_compares_names_without_regard_to_case() -> None: document = _renamed_model() document["datasets"][0]["fields"] += [ _field("amount", "AMOUNT"), _field("amount_cents", "amount * 100"), ] + assert _field_reference_findings(document) == [] + + +def test_field_shadowing_check_compares_names_without_regard_to_case() -> None: + document = _renamed_model() + document["datasets"][0]["fields"].append(_field("total_eur", "Total_USD * 0.9")) + errors = _field_reference_findings(document) + assert len(errors) == 1 - assert "defined as 'AMOUNT'" in errors[0] + assert "'Total_USD' is the source column, not the field 'total_usd'" in errors[0] + + +def test_field_shadowing_check_reads_a_date_part_as_a_unit_not_a_column() -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("day", "EXTRACT(DAY FROM order_date)"), + _field("month", "EXTRACT(MONTH FROM order_date)"), + _field("days_open", "DATEDIFF(day, opened_at, closed_at)"), + _field("months_open", "DATEDIFF(month, opened_at, closed_at)"), + ] + assert _field_reference_findings(document) == [] def test_field_shadowing_check_needs_a_sql_definition_to_compare() -> None: @@ -2038,9 +2057,15 @@ def test_renamed_fields_example_passes_the_field_reference_check() -> None: @pytest.mark.parametrize("expression", [ "DATEDIFF(day, orders.shipped, orders.ordered)", + "DATEDIFF(month, orders.shipped, orders.ordered)", + "DATEDIFF(days, orders.shipped, orders.ordered)", "TIMESTAMPDIFF(MINUTE, orders.shipped, orders.ordered)", + "TIMESTAMPDIFF(MONTH, orders.shipped, orders.ordered)", "DATEADD(day, 1, orders.ordered)", + "DATEADD(dd, 1, orders.ordered)", + "DATEADD(month, 1, orders.ordered)", "DATE_PART(year, orders.ordered)", + "DATE_PART(month, orders.ordered)", ]) def test_metric_check_reads_a_date_part_as_a_unit_not_a_column(expression) -> None: document = _renamed_model(metrics=[_metric("age", expression)]) @@ -2049,6 +2074,89 @@ def test_metric_check_reads_a_date_part_as_a_unit_not_a_column(expression) -> No assert _VALIDATE.validate_metric_references(document) == [] +def test_metric_check_compares_names_without_regard_to_case() -> None: + document = _renamed_model(metrics=[ + _metric("Revenue", "SUM(ORDERS.TOTAL_USD)"), + _metric("revenue_eur", "SUM(ORDERS.TOTAL_EUR)"), + _metric("doubled", "revenue * 2"), + ]) + + errors = _VALIDATE.validate_metric_references(document) + + assert len(errors) == 2 + assert "'ORDERS.TOTAL_EUR' is not a declared field of dataset 'ORDERS'" in errors[0] + assert "'revenue' names the metric 'Revenue'" in errors[1] + + +def test_datasets_that_differ_only_in_case_pool_their_fields() -> None: + # Whether two such names may coexist is validate_unique_names' question; + # the Name Resolution checks must not warn about either one's own fields. + document = _renamed_model() + document["datasets"].append( + {"name": "Orders", "source": "db.s.orders2", "primary_key": ["order_id"], "fields": [_field("oid", "k")]} + ) + assert _field_reference_findings(document) == [] + + +def test_a_field_passing_through_its_own_column_in_another_case_is_not_shadowing() -> None: + document = _renamed_model() + document["datasets"][0]["fields"].append(_field("Total_USD", "total_usd")) + + assert _field_reference_findings(document) == [] + + +def test_fields_that_differ_only_in_case_compare_against_the_first_declared() -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("Total_USD", "total_cents"), + _field("total_eur", "total_usd * 0.9"), + ] + + errors = _field_reference_findings(document) + + assert len(errors) == 1 + assert "not the field 'total_usd' defined as 'total_cents / 100.0'" in errors[0] + + +def test_a_column_named_like_a_unit_outside_the_unit_position_is_a_column() -> None: + document = _renamed_model(metrics=[ + _metric("age", "SUM(DATEDIFF(day, ms, orders.total_usd))"), + _metric("month_start", "MIN(DATE_TRUNC(month, d))"), + ]) + document["datasets"][0]["fields"] += [_field("q", "quarter_start"), _field("closed_at")] + document["datasets"][0]["fields"].append(_field("open_days", "DATEDIFF(day, q, closed_at)")) + + errors = _field_reference_findings(document) + + assert [e.split(": ", 2)[2] for e in errors] == [ + "'ms' is not qualified; metric expressions refer to fields as dataset.field", + "'d' is not qualified; metric expressions refer to fields as dataset.field", + "'q' is the source column, not the field 'q' defined as 'quarter_start'", + ] + + +def test_a_parser_that_knows_the_unit_leaves_the_date_arguments_as_columns() -> None: + # Snowflake reads the unit itself and puts the last date argument where + # the default parser puts the unit, so `d` must still be seen. + document = _renamed_model(metrics=[ + _metric("ok", "SUM(DATEDIFF(day, orders.total_usd, orders.total_usd))", dialect="SNOWFLAKE"), + _metric("bad", "SUM(DATEDIFF(day, orders.total_usd, d))", dialect="SNOWFLAKE"), + ]) + + errors = _VALIDATE.validate_metric_references(document) + + assert len(errors) == 1 + assert "'d' is not qualified" in errors[0] + + +def test_key_check_compares_names_without_regard_to_case() -> None: + document = _renamed_model() + document["datasets"][0]["primary_key"] = ["ORDER_ID"] + document["relationships"][0]["from_columns"] = ["Customer_Id"] + + assert _VALIDATE.validate_key_references(document) == [] + + def test_metric_check_still_reports_a_bare_column_named_like_a_unit() -> None: document = _renamed_model(metrics=[_metric("days", "SUM(day)")]) diff --git a/validation/validate.py b/validation/validate.py index 378b840f..db80fa8b 100644 --- a/validation/validate.py +++ b/validation/validate.py @@ -93,6 +93,7 @@ try: import sqlglot from sqlglot import exp + from sqlglot.dialects.dialect import Dialect from sqlglot.errors import ParseError, TokenError SQLGLOT_AVAILABLE = True except ImportError: @@ -332,19 +333,31 @@ def validate_relationship_column_arity(data: dict) -> list[str]: return errors +def fold(name: str) -> str: + """The form regular identifiers are compared in: they are case-insensitive + (expression_language.md), so `id`, `Id` and `ID` are one name. Upper-casing + also merges a few non-ASCII pairs, which a warning can live with.""" + return name.upper() + + def declared_fields(model: dict) -> dict[str, set[str]]: - """The names of the fields each dataset declares, by dataset name. + """The folded names of the fields each dataset declares, by folded dataset name. A dataset without a fields list declares none: model-level constructs do not reach through a dataset to its source (spec.md, Name Resolution). """ - return { - d["name"]: { - f.get("name") for f in as_list(d.get("fields")) if isinstance(f, dict) and f.get("name") - } - for d in as_list(model.get("datasets")) - if isinstance(d, dict) and d.get("name") - } + fields: dict[str, set[str]] = {} + for d in as_list(model.get("datasets")): + if not isinstance(d, dict) or not isinstance(d.get("name"), str): + continue + # Names that differ only by case are left to validate_unique_names to + # judge; here they pool their fields rather than hide each other's. + fields.setdefault(fold(d["name"]), set()).update( + fold(f["name"]) + for f in as_list(d.get("fields")) + if isinstance(f, dict) and isinstance(f.get("name"), str) + ) + return fields def sql_dialect_expressions(node: dict, dialect: str | None = None) -> list[tuple[str, str]]: @@ -378,26 +391,46 @@ def parse_sql(expr: str, dialect: str): return None -# Functions sqlglot leaves untyped under the default dialect whose first argument -# is a date part; the typed ones (DateDiff, DateAdd, ...) are matched by class. -DATE_PART_FUNCTIONS = {"DATEADD", "DATE_ADD", "DATEDIFF", "DATE_DIFF", "DATE_PART", "DATEPART"} +# Date functions sqlglot leaves untyped under the default parser; their first +# argument is the unit. DATEDIFF and TIMESTAMPDIFF parse to typed nodes. +DATE_PART_FUNCTIONS = { + "DATEADD", "DATE_ADD", "DATEDIFF", "DATE_DIFF", "DATE_PART", "DATEPART", + "TIMESTAMPADD", "TIMESTAMPDIFF", +} + + +def is_date_part_name(name: str) -> bool: + """Whether a name is a date part or one of its aliases (day, dd, days, ...).""" + parts = Dialect.DATE_PART_MAPPING + return name.upper() in parts or name.upper() in parts.values() def is_date_part(column) -> bool: """Whether a bare name is the unit argument of a date function. - The default parser reads ``day`` in ``DATEDIFF(day, a, b)`` as a column, - while the Snowflake one reads it as a unit. + Only the unit position counts, since the alias table also holds short + names such as q, w and ms that are ordinary column names elsewhere. A + parser that knows the function reads the unit into ``unit`` and the date + arguments into ``this`` and ``expression``; the default parser does not + know DATEDIFF's unit, so it leaves the written unit as the column in + ``this`` and reads the last argument as ``unit`` instead. The two are told + apart by whether ``unit`` names a date part; when the last argument is + itself named like one, the two readings cannot be told apart and the + column in ``this`` is reported. Functions the parser leaves anonymous + carry the unit as their first argument. DATE_ADD, DATE_TRUNC and their + kin never parse the unit as a column. """ exp = sqlglot.exp parent = column.parent - typed = (exp.DateDiff, exp.TimestampDiff, exp.DateAdd, exp.DateSub, exp.TimestampAdd, - exp.TimestampSub, exp.DateTrunc, exp.TimestampTrunc) - anonymous = isinstance(parent, exp.Anonymous) and parent.name.upper() in DATE_PART_FUNCTIONS - if column.table or not (isinstance(parent, typed) or anonymous): + if column.table or not is_date_part_name(column.name): return False - units = exp.TimeUnit.UNABBREVIATED_UNIT_NAME - return column.name.upper() in units or column.name.upper() in units.values() + if isinstance(parent, (exp.DateDiff, exp.TimestampDiff)): + unit = parent.args.get("unit") + unit_consumed = unit is not None and is_date_part_name(unit.name) + return column.arg_key == "this" and not unit_consumed + if isinstance(parent, exp.Anonymous) and parent.name.upper() in DATE_PART_FUNCTIONS: + return bool(parent.expressions) and parent.expressions[0] is column + return False def validate_key_references(data: dict) -> list[str]: @@ -412,12 +445,15 @@ def validate_key_references(data: dict) -> list[str]: def undeclared(dataset_name: str, columns) -> list[str]: names = [c for c in as_list(columns) if isinstance(c, str)] - return [c for c in dict.fromkeys(names) if c not in fields.get(dataset_name, set())] + declared = fields.get(fold(dataset_name), set()) + return [c for c in dict.fromkeys(names) if fold(c) not in declared] for dataset in as_list(data.get("datasets")): - if not isinstance(dataset, dict) or dataset.get("name") not in fields: + if not isinstance(dataset, dict) or not isinstance(dataset.get("name"), str): continue name = dataset["name"] + if fold(name) not in fields: + continue context = f"Dataset '{name}' in model '{model_name}'" for col in undeclared(name, dataset.get("primary_key")): warnings.append(ValidationWarning( @@ -437,7 +473,7 @@ def undeclared(dataset_name: str, columns) -> list[str]: context = f"Relationship '{rel.get('name', '')}' in model '{model_name}'" for side, endpoint in (("from_columns", "from"), ("to_columns", "to")): dataset_name = rel.get(endpoint) - if not isinstance(dataset_name, str) or dataset_name not in fields: + if not isinstance(dataset_name, str) or fold(dataset_name) not in fields: continue # reported by validate_references for col in undeclared(dataset_name, rel.get(side)): warnings.append(ValidationWarning( @@ -459,14 +495,18 @@ def validate_metric_references(data: dict) -> list[str]: fields = declared_fields(data) field_names = set().union(*fields.values()) if fields else set() metrics = [m for m in as_list(data.get("metrics")) if isinstance(m, dict)] - metric_names = {m.get("name") for m in metrics} + declared_metrics = {fold(m["name"]): m["name"] for m in metrics if isinstance(m.get("name"), str)} warnings = [] for metric in metrics: metric_name = metric.get("name", "") + own_name = fold(metric_name) if isinstance(metric.get("name"), str) else None # A bare name that is another metric, and not also a field or this # metric itself, was most likely meant as that metric. - other_metrics = metric_names - field_names - {metric_name} + other_metrics = { + folded: name for folded, name in declared_metrics.items() + if folded not in field_names and folded != own_name + } for dialect, expr in sql_dialect_expressions(metric): tree = parse_sql(expr, dialect) if tree is None: @@ -475,28 +515,29 @@ def validate_metric_references(data: dict) -> list[str]: for column in tree.find_all(sqlglot.exp.Column): if isinstance(column.this, sqlglot.exp.Star) or is_date_part(column): continue + name, table = fold(column.name), fold(column.table) if column.db: written = ".".join(part.name for part in column.parts) message = ( f"'{written}' has more than two parts; metric expressions refer to " f"fields as dataset.field" ) - elif not column.table and column.name in other_metrics: + elif not column.table and name in other_metrics: message = ( - f"'{column.name}' names the metric '{column.name}'; metric expressions " - f"cannot refer to other metrics" + f"'{column.name}' names the metric '{other_metrics[name]}'; " + f"metric expressions cannot refer to other metrics" ) elif not column.table: message = ( f"'{column.name}' is not qualified; metric expressions refer to fields " f"as dataset.field" ) - elif column.table not in fields: + elif table not in fields: message = ( f"'{column.table}.{column.name}' references unknown dataset " f"'{column.table}'" ) - elif column.name not in fields[column.table]: + elif name not in fields[table]: message = ( f"'{column.table}.{column.name}' is not a declared field of dataset " f"'{column.table}'" @@ -525,7 +566,7 @@ def validate_field_shadowing(data: dict) -> list[str]: def passes_through(tree, name: str) -> bool: while isinstance(tree, (sqlglot.exp.Paren, sqlglot.exp.Alias)): tree = tree.this - return isinstance(tree, sqlglot.exp.Column) and not tree.table and tree.name == name + return isinstance(tree, sqlglot.exp.Column) and not tree.table and fold(tree.name) == fold(name) for dataset in as_list(data.get("datasets")): if not isinstance(dataset, dict): @@ -535,16 +576,23 @@ def passes_through(tree, name: str) -> bool: f for f in as_list(dataset.get("fields")) if isinstance(f, dict) and isinstance(f.get("name"), str) ] - by_name = {f["name"]: f for f in declared} + # Of two fields whose names differ only by case, the first declared is + # the one other expressions are compared against. + by_name: dict[str, dict] = {} + for f in declared: + by_name.setdefault(fold(f["name"]), f) for field in declared: for dialect, expr in sql_dialect_expressions(field): 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] + names = [ + c.name for c in tree.find_all(sqlglot.exp.Column) + if not c.table and not is_date_part(c) + ] for name in dict.fromkeys(names): - other = by_name.get(name) - if other is None or other is field: + other = by_name.get(fold(name)) + if other is None or fold(other["name"]) == fold(field["name"]): continue definitions = ( sql_dialect_expressions(other, dialect) or sql_dialect_expressions(other) @@ -559,7 +607,7 @@ def passes_through(tree, name: str) -> bool: ) warnings.append(ValidationWarning( f"[Reference] Warning: {context}: '{name}' is the source column, not the " - f"field '{name}' defined as '{parsed[0][0]}'" + f"field '{other['name']}' defined as '{parsed[0][0]}'" )) return warnings From b17515add1d781876dc34cee8ac4fec30627dfac Mon Sep 17 00:00:00 2001 From: Kyoung Min Kim Date: Mon, 26 Oct 2026 13:06:24 +0900 Subject: [PATCH 3/4] refactor(validation): read the reference checks through iter_expressions and the shared parser The field-reference checks had their own walk over `expression.dialects` and their own parse_one call. Both now go through the helpers the other checks use: read_expressions wraps _expressions_in, so the top-level `sql` is read as OSSIE_SQL_2026 and an OSSIE_SQL_2026 dialect entry that a `sql` supersedes is skipped; parse_tree wraps _parse_sql_expression, so a dialect outside SQLGLOT_DIALECTS is left to the unsupported-dialect warning instead of being parsed as ANSI, and the single-expression and SELECT-retry handling reach these checks too. _expressions_in skips the shapes the schema rejects, so a check called directly on an unvetted document reports nothing. Generated-by: Claude Code --- validation/tests/test_validate.py | 48 ++++++++++++++++ validation/validate.py | 94 ++++++++++++++++--------------- 2 files changed, 96 insertions(+), 46 deletions(-) diff --git a/validation/tests/test_validate.py b/validation/tests/test_validate.py index 6dbbf747..eae6e23e 100644 --- a/validation/tests/test_validate.py +++ b/validation/tests/test_validate.py @@ -2205,3 +2205,51 @@ def test_cli_prints_reference_warnings_and_still_passes(run_validator) -> None: "[Reference] Warning: Dataset 'orders' in model 'm': primary_key names 'o_orderkey', " "which is not a declared field" ) in output + + +def test_metric_check_reads_the_portable_sql_form() -> None: + document = _renamed_model(metrics=[{"name": "revenue", "expression": {"sql": "SUM(total_usd)"}}]) + + errors = _VALIDATE.validate_metric_references(document) + + assert errors == [ + "[Reference] Warning: Metric 'revenue' in model 'm' (OSSIE_SQL_2026): 'total_usd' is not " + "qualified; metric expressions refer to fields as dataset.field" + ] + + +def test_metric_check_skips_a_dialect_entry_the_portable_sql_supersedes() -> None: + # The top-level `sql` is what a client reads, so the OSSIE_SQL_2026 dialect + # entry beside it is not checked twice under a second name. + document = _renamed_model(metrics=[{ + "name": "revenue", + "expression": { + "sql": "SUM(orders.total_usd)", + "dialects": [{"dialect": "OSSIE_SQL_2026", "expression": "SUM(total_usd)"}], + }, + }]) + + assert _VALIDATE.validate_metric_references(document) == [] + + +def test_reference_checks_skip_a_dialect_this_validator_does_not_parse() -> None: + # A dialect outside SQLGLOT_DIALECTS is carried, not read: validate_sql warns + # once that it went unchecked, and these checks say nothing about it. + document = _renamed_model(metrics=[_metric("revenue", "SUM(total_usd)", dialect="DUCKDB")]) + document["datasets"][0]["fields"].append(_field("total_eur", "total_usd * 0.9", dialect="DUCKDB")) + + assert _field_reference_findings(document) == [] + + +def test_field_shadowing_check_reads_the_portable_sql_form() -> None: + document = _renamed_model() + document["datasets"][0]["fields"].append( + {"name": "total_eur", "expression": {"sql": "total_usd * 0.9"}} + ) + + errors = _VALIDATE.validate_field_shadowing(document) + + assert errors == [ + "[Reference] Warning: Field 'orders.total_eur' in model 'm' (OSSIE_SQL_2026): 'total_usd' is " + "the source column, not the field 'total_usd' defined as 'total_cents / 100.0'" + ] diff --git a/validation/validate.py b/validation/validate.py index db80fa8b..6db3c020 100644 --- a/validation/validate.py +++ b/validation/validate.py @@ -360,37 +360,6 @@ def declared_fields(model: dict) -> dict[str, set[str]]: return fields -def sql_dialect_expressions(node: dict, dialect: str | None = None) -> list[tuple[str, str]]: - """The (dialect, expression) pairs of a field or metric that sqlglot can - parse, or only those in one dialect when it is given.""" - expression = node.get("expression") - if not isinstance(expression, dict): - return [] - found = [] - for dialect_expr in as_list(expression.get("dialects")): - if not isinstance(dialect_expr, dict): - continue - d = dialect_expr.get("dialect", "ANSI_SQL") - expr = dialect_expr.get("expression") - if not isinstance(expr, str) or not expr or d not in SQLGLOT_DIALECTS: - continue - if dialect is None or d == dialect: - found.append((d, expr)) - return found - - -def parse_sql(expr: str, dialect: str): - """The sqlglot tree of expr, or None when it is not one expression. - - The SQL check reports what does not parse at all. What parses only as a - SELECT item, such as ``DISTINCT x``, passes that check and is skipped here. - """ - try: - return sqlglot.parse_one(expr, dialect=SQLGLOT_DIALECTS.get(dialect)) - except (ParseError, TokenError, RecursionError): - return None - - # Date functions sqlglot leaves untyped under the default parser; their first # argument is the unit. DATEDIFF and TIMESTAMPDIFF parse to typed nodes. DATE_PART_FUNCTIONS = { @@ -507,11 +476,11 @@ def validate_metric_references(data: dict) -> list[str]: folded: name for folded, name in declared_metrics.items() if folded not in field_names and folded != own_name } - for dialect, expr in sql_dialect_expressions(metric): - tree = parse_sql(expr, dialect) + for expression in read_expressions(metric, "model_metric"): + context = expression.context(model_name) + tree = parse_tree(expression.text, expression.dialect, context) if tree is None: continue - context = f"Metric '{metric_name}' in model '{model_name}' ({dialect})" for column in tree.find_all(sqlglot.exp.Column): if isinstance(column.this, sqlglot.exp.Star) or is_date_part(column): continue @@ -582,8 +551,9 @@ def passes_through(tree, name: str) -> bool: for f in declared: by_name.setdefault(fold(f["name"]), f) for field in declared: - for dialect, expr in sql_dialect_expressions(field): - tree = parse_sql(expr, dialect) + for expression in read_expressions(field, "field", dataset_name): + context = expression.context(model_name) + tree = parse_tree(expression.text, expression.dialect, context) if tree is None: continue names = [ @@ -595,16 +565,18 @@ def passes_through(tree, name: str) -> bool: if other is None or fold(other["name"]) == fold(field["name"]): continue definitions = ( - sql_dialect_expressions(other, dialect) or sql_dialect_expressions(other) + read_expressions(other, "field", dataset_name, expression.dialect) + or read_expressions(other, "field", dataset_name) ) # A definition that does not parse gives nothing to compare. - parsed = [(e, t) for d, e in definitions if (t := parse_sql(e, d)) is not None] + parsed = [ + (d.text, tree) + for d in definitions + if (tree := parse_tree(d.text, d.dialect, d.context(model_name))) + is not None + ] if not parsed or any(passes_through(t, name) for _, t in parsed): continue - context = ( - f"Field '{dataset_name}.{field['name']}' in model '{model_name}' " - f"({dialect})" - ) warnings.append(ValidationWarning( f"[Reference] Warning: {context}: '{name}' is the source column, not the " f"field '{other['name']}' defined as '{parsed[0][0]}'" @@ -705,6 +677,13 @@ def validate_sql_expression(expr: str, dialect: str, context: str) -> str | None return error +def parse_tree(expr: str, dialect: str, context: str): + """The parsed tree of one expression, or None when it does not parse: for the + checks that read the columns and leave the error to the SQL check.""" + tree, _error = _parse_sql_expression(expr, dialect, context) + return tree + + ExpressionKind = Literal["field", "dataset_metric", "model_metric"] @@ -742,7 +721,11 @@ def context(self, model_name: str) -> str: def _expressions_in( member: dict, kind: ExpressionKind, dataset_name: str, field_names: frozenset[str] ) -> Iterator[Expression]: - expression = member.get("expression") or {} + # Shapes the schema rejects are skipped, not read: the reference checks + # run on documents the schema check has already failed. + expression = member.get("expression") + if not isinstance(expression, dict): + return name = member.get("name", "") sql = expression.get("sql") if sql is not None: @@ -754,11 +737,13 @@ def _expressions_in( text=sql, field_names=field_names, ) - for dialect_expr in expression.get("dialects", []): + for dialect_expr in as_list(expression.get("dialects")): + if not isinstance(dialect_expr, dict): + continue text = dialect_expr.get("expression") - if text is None: + dialect = dialect_expr.get("dialect") + if text is None or not isinstance(dialect, str): continue - dialect = dialect_expr["dialect"] yield Expression( kind=kind, dataset_name=dataset_name, @@ -789,6 +774,23 @@ def iter_expressions(data: dict) -> Iterator[Expression]: yield from _expressions_in(metric, "model_metric", "", frozenset()) +def read_expressions( + member: dict, kind: ExpressionKind, dataset_name: str = "", dialect: str | None = None +) -> list[Expression]: + """The expressions of one field or metric that the reference checks read: the + top-level `sql`, as OSSIE_SQL_2026, and the dialect entries, less an + OSSIE_SQL_2026 entry that a top-level `sql` supersedes. Given a dialect, only + the expressions in it. Whether one parses is parse_tree's call. + """ + return [ + expression + for expression in _expressions_in(member, kind, dataset_name, frozenset()) + if expression.text + and expression.superseded_by is None + and (dialect is None or expression.dialect == dialect) + ] + + def validate_sql(data: dict) -> list[str]: """Validate SQL expressions in fields and metrics.""" # Only core semantic model documents contain a root datasets property. From ed115833a65e04e65e0352cf2ff27edfd4e1e9e6 Mon Sep 17 00:00:00 2001 From: Kyoung Min Kim Date: Mon, 26 Oct 2026 13:07:09 +0900 Subject: [PATCH 4/4] fix(validation): walk columns the same way in every check, units included validate_metric_scoping walked tree.find_all(exp.Column) without the is_date_part filter the reference checks apply, so a dataset-scoped metric in the spec's own DATEDIFF form was rejected for referencing the field MINUTE. The three checks now read columns through one iter_columns: `*` and the unit of a date function are no reference, and the unit slot the default parser fills with the last DATEDIFF argument, a Var that escaped every check, is read back as the bare column it holds. The scoping check also folds names the way the reference checks do. Generated-by: Claude Code --- validation/tests/test_validate.py | 114 ++++++++++++++++++++++++++++++ validation/validate.py | 65 +++++++++++------ 2 files changed, 159 insertions(+), 20 deletions(-) diff --git a/validation/tests/test_validate.py b/validation/tests/test_validate.py index eae6e23e..78255970 100644 --- a/validation/tests/test_validate.py +++ b/validation/tests/test_validate.py @@ -2253,3 +2253,117 @@ def test_field_shadowing_check_reads_the_portable_sql_form() -> None: "[Reference] Warning: Field 'orders.total_eur' in model 'm' (OSSIE_SQL_2026): 'total_usd' is " "the source column, not the field 'total_usd' defined as 'total_cents / 100.0'" ] + + +@pytest.mark.parametrize("dialect", ["OSSIE_SQL_2026", "ANSI_SQL", "SNOWFLAKE"]) +def test_dataset_scoped_metric_reads_a_date_part_as_a_unit_not_a_field(dialect) -> None: + # The spec's own DATEDIFF form: MINUTE is the unit, not an undeclared field. + doc = _orders_with( + fields=[{"name": "last_contact_at", "expression": _ds_expr("last_contact_at", dialect)}], + metrics=[{ + "name": "minutes_since_last_contact", + "expression": _ds_expr("DATEDIFF(MINUTE, MAX(last_contact_at), CURRENT_TIMESTAMP)", dialect), + }], + ) + + assert _VALIDATE.validate_metric_scoping(doc) == [] + + +def test_dataset_scoped_metric_still_checks_the_date_arguments() -> None: + doc = _orders_with( + fields=[{"name": "last_contact_at", "expression": _ds_expr("last_contact_at")}], + metrics=[{ + "name": "bad", + "expression": _ds_expr("DATEDIFF(MINUTE, MAX(last_contact_at), closed_at)"), + }], + ) + + errors = _VALIDATE.validate_metric_scoping(doc) + + assert len(errors) == 1 + assert "undeclared field(s) 'CLOSED_AT'" in errors[0] + + +def test_dataset_scoped_metric_compares_names_without_regard_to_case() -> None: + doc = _orders_with( + fields=[{"name": "amount", "expression": _ds_expr("amount")}], + metrics=[{"name": "total_amount", "expression": _ds_expr("SUM(Amount)")}], + ) + + assert _VALIDATE.validate_metric_scoping(doc) == [] + + +def test_metric_check_reads_a_column_in_the_unit_slot_as_a_column() -> None: + # Under the default parser the last DATEDIFF argument lands in `unit` as a + # Var, so without this it would escape every check. + document = _renamed_model(metrics=[ + _metric("age", "SUM(DATEDIFF(day, orders.total_usd, closed_at))"), + ]) + + errors = _VALIDATE.validate_metric_references(document) + + assert len(errors) == 1 + assert "'CLOSED_AT' is not qualified" in errors[0] + + +def test_field_shadowing_check_reads_a_column_in_the_unit_slot_as_a_column() -> None: + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("closed_at", "closed_ts"), + _field("open_days", "DATEDIFF(day, opened_at, closed_at)"), + ] + + errors = _VALIDATE.validate_field_shadowing(document) + + assert errors == [ + "[Reference] Warning: Field 'orders.open_days' in model 'm' (ANSI_SQL): 'CLOSED_AT' is the " + "source column, not the field 'closed_at' defined as 'closed_ts'" + ] + + +def test_validate_module_imports_without_sqlglot(monkeypatch) -> None: + # A None entry in sys.modules makes `import sqlglot` raise ImportError, the + # way a missing install does; the module must still load and say so. + import sys + + for name in [m for m in sys.modules if m == "sqlglot" or m.startswith("sqlglot.")]: + monkeypatch.setitem(sys.modules, name, None) + spec = spec_from_file_location("ossie_validate_without_sqlglot", _VALIDATE_PATH) + module = module_from_spec(spec) + spec.loader.exec_module(module) + + assert module.SQLGLOT_AVAILABLE is False + + +@pytest.mark.parametrize("expression, dialect", [ + ("SUM(DATEDIFF(day, orders.total_usd, '2026-01-01'))", "ANSI_SQL"), + ("SUM(DATEDIFF(day, orders.total_usd, 1))", "ANSI_SQL"), + ("SUM(DATE_DIFF(orders.total_usd, orders.total_usd, ISOWEEK))", "BIGQUERY"), + ("SUM(DATEDIFF(nanoseconds, orders.total_usd, orders.total_usd))", "SNOWFLAKE"), + ("SUM(DATEDIFF(day, orders.total_usd, orders.d))", "ANSI_SQL"), +]) +def test_a_literal_or_an_unlisted_unit_in_the_unit_slot_is_not_a_column(expression, dialect) -> None: + # A literal the default parser leaves in `unit`, a unit alias the table lacks + # under a parser that knows the function, and a declared column named like + # a unit alias in the last slot: none of them is a bare column. + document = _renamed_model(metrics=[_metric("x", expression, dialect=dialect)]) + document["datasets"][0]["fields"].append(_field("d", "delivered_at")) + + assert _VALIDATE.validate_metric_references(document) == [] + + +def test_field_shadowing_check_compares_against_a_definition_that_only_parses_as_a_select_item() -> None: + # `DISTINCT amount` parses only as a projection; it is a definition all the + # same, and not the pass-through of the column `amount`. + document = _renamed_model() + document["datasets"][0]["fields"] += [ + _field("amount", "DISTINCT amount"), + _field("amount_eur", "amount * 0.9"), + ] + + errors = _VALIDATE.validate_field_shadowing(document) + + assert errors == [ + "[Reference] Warning: Field 'orders.amount_eur' in model 'm' (ANSI_SQL): 'amount' is the " + "source column, not the field 'amount' defined as 'DISTINCT amount'" + ] diff --git a/validation/validate.py b/validation/validate.py index 6db3c020..4ae9729f 100644 --- a/validation/validate.py +++ b/validation/validate.py @@ -389,19 +389,47 @@ def is_date_part(column) -> bool: carry the unit as their first argument. DATE_ADD, DATE_TRUNC and their kin never parse the unit as a column. """ - exp = sqlglot.exp parent = column.parent if column.table or not is_date_part_name(column.name): return False if isinstance(parent, (exp.DateDiff, exp.TimestampDiff)): unit = parent.args.get("unit") - unit_consumed = unit is not None and is_date_part_name(unit.name) + unit_consumed = isinstance(unit, exp.Var) and is_date_part_name(unit.name) return column.arg_key == "this" and not unit_consumed if isinstance(parent, exp.Anonymous) and parent.name.upper() in DATE_PART_FUNCTIONS: return bool(parent.expressions) and parent.expressions[0] is column return False +def iter_columns(tree) -> Iterator["exp.Column"]: + """The column references of a parsed expression, read the same way by every + check that walks one: `*` references no column, the unit of a date function + is not one, and a unit slot the parser filled with a column is read as that + column. The default parser does not know DATEDIFF's unit, so in + `DATEDIFF(day, opened_at, closed_at)` it leaves `day` in `this` as a column + and reads `closed_at` into `unit` as a Var. When `this` is a bare column + named like a date part and `unit` is a Var that is not one, the Var is the + column, in the upper-cased spelling the parser keeps; a literal there, or a + unit the alias table lacks under a parser that knows the function, is left + alone. + """ + for column in tree.find_all(exp.Column): + if isinstance(column.this, exp.Star) or is_date_part(column): + continue + yield column + for node in tree.find_all(exp.DateDiff, exp.TimestampDiff): + first, unit = node.this, node.args.get("unit") + if ( + isinstance(first, exp.Column) + and not first.table + and is_date_part_name(first.name) + and isinstance(unit, exp.Var) + and not is_date_part_name(unit.name) + and unit.name.isidentifier() + ): + yield exp.column(unit.name) + + def validate_key_references(data: dict) -> list[str]: """Warn when primary_key, unique_keys or relationship from_columns/to_columns name something other than a declared field of their dataset (spec.md, Name @@ -481,9 +509,7 @@ def validate_metric_references(data: dict) -> list[str]: tree = parse_tree(expression.text, expression.dialect, context) if tree is None: continue - for column in tree.find_all(sqlglot.exp.Column): - if isinstance(column.this, sqlglot.exp.Star) or is_date_part(column): - continue + for column in iter_columns(tree): name, table = fold(column.name), fold(column.table) if column.db: written = ".".join(part.name for part in column.parts) @@ -533,9 +559,9 @@ def validate_field_shadowing(data: dict) -> list[str]: warnings = [] def passes_through(tree, name: str) -> bool: - while isinstance(tree, (sqlglot.exp.Paren, sqlglot.exp.Alias)): + while isinstance(tree, (exp.Paren, exp.Alias)): tree = tree.this - return isinstance(tree, sqlglot.exp.Column) and not tree.table and fold(tree.name) == fold(name) + return isinstance(tree, exp.Column) and not tree.table and fold(tree.name) == fold(name) for dataset in as_list(data.get("datasets")): if not isinstance(dataset, dict): @@ -556,10 +582,7 @@ def passes_through(tree, name: str) -> bool: tree = parse_tree(expression.text, expression.dialect, context) if tree is None: continue - names = [ - c.name for c in tree.find_all(sqlglot.exp.Column) - if not c.table and not is_date_part(c) - ] + names = [c.name for c in iter_columns(tree) if not c.table] for name in dict.fromkeys(names): other = by_name.get(fold(name)) if other is None or fold(other["name"]) == fold(field["name"]): @@ -694,7 +717,8 @@ class Expression: `kind` identifies the declaring element, and `name` identifies the field or metric. `dialect` is OSSIE_SQL_2026 for portable `sql` expressions, or the dialect's label otherwise. `field_names` contains the enclosing - dataset's field names, or is empty for model-scoped metrics. + dataset's field names as fold() compares them, or is empty for model-scoped + metrics. `superseded_by` is the authoritative top-level `sql` text when this is an OSSIE_SQL_2026 dialect entry that `sql` outranks, else None: such an entry is non-authoritative, since a client reads the `sql` instead. @@ -721,8 +745,8 @@ def context(self, model_name: str) -> str: def _expressions_in( member: dict, kind: ExpressionKind, dataset_name: str, field_names: frozenset[str] ) -> Iterator[Expression]: - # Shapes the schema rejects are skipped, not read: the reference checks - # run on documents the schema check has already failed. + # Shapes the schema rejects are skipped, not read, so a check called directly + # on an unvetted document reports nothing rather than failing. expression = member.get("expression") if not isinstance(expression, dict): return @@ -764,7 +788,7 @@ def iter_expressions(data: dict) -> Iterator[Expression]: for dataset in data.get("datasets", []): dataset_name = dataset.get("name", "") field_names = frozenset( - f["name"].casefold() for f in dataset.get("fields", []) if f.get("name") + fold(f["name"]) for f in dataset.get("fields", []) if f.get("name") ) for field in dataset.get("fields", []): yield from _expressions_in(field, "field", dataset_name, field_names) @@ -785,7 +809,8 @@ def read_expressions( return [ expression for expression in _expressions_in(member, kind, dataset_name, frozenset()) - if expression.text + if isinstance(expression.text, str) + and expression.text and expression.superseded_by is None and (dialect is None or expression.dialect == dialect) ] @@ -864,8 +889,8 @@ def validate_metric_scoping(data: dict) -> list[str]: in the model-root metrics. * Every field a dataset-scoped metric references must be a declared field of the dataset; referencing an undeclared name (e.g. a raw source column) is - not yet allowed. A bare name is compared case-insensitively; `COUNT(*)` and - literals reference no field and are fine. + not yet allowed. A bare name is compared case-insensitively; `COUNT(*)`, + literals and the unit of a date function reference no field and are fine. Checking the expression needs sqlglot; dialects it cannot parse are skipped rather than reported. @@ -885,11 +910,11 @@ def scope_errors(expression: Expression) -> list[str]: qualifiers = set() undeclared = set() - for col in tree.find_all(exp.Column): + for col in iter_columns(tree): parts = [part.name for part in col.parts] if len(parts) > 1 and parts[0]: qualifiers.add(parts[0]) - elif parts and parts[0].casefold() not in expression.field_names: + elif parts and fold(parts[0]) not in expression.field_names: undeclared.add(parts[0]) prefix = (