Generate checks for declared primary keys - #1404
Merged
Merged
Conversation
create_checks handled `required` and `unique` but ignored `primaryKey`, so a contract could declare a primary key and `datacontract test` would report the data valid while the column contained both duplicates and nulls. A primary key now produces a not-null check per key column plus a uniqueness check. A composite key is unique as a tuple rather than column by column, so its members are checked together in a single model-level check using the existing multi-column DUPLICATE_COUNT support, ordered by primaryKeyPosition. Columns that also declare `required` or `unique` keep only the existing check, so declaring all three does not emit near-duplicate rows.
Collaborator
|
Thank you for the PR, LGTM! I think the Changelog entry is fine - you're right that this change will make some currently-passing contracts fail, but only in cases where this should be the expected behavior anyways. |
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.
Closes #1220.
create_checks.pyhandledrequiredanduniquebut had noprimaryKeybranch, so a declared primary key generated nothing. On a CSV whoseorder_idis both duplicated and null:Composite keys
This is the part that makes it more than a copy of the
uniquebranch. Emitting_duplicate_count_checkper key column would require each member of a composite key to be individually unique, which is wrong and would fail nearly every composite-PK contract. So a composite key gets one model-level check over the tuple, using the multi-columnDUPLICATE_COUNTsupport thatCheckSpec.columnsandibis_check_executealready have, ordered byprimaryKeyPosition:De-duplication
Contracts routinely declare
required,uniqueandprimaryKeyon the same column —tests/fixtures/schema-evolution/odcs-datacontract-cities-version-1.yamldoes exactly that oncity_id. The primary-key checks are skipped whererequiredoruniquehas already emitted an equivalent one, so those contracts see no new rows and no near-duplicate output. Keys stay distinct (field_primary_key_required/field_primary_key_unique/<model>__primary_key_unique) since_collect_failed_samplesresolves checks by key.Two things for you to rule on
field_primary_key_required/field_primary_key_uniquefor single columns andprimary_key_uniquefor the composite, mirroringfield_required/field_unique. Easy to rename if you'd prefer something else.Verification
Full suite is unchanged: 30 failed / 948 passed / 41 errors both before and after (the failures and collection errors are optional backends — Trino, SQL Server, S3 — that need extras I don't have locally).
tests/test_test_schema_evolution.py,test_create_checks_physical_type.py,test_create_checks_nested_type.py,test_data_contract_checks.py: 97 passed.New
tests/test_create_checks_primary_key.pycovers single keys, composite tuples,physicalNamemapping, position ordering, and the de-duplication guard. Reverting onlycreate_checks.pywhile keeping the tests fails 3 of the 5; the other 2 are over-generation guards and pass either way by design.End-to-end runs above were real
datacontract testinvocations against local CSV through duckdb, not unit fixtures.Tests pass (
uv run pytest)Code formatted (
uv run ruff check --fix && uv run ruff format)Docs updated (if relevant) — no user-facing doc lists generated checks
CHANGELOG.md entry added