Warn when relationship to_columns does not cover a declared key - #330
Open
kayemkim wants to merge 1 commit into
Open
Warn when relationship to_columns does not cover a declared key#330kayemkim wants to merge 1 commit into
kayemkim wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The spec defines
to_columnsas "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_referencesnow checks thatto_columnscovers thetodataset'sprimary_keyor one of itsunique_keys. Two deliberate softenings, both drawn from how the repo already treats this rule:to_columnsthat 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_keyalready applies exactly these semantics.unique_keysfrom a single join'srelyhint), so a non-coveringto_columnsis 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_keyandunique_keysare optional, so there is nothing to check against. The check also guards against shapes that already fail schema validation (nullunique_keys, non-listto_columns, flatunique_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:
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 mirroringvalidate.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
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changesConverters
converters/is updated to reflect spec or ontology changesValidation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changedExamples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green)Compliance