Skip to content

fix(validation): enforce equal arity for relationship column arrays - #375

Open
christianeu-db wants to merge 1 commit into
apache:mainfrom
christianeu-db:fix-relationship-column-arity
Open

christianeu-db wants to merge 1 commit into
apache:mainfrom
christianeu-db:fix-relationship-column-arity

Conversation

@christianeu-db

@christianeu-db christianeu-db commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Enforce the spec's already-documented requirement that a relationship's from_columns and to_columns have the same number of columns.

$defs/Relationship constrains each array with minItems: 1 but not their relative length, so a document with mismatched arity — describing an incomplete tuple join (e.g. from_columns: [a, b] with to_columns: [c]) — validated successfully. core-spec/spec.md ("Relationships") already states the two arrays must correspond positionally and have the same number of columns; JSON Schema cannot express this cross-array equality, so the rule is enforced in validation/validate.py:

  • New validate_relationship_column_arity check, alongside the existing endpoint-validation rules (validate_references), emitting an [Arity] error per mismatched relationship. It skips documents that already fail schema validation.

Covered by tests in validation/tests/test_validate.py (equal-length simple and composite pass; both mismatch directions rejected; non-list to_columns skipped without crashing).

No spec or JSON Schema change is needed — this only closes the enforcement gap.

Related Issues

Addresses Problem 6 (Relationship Column Array Arity) from #374

Checklist

Validation

  • Validation rules in validation/ are updated
  • New validation cases are covered by tests

Tests

  • New functionality is covered by tests

Compliance

  • No new source files are added (existing ASF license headers unchanged)
  • No third-party dependencies are added

Documentation and examples: no change — the spec already documents the constraint and the existing examples already conform.

@christianeu-db
christianeu-db force-pushed the fix-relationship-column-arity branch 2 times, most recently from 4206786 to 457aae0 Compare September 10, 2026 02:44
The spec (core-spec/spec.md, "Relationships") requires from_columns and
to_columns to correspond positionally and to have the same number of
columns, but nothing enforced it. JSON Schema cannot express a
cross-array equality constraint, so a document with mismatched arities
(for example from_columns: [a, b] with to_columns: [c]) validated
successfully despite describing an incomplete tuple join.

Add validate_relationship_column_arity to validation/validate.py as a
new endpoint-validation rule, alongside validate_references. It emits an
[Arity] error per mismatched relationship and skips documents that
already fail schema validation. Covered by tests in
validation/tests/test_validate.py.

Addresses Problem 6 (Relationship Column Array Arity) from
apache#374

Signed-off-by: Chris Eubank <108756251+christianeu-db@users.noreply.github.com>
@christianeu-db
christianeu-db force-pushed the fix-relationship-column-arity branch from 457aae0 to 9d5fec3 Compare September 10, 2026 03:14
@christianeu-db
christianeu-db marked this pull request as ready for review September 10, 2026 03:14

@kayemkim kayemkim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself looks right to me. The rule is already in the spec text (spec.md, "Important Notes": "Both arrays must have the same number of columns", with the same note on to_columns in spec.yaml), the schema has minItems: 1 on both arrays but nothing that ties their lengths together, and I confirmed on main that a relationship with from_columns: [customer_id, region_id] and to_columns: [region] validates with exit 0. With this patch it fails with the [Arity] error, and a document where from_columns is missing altogether is left to the schema error as intended. The full validation-ci matrix (3.11 to 3.14) passes on a merge with current main, and all 11 semantic-model YAML files in the repo (examples and converter fixtures) keep their current pass/fail status.

One thing worth flagging for whoever merges: this is the third open PR adding the same check. #307 (August 5) puts it inside validate_references with a [Relationship] tag, and #170 (June 26) bundles it with a field-existence check. Both are still waiting for review. I don't have a preference on which one lands, but it would be good to pick one and close the others so the authors aren't left waiting, and so the message tag and placement stay consistent with whichever is chosen.

@christianeu-db

Copy link
Copy Markdown
Contributor Author

The change itself looks right to me. The rule is already in the spec text (spec.md, "Important Notes": "Both arrays must have the same number of columns", with the same note on to_columns in spec.yaml), the schema has minItems: 1 on both arrays but nothing that ties their lengths together, and I confirmed on main that a relationship with from_columns: [customer_id, region_id] and to_columns: [region] validates with exit 0. With this patch it fails with the [Arity] error, and a document where from_columns is missing altogether is left to the schema error as intended. The full validation-ci matrix (3.11 to 3.14) passes on a merge with current main, and all 11 semantic-model YAML files in the repo (examples and converter fixtures) keep their current pass/fail status.

One thing worth flagging for whoever merges: this is the third open PR adding the same check. #307 (August 5) puts it inside validate_references with a [Relationship] tag, and #170 (June 26) bundles it with a field-existence check. Both are still waiting for review. I don't have a preference on which one lands, but it would be good to pick one and close the others so the authors aren't left waiting, and so the message tag and placement stay consistent with whichever is chosen.

@jbonofre would you mind taking a look? I don't have a strong preference on which one merges (although #170 has been open for a while and covers other validation which might be tackled in other PRs as well). But, it would be nice to close this long-standing issue (especially, since it is showing up on recent field reports)?

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