fix(validation): resolve PK identity-raise vs check-report tension (item 8j) - #297
Merged
Merged
Conversation
…sk 1) BaseDataContract.to_checks() never emitted primary_key_unique - only compile_datacontract(TypeSpec) did, so a TypeSpec and an equivalent BaseDataContract subclass validated a resource differently inside dag.validate (spec §4.1, Fact C). Reuses the existing, already-tested primary_key_check(spec) helper; cls.to_typespec() already collapses natural_key into primary_key, so this fires for both declaration styles with no new logic.
…ontract* (item 8j, task 2) BaseDataContract.validate_datacontract/validate_datacontract_quick had no way to reach the already-shipped, already-tested Validator.validate/allow_imperfect_key escape hatch (spec §4.2, Fact A) - a declared primary_key/natural_key always raised IdentityInvalidError on a null/duplicate key before checks ran, with no way to opt into primary_key_unique reporting instead. Also corrects validate_datacontract's docstring, which claimed "never raises" - false for keyed-identity failures.
…per-resource identity isolation (item 8j, task 3) dag.validate/validate_quick had no allow_imperfect_key parameter, so every declared keyed identity always raised IdentityInvalidError deep inside validate_relation - and that raise escaped the per-resource loop uncaught, aborting validation of every resource after the failing one in the batch (spec §4.3, Fact B). RelationDAG.validate/validate_quick -> relations/dag/validation.py's validate/validate_quick/_run -> ValidationRunner.validate_dag now thread allow_imperfect_key end to end. validate_dag's per-resource loop wraps validate_relation in try/except IdentityInvalidError and isolates the failure into that resource's own ValidationResult (check_id="__identity__", status="error") - never a raised exception, never an aborted batch (spec §3.2). Isolation is categorical across every IdentityInvalidError cause, including the missing-key-fields raise that allow_imperfect_key never suppresses (spec §7 round-2 fix). Updates item 8l's test_duplicate_primary_key_raises_identity_error (tests/relations/dag/cross_backend/test_datapackage_validation_loop.py) to test_duplicate_primary_key_isolates_identity_failure, asserting the new isolated-result contract instead of a raise - the old assertion exercised exactly the DAG-tier behavior this task supersedes.
discreteds
added a commit
that referenced
this pull request
Aug 18, 2026
…-11) (#299) Root cause (PR #297 CI investigation): DuckDB gives no row-order guarantee for ASOF JOIN output absent an explicit ORDER BY. Ibis's join_asof backend (relsys_ib_ext_ma_util.py) never applied one, so test_asof_backward_strategy[ibis-duckdb] flipped strict-xfail/XPASS ~50/50 across independent runs (confirmed via 8 isolated process reruns pre/post fix). Values were always correct; only row order backing the test's positional assertions flapped. Same incident class as backlog item 101 (PR #293, 2026-08-16, flagged but not fixed then). Fix: order the join_asof result by the join key(s) before returning, matching polars/narwhals' left-order preservation for asof joins. Deterministic across 8/8 verification runs post-fix. IB-REL-11 split: ibis-duckdb portion closed resolved_in_mountainash; distinct, still-genuine ibis-sqlite raise (no ASOF JOIN translation) carved out to new IB-REL-13. Updated divergences.py, upstream-issues.yaml, known-divergences.md, and the test's xfail marker in lockstep; regenerated expression-coverage.{md,json} and _spine_expectation_census.md. AGENTS.md: removed the stale 'local Ibis fork' dependency note — CI/dev both resolve ibis-framework from PyPI, no path dependency exists. Backlog: filed mountainash-central item 108 for a distinct, unrelated latent bug found in the same function (join_asof silently drops the strategy= parameter on Ibis — forward/nearest behave as backward) and closed item 101's flagged sub-issue.
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
1 participant
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
Closes backlog item 8j: resolves the spec-level tension between §7's keyed-identity raise-by-default precondition and acceptance-16's
primary_key_uniquecheck (meant to report the same duplicate/null-key condition as a validation outcome), plus aBaseDataContract.to_checks()vscompile_datacontract()asymmetry.datacontracts/contract.py):BaseDataContract.to_checks()now emitsprimary_key_unique(reuses the existingprimary_key_check(spec)helper) — fixes the asymmetry where aTypeSpecand an equivalentBaseDataContractsubclass validated a resource differently.datacontracts/contract.py):allow_imperfect_keythreaded throughvalidate_datacontract/validate_datacontract_quick, restoring the already-shippedValidator.validateescape hatch at this entry point. Docstring corrected — it claimed "never raises", which was false for keyed-identity failures.relations/dag/dag.py,relations/dag/validation.py,validation/runner.py):allow_imperfect_keythreaded throughRelationDAG.validate/validate_quick.dag.validate/validate_quicknow never raiseIdentityInvalidErrorout of the call — a resource's invalid keyed identity is isolated into that resource's ownValidationResult(check_id="__identity__",status="error"), every other resource in the batch still validates and reports normally. Isolation is categorical across everyIdentityInvalidErrorcause (including the unconditional missing-key-fields raise).mountainash-central's 2026-07-03 validation spec status line with a pointer to this item's design amendment.Also updates item 8l's
test_duplicate_primary_key_raises_identity_error→test_duplicate_primary_key_isolates_identity_failure(tests/relations/dag/cross_backend/test_datapackage_validation_loop.py): that test exercised exactly the pre-8j DAG-tier raise behavior this item deliberately supersedes per spec §3.2.Unblocks item 8k (syntheticdata round-trip), which is gated on
primary_key_uniquereporting for real.Verification
hatch run ruff:check— clean.hatch run mypy:check— zero new errors (98 pre-existing, none on touched files).hatch run test:test-quick— 23005 passed / 0 failed (develop baseline 22991 + 14 new cases, exactly matching the plan's predicted count), 81 skipped, 3465 xfailed (pre-existing baselines unchanged).Spec:
mountainash-central/04.planning/mountainash/superpowers/specs/2026-08-17-pk-identity-vs-check-tension-design.md(rev 3)Plan:
mountainash-central/04.planning/mountainash/superpowers/plans/2026-08-17-pk-identity-vs-check-tension.md(rev 2)