Skip to content

feat(compliance): grade key and relationship portability - #699

Open
MikeNitsenko wants to merge 3 commits into
apache:mainfrom
MikeNitsenko:codex/portability-keys-relationships
Open

MikeNitsenko wants to merge 3 commits into
apache:mainfrom
MikeNitsenko:codex/portability-keys-relationships

Conversation

@MikeNitsenko

Copy link
Copy Markdown
Contributor

The portability grader can report unchanged PORTABLE findings after a converter removes keys and relationships, truncates a compound key, or changes a join's column pairing. Collect and grade these declarations alongside expressions.

Closes #693.

Changes

  • Add key and relationship findings. Preserve key roles and compound grouping; ignore column ordering within a key and alternate-key list ordering.
  • Compare complete join-column pairs and endpoint datasets. Reordering both sides together is PORTABLE; changing the pairing is REWRITTEN. Retain unmatched columns so zip cannot hide truncation.
  • Recover unambiguous key-role changes, split compound keys, changed alternate keys, and relationship renames as REWRITTEN. Ambiguous matches remain drops and additions.
  • Require a diagnostic naming the original key, returned column, and surrogate before classifying a single-column replacement as PLACEHOLDER. Truncation alone is REWRITTEN.
  • Add an independent K0 fixture with the four-part customer-sales-area key, an alternate key, a fact primary key, and their compound relationship. Register the structural elements in the manifest and wire tests and fixture validation into CI.

Expression classification is unchanged. This compares declarations, not warehouse data. No runtime dependencies or schema changes.

Integration with #631

Draft while #631 is pending, following the discussion on #693. Targets current main; K0 stays separate from M0–M6.

The combined grader and fixtures passed 54 tests in a temporary checkout after resolving the two small _collect overlaps and expanding #631's manifest kind allowlist to key and relationship. These integration edits will be applied on rebase after #631 lands. Its pending implementation is not included here.

Validation

  • 49 passed on Python 3.11 with SQLGlot; 49 passed on Python 3.14 without SQLGlot.
  • K0 passed validation/validate.py and the CLI identity check returned all 16 elements PORTABLE, including four structural declarations.
  • Ruff E/F checks, Apache RAT, and git diff --check passed.
  • Regressions cover drops, truncation, role changes, split and ambiguous keys, join pairing and arity, scoped diagnostics, cross-dataset isolation, CLI JSON, and fixture/manifest agreement.

AI assistance

Prepared with AI assistance (Codex).

@github-actions github-actions Bot added the infra label Nov 5, 2026
@MikeNitsenko
MikeNitsenko marked this pull request as ready for review November 9, 2026 19:57
Copilot AI balanced review requested due to automatic review settings November 9, 2026 19:57
@MikeNitsenko

Copy link
Copy Markdown
Contributor Author

Please merge #631 first. Once it lands, I’ll rebase this PR and rerun the checks before #699 is merged.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open issues
What changed in this PR

Extends the portability grader to evaluate structural declarations (keys and relationships) alongside expressions, adding a dedicated K0 fixture, tests, and CI validation to prevent structural regressions in converter round-trips.

Changes:

  • Collects and grades key and relationship declarations (including key roles, compound grouping, and join-column pairings).
  • Adds K0 structural fixture and comprehensive structural grading tests (including diagnostics/scoping and CLI JSON output).
  • Wires portability grader tests + K0 fixture validation into CI and updates documentation/manifest wording.
File Description
compliance/​portability/​manifest.yaml Updates outcome descriptions and registers the new structural rung K0 and its elements.
compliance/​portability/​k0/​k0_compound_keys.ossie.yaml Adds standalone K0 fixture with compound keys and a compound relationship join.
compliance/​portability/​grader/​test_structure.py Introduces structural grading tests covering keys/relationships, ambiguity rules, diagnostics, and fixture/manifest alignment.
compliance/​portability/​grader/​grade.py Implements structural collection, structural repair pairing, and surrogate-diagnostic handling for keys.
compliance/​portability/​grader/​README.md Documents structural grading rules (naming/comparison/diagnostic requirements) and how to run tests.
compliance/​portability/​README.md Links the K0 rung and structural grading rules into the portability ladder docs.
.github/​workflows/​validation-ci.yml Runs portability grader tests and validates the K0 fixture in CI; expands workflow path filters to portability files.

Review effort: Lite

Comment thread compliance/portability/grader/grade.py Resolved
Comment thread compliance/portability/grader/grade.py Resolved
Comment thread compliance/portability/grader/grade.py Resolved
@MikeNitsenko

Copy link
Copy Markdown
Contributor Author

@jbonofre could you please take a look when you get a chance? thanks!

@christianeu-db christianeu-db 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.

Thanks @MikeNitsenko! Left two comments; the fixture version needs fixing before merge, and I'll take another look once this is rebased onto #631.

# under the License.

# K0: preserve a four-part ERP customer-sales-area identity and its join.
version: "0.2.0.dev0"

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.

Since #691, main expects 0.3.0.dev0, so the new Validate compound-key portability fixture step fails on the merge with main with [Schema] version: '0.3.0.dev0' was expected.

Comment thread compliance/portability/grader/grade.py Outdated

def _mentions(name, text):
"""Match a scoped structural name without matching another dataset's suffix."""
return re.search(r"(?<![\w.])" + re.escape(name) + r"(?![\w.])", text) is not None

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.

Not a blocker: the lookahead also rejects a sentence-ending period, so Dropped relationship sales_to_areas. grades DROPPED_SILENT, and areas.primary_key replaced by surrogate area_id. grades REWRITTEN instead of PLACEHOLDER. Rejecting a . only when a word character follows keeps the scoping your tests check:

    return re.search(r"(?<![\w.])" + re.escape(name) + r"(?!\.?\w)", text) is not None

@christianeu-db christianeu-db 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.

Thanks @MikeNitsenko, both points are addressed. I'll take another look and approve once #631 lands and this is rebased onto it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compliance/portability: grade keys and relationships, not only expressions

3 participants