Skip to content

Warn when relationship to_columns does not cover a declared key - #330

Merged
khush-bhatia merged 1 commit into
apache:mainfrom
kayemkim:validate-to-columns-key-coverage
Aug 28, 2026
Merged

Warn when relationship to_columns does not cover a declared key#330
khush-bhatia merged 1 commit into
apache:mainfrom
kayemkim:validate-to-columns-key-coverage

Conversation

@kayemkim

Copy link
Copy Markdown
Contributor

Summary

The spec defines to_columns as "Primary/unique key columns in the 'to' dataset" (core-spec/spec.md, osi-schema.json), but the validator only checks that the relationship's datasets exist. A relationship that joins to non-key columns silently breaks many-to-one semantics downstream — joins fan out, and consumers that trust the declared cardinality produce wrong results. #301 shows this class of document being produced in practice and rejected by downstream consumers; a validator-side check catches it at the document level regardless of which tool produced it.

validate_references now checks that to_columns covers the to dataset's primary_key or one of its unique_keys. Two deliberate softenings, both drawn from how the repo already treats this rule:

  • Coverage rather than exact equality: a to_columns that is a superset of a key still guarantees the join cardinality (tenant-sharded joins are a common shape), and the Databricks converter's _covers_unique_key already applies exactly these semantics.
  • Warning rather than error: declared keys can be a partial recovery of the dataset's real keys (the Databricks importer, for instance, records unique_keys from a single join's rely hint), so a non-covering to_columns is suspicious but not provably wrong. The message goes through the validator's existing warning channel and does not fail validation. Happy to tighten this to an error if you'd rather trust key declarations as exhaustive.

Datasets that declare no keys are skipped — both primary_key and unique_keys are optional, so there is nothing to check against. The check also guards against shapes that already fail schema validation (null unique_keys, non-list to_columns, flat unique_keys) so the semantic pass can't crash or mislead on an invalid document.

Ran against every semantic-model YAML in the repo (examples plus converter fixtures): no regressions, all still pass. A relationship pointing at non-key columns now reports:

[Reference] Warning: Relationship 'orders_to_customers' in model 'm': to_columns ['region'] does not cover the primary key or a unique key of dataset 'customers'

A couple of adjacent gaps I noticed but left out of scope, flagging in case they're worth issues: no CI workflow currently runs validation/tests (this suite included), and the orionbelt converter's mirror validator (which documents itself as mirroring validate.py) doesn't have this rule — I can follow up on either. #307 (pending) adds from/to column count validation to the same function and introduces the same test file path; the checks are complementary and I'm happy to rebase if it lands first.

Related Issues

Related to #301 (validator-side guard for the same class of invalid document; does not fix the converter itself).

Checklist

Specification

  • Spec changes are included in core-spec/ and follow the existing structure
  • Spec changes have been discussed on the mailing list or in a linked issue
  • Breaking changes to the spec are clearly called out in the summary

Ontology

  • Ontology changes in ontology/ are consistent with spec changes
  • New or modified terms are defined and documented

Converters

  • Converter logic in converters/ is updated to reflect spec or ontology changes
  • New converters include tests under the converter's test directory

Validation

  • Validation rules in validation/ are updated if the spec changed
  • New validation cases are covered by tests

Documentation

  • docs/ is updated to reflect any user-facing changes
  • New features or behaviors are documented with examples where appropriate
  • CONTRIBUTING.md is updated if the contribution process changed

Examples

  • examples/ are added or updated for any new spec constructs or converter support

Tests

  • All existing tests pass (pytest / CI green)
  • New functionality is covered by tests

Compliance

  • ASF license headers are present on all new source files
  • No third-party dependencies are added without PMC/IPMC approval

The spec defines to_columns as "Primary/unique key columns in the 'to'
dataset", but the validator only checked that relationship datasets
exist. A relationship joining to non-key columns breaks many-to-one
semantics downstream (see apache#301 for a converter emitting exactly this
and consumers rejecting it).

validate_references now checks that to_columns covers the to dataset's
primary_key or one of its unique_keys. Coverage rather than exact
equality: a superset of a key still guarantees the join cardinality,
and the databricks converter's _covers_unique_key already applies the
same semantics. Reported as a warning rather than an error because
declared keys may be an incomplete recovery of the dataset's real
keys, and datasets that declare no keys are skipped entirely.

Shape guards keep the semantic check from crashing or misreporting on
documents that already fail schema validation (null unique_keys,
non-list to_columns, flat unique_keys).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: km <kayemkim@gmail.com>
@kayemkim

Copy link
Copy Markdown
Contributor Author

Two updates since opening this.

#337 touches the same validate.py (rejecting duplicate mapping keys) and adds the validation CI workflow whose absence I flagged in the summary. That workflow runs uv run validation/test_validate.py directly, while this PR keeps its tests at validation/tests/test_validate.py, so they would not be picked up as-is. If #337 lands first I'll rebase and move the tests to match its layout so the new check actually runs in CI. The validate.py changes themselves should not conflict: #337 changes the YAML loading path, this PR changes validate_references.

Also a correction to the summary above: "all still pass" overstates it. A few repo files fail validation on main as well, for reasons unrelated to this change: examples/flights.yaml is an ontology-layer document (it passes against ontology/ontology.json), and the orionbelt fixtures declare version: 0.2.0.dev0, which the core schema rejects. Those fail identically with and without this change, so the accurate claim is no regressions, not a clean pass.

@khush-bhatia
khush-bhatia merged commit 4cf6555 into apache:main Aug 28, 2026
jklahr pushed a commit to jklahr/jklahr-ossie that referenced this pull request Aug 29, 2026
Extends the validator test suite added in apache#330 with cases for the metric
scoping and metric name checks.

Each test under "reported in review" corresponds to a defect found in review of
apache#343 and fails against the validator as it stood before that review: raw-cased
qualifier comparison, three-part STRUCT paths read as dataset references, local
aliases and subquery sources reported as cross-dataset references, a traceback
on an explicitly null expression, non-deterministic collision output, and the
missing field/metric name collision check.

Also covers the deliberately permitted cases, so a later change does not
constrain them by accident: two datasets may reuse a metric name, and a
model-scoped metric may take the name of a field or of a dataset.

Follows the module-loading and importorskip pattern established by the existing
tests. sqlglot is skipped rather than asserted, since the scoping checks no-op
without it and would otherwise pass without asserting anything.

Assisted-by: Cortex Code <noreply@snowflake.com>
@jklahr jklahr mentioned this pull request Aug 29, 2026
17 tasks
jklahr pushed a commit to jklahr/jklahr-ossie that referenced this pull request Aug 29, 2026
Rule 1 said a dataset-scoped metric's expression references fields of its
dataset. That forces an author to declare a field for a column they only
want to aggregate, and then hide it. It also narrowed something the spec
already states: the Fields section describes fields as "row-level
attributes that can be used for grouping, filtering, and in metric
expressions".

The expression now reaches both namespaces, with a distinct spelling for
each. A declared field is written dataset_name.field_name, which is how a
metric reuses a field's expression instead of repeating it. A column of
the source is written unqualified.

Two spellings rather than one shared namespace avoids a shadowing rule. A
field and a source column may share a name without ambiguity, so
declaring a field named after an existing column does not change the
meaning of an expression already using the bare name.

This replaces the hard error on self-qualification, since orders.amount
now means the declared field amount, with a check that a qualified
reference names a declared field of the declaring dataset. That is
verifiable from the model, so SUM(orders.tax) is reported and points the
author at SUM(tax). Whether a bare name is a real column stays unchecked,
because it needs catalog metadata the model does not carry, and nothing
else in validate.py checks a field's expression against real columns
either.

validate.py: _leading_qualifiers becomes _qualified_references, returning
(qualifier, name) pairs so the name a qualifier introduces can be
resolved against the dataset's field list.

spec.md: the worked example now demonstrates both spellings on a derived
field rather than two identity fields, and the second INVALID case is a
qualified reference to an undeclared name instead of a self-qualifier.

Tests: 21 functions added overall, collecting 32 cases with the 10 from
 apache#330. Reverting only validate.py to 592db69 fails 11 of the added
functions and none of the 10 pre-existing ones.

Assisted-by: Cortex Code <noreply@snowflake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants