DEV-1688: cleaner handling of join arity (cardinality) - #279
Merged
ZmeiGorynych merged 24 commits intoAug 17, 2026
Conversation
…tion incident Working-tree state recovered from the failed disk and committed so it cannot be lost again. This is a raw recovery snapshot and may contain minor corruption mixed with genuine WIP; review before building on it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolves the CLAUDE.md conflict by taking main's condensed form. main restructured CLAUDE.md, moving per-feature detail into docs/ and the new append-only DECISIONS.md; all three DEV-1688 bullets lived in the region main deleted, so they are re-homed into the new structure in the following commit rather than resurrected here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bug fix (found by running detect-cardinality end-to-end against the demo DuckDB): _unique_contradictions treated ANY primary_key column as a solo uniqueness claim, so every member of a COMPOSITE primary key was reported as "declared unique but the data has duplicates". jaffle_shop's supplies table (PK (id, sku), joined on sku alone) tripped it. Being half of (id, sku) claims nothing about sku on its own, which is exactly the subset rule is_key_set_unique already implements: (id, sku) is not a subset of (sku). New _declares_solo_unique helper counts a PK column as a solo claim only when it is the whole primary key. Two regression tests: sole-PK still reports, composite-PK member no longer does. MCP docstrings (the gap the issue called out): edit_model now documents column `unique` and join `cardinality` (plus the composite-key grouping); create_model documents `unique`. These params are list[dict[str, Any]], so the docstring is the only channel that reaches an agent -- MCP exposes no per-field schema for them. Giving them typed schemas is a worthwhile follow-up but a cross-cutting refactor beyond this issue. Docs re-homed into main's new structure: - DECISIONS.md: DEV-1688 entry (LEFT-vs-cardinality orthogonality, the asymmetric-evidence model, the subset rule, composite-FK grouping) - docs/reference/cli.md + docs/interfaces/cli.md: `slayer joins detect-cardinality` (main moved the CLI list off CLAUDE.md, leaving the command undocumented post-merge) - docs/concepts/ingestion.md: Column.unique, composite-FK grouping and structural cardinality inference; corrected the "existing columns and joins are never mutated" claim, which DEV-1688's additive gap-fill of cardinality/unique made untrue - docs/dbt/dbt_import.md: the example join output was stale -- the dbt converter now emits cardinality (foreign->primary many_to_one, peer one_to_one, inverted on the mirrored reverse edge) - docs/osi/osi_import.md: relationship mapping records many_to_one Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional ChangesJoin cardinality metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SlayerQueryEngine
participant SQLDatasource
participant ModelStorage
CLI->>SlayerQueryEngine: detect_join_cardinality(filters, persist)
SlayerQueryEngine->>SQLDatasource: query join-key statistics
SQLDatasource-->>SlayerQueryEngine: return uniqueness statistics
SlayerQueryEngine->>ModelStorage: persist detected cardinality
SlayerQueryEngine-->>CLI: return detection report
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codex (valid): slayer/facade/translator.py::_dynamic_join_cardinality had the same composite-PK bug already fixed in _unique_contradictions -- every member of a composite primary key carries primary_key=True, so a Metabase dynamic join onto ONE member wrongly claimed many_to_one. The join constrains only that column, so the composite key's uniqueness does not carry (the same subset rule is_key_set_unique applies: (id, sku) is not a subset of (sku)). Extracted the predicate into slayer/engine/cardinality.py::declares_solo_unique so there is ONE implementation, living next to is_key_set_unique whose rule it mirrors. Both call sites now use it. slayer/engine/cardinality.py is a pydantic-only leaf (engine/__init__.py is empty), so the facade import adds no cycle and no heavy dependency. Existing facade tests only covered a single-column PK; added composite-PK and non-PK-unique cases plus four unit tests for the helper. Codex (invalid, no change): join_sync._mirror_inner_joins "overwrites a user-set cardinality" on the reverse edge. That edge is a DERIVED mirror, not curated metadata -- join_sync already unconditionally reconciles its join_pairs (pre-existing behaviour), and DEV-1688 explicitly specifies reconciling the reverse edge "even when only the cardinality changed". The never-overwrite invariant applies to the forward edge on re-ingest, which is unchanged. Sonar python:S3776 (both valid, both new code in this PR): - ingestion._get_unique_key_sets, cognitive complexity 19 -> split its three defensive try/except scans into _pk_key_sets / _unique_constraint_key_sets / _unique_index_key_sets over a shared _safe_introspect helper. _get_single_column_unique_names duplicated two of those scans verbatim and now reuses them, so the "collect key-sets" logic exists once. - query_engine.detect_join_cardinality, cognitive complexity 24 -> extracted the per-datasource pass into _detect_datasource_joins and the scope/lookup resolution into _resolve_detection_scope. The scope helper keeps the by_name lookup spanning the whole datasource even when --model narrows the scope, so join targets still resolve. Behaviour unchanged: re-verified end-to-end against the demo DuckDB (fresh ingest + detect-cardinality) -- 6 joins, all `confirms`, no false positives. 7249 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
slayer/engine/ingestion.py (4)
418-429: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the swallowed reflection failure.
_safe_introspectcatches every exception and returns[]with no signal. The docstring explains the intent for backends that expose no constraint metadata. A permissions error or a driver defect produces the same silent[].The consequence is not a crash. It is silently wrong metadata: a missing unique key-set makes
infer_structural_cardinalityreturnNone, so a realmany_to_onejoin is recorded as undetermined and the user has no way to see why.This module already logs degrades elsewhere. Line 942 logs a warning when the FK graph has cycles, and the SQLite probe logs a warning on probe errors. Add a debug-level log with the exception so the cause is recoverable from logs.
♻️ Proposed change
-def _safe_introspect(fn) -> list: +def _safe_introspect(fn: Callable[[], Iterable]) -> list: """Run a best-effort introspection call, yielding ``[]`` on failure. Constraint/index reflection is unsupported or partial on several backends (ClickHouse and BigQuery expose no FK metadata; duckdb-engine doesn't reflect indices), so a raising call must degrade to "no evidence" rather than abort ingestion. """ try: return list(fn()) - except Exception: + except Exception as exc: # noqa: BLE001 — degrade to "no evidence" + logger.debug( + "Constraint/index reflection unavailable (%s); " + "treating as no uniqueness evidence.", exc, + ) return []Add
CallableandIterableto the existingtyping/collections.abcimport at the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/ingestion.py` around lines 418 - 429, Update _safe_introspect to accept the appropriate callable/iterable typing imports and log swallowed reflection exceptions at debug level, including the exception details, before returning []. Preserve the existing best-effort fallback and avoid changing successful introspection behavior.
487-491: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse keyword arguments when calling the new helpers.
Lines 488-490 pass four positional arguments to
_pk_key_setsand three to the two key-set helpers. The coding guidelines require keyword arguments for functions with more than one parameter.inspector,table_name,schema, andsa_engineare all easy to transpose because three of them are strings or optional.The same pattern appears at line 541, line 555, lines 901-902, lines 959-961, and line 1211.
♻️ Proposed change
return ( - _pk_key_sets(inspector, table_name, schema, sa_engine) - + _unique_constraint_key_sets(inspector, table_name, schema) - + _unique_index_key_sets(inspector, table_name, schema) + _pk_key_sets( + inspector=inspector, + table_name=table_name, + schema=schema, + sa_engine=sa_engine, + ) + + _unique_constraint_key_sets( + inspector=inspector, table_name=table_name, schema=schema, + ) + + _unique_index_key_sets( + inspector=inspector, table_name=table_name, schema=schema, + ) )As per coding guidelines: "Use keyword arguments for functions with more than one parameter, and keep imports at the top of files."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/ingestion.py` around lines 487 - 491, Update every affected call to _pk_key_sets, _unique_constraint_key_sets, and _unique_index_key_sets—including the additional call sites identified in the comment—to pass all arguments by their parameter names rather than positionally; preserve the existing values and behavior.Source: Coding guidelines
1106-1107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
_join_siginstead of keeping a third copy of the signature expression.The new
_join_sigat lines 1106-1107 builds(target_model, tuple(sorted(...))). The identical expression already exists at line 1044 inside_existing_join_signatures, and line 1138 inlines it a third time in the same function that now calls_join_sig.Join identity drives the additive merge and the duplicate-target error. If one copy drifts from the others, the merge silently matches the wrong join or raises a spurious conflict. Route all three sites through
_join_sig.♻️ Proposed change
new_join_targets: list[str] = [] for j in fresh.joins: - sig = (j.target_model, tuple(sorted((p[0], p[1]) for p in j.join_pairs))) + sig = _join_sig(j) if sig in existing_join_sigs:Apply the same substitution inside
_existing_join_signatures:def _existing_join_signatures(model: SlayerModel) -> set[tuple]: """Return the set of (target_model, sorted join_pair tuples) signatures for joins already on ``model``. Used to detect new joins. """ return {_join_sig(j) for j in model.joins}Move
_join_sigabove_existing_join_signaturesso the definition precedes both uses.Also applies to: 1136-1138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/ingestion.py` around lines 1106 - 1107, Move _join_sig above _existing_join_signatures, then replace the duplicated signature expressions in _existing_join_signatures and the join-merge logic around the third occurrence with calls to _join_sig. Ensure all join identity checks use this single helper while preserving the existing additive merge and duplicate-target conflict behavior.
553-563: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
target_uniquesper referenced table.Line 555 calls
_get_unique_key_setsinside the group loop. That helper performs up to three reflection round-trips:get_pk_constraint,get_unique_constraints, andget_indexes.When a table has several FKs to the same target, the same target is reflected repeatedly.
tests/test_ingestion.pylines 208-228 pin exactly that shape withbuyer_idandseller_idboth referencingusers. On a wide schema the cost scales with the FK count instead of the distinct-target count.Cache per
ref_tableinside the function.♻️ Proposed change
joins = [] seen_signatures: set[tuple] = set() + target_uniques_cache: dict[str, list[list[str]]] = {} for ref_table, pairs in groups:source_cols = [s for s, _ in pairs] target_cols = [t for _, t in pairs] - target_uniques = _get_unique_key_sets(inspector, ref_table, schema, sa_engine) + if ref_table not in target_uniques_cache: + target_uniques_cache[ref_table] = _get_unique_key_sets( + inspector=inspector, + table_name=ref_table, + schema=schema, + sa_engine=sa_engine, + ) + target_uniques = target_uniques_cache[ref_table]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/ingestion.py` around lines 553 - 563, Cache the result of _get_unique_key_sets in the enclosing ingestion function by ref_table, so each referenced table is reflected only once even when multiple FK groups target it. Reuse the cached target_uniques value in the cardinality calculation while preserving the existing source-column and target-column logic.slayer/engine/cardinality.py (1)
44-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations to
declares_solo_unique.Every other function in this module is fully annotated.
columnsandcolumnare bare. The docstring shows both areColumnvalues. Use aTYPE_CHECKINGimport so the annotation adds no import cycle risk withslayer.core.models.♻️ Proposed change
from __future__ import annotations +from typing import TYPE_CHECKING + from pydantic import BaseModel, Field from slayer.core.enums import JoinCardinality, StrEnum + +if TYPE_CHECKING: + from slayer.core.models import Column-def declares_solo_unique(*, columns, column) -> bool: +def declares_solo_unique(*, columns: list["Column"], column: "Column") -> bool:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/cardinality.py` around lines 44 - 56, Annotate the columns and column parameters of declares_solo_unique with the appropriate Column types, using the module’s TYPE_CHECKING pattern to import Column from slayer.core.models without creating a runtime import cycle. Preserve the existing return annotation and uniqueness logic.slayer/osi/converter.py (1)
528-529: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider verifying target uniqueness before asserting
MANY_TO_ONE.This path stamps
MANY_TO_ONEon every OSI relationship without checking the target key.DECISIONS.mdline 68 records the opposite policy for inference: cardinality stays unset unless the target key is verified unique. The mismatch has a concrete downstream effect._claims_target_unique(MANY_TO_ONE)returnsTrueinslayer/engine/cardinality.py, socompute_verdictreportsCONTRADICTS_HARDwhen profiling later observes duplicates on the target side — for a value SLayer asserted itself with no evidence.The converter already has the introspected models in
self._models, so it can reuseis_key_set_uniqueover the target model'sprimary_key/uniquecolumns and leavecardinality=Nonewhen the target key is not verified unique.Note that
docs/osi/osi_import.mdline 26 documents the current direction-implied behavior, so update that row if you change the rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/osi/converter.py` around lines 528 - 529, Update the OSI relationship conversion around cardinality=JoinCardinality.MANY_TO_ONE to verify the target model’s primary_key/unique columns via self._models and is_key_set_unique before asserting MANY_TO_ONE; leave cardinality unset when uniqueness is not verified. Update the corresponding OSI behavior row in docs/osi/osi_import.md to document the new rule.tests/test_join_cardinality_producers.py (1)
100-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting that
join_typestays LEFT.
DECISIONS.mdline 68 records "FK-derived joins stay LEFT, never auto-INNER" as an invariant of this change. These tests assert onlycardinality. Add ajoin_typeassertion so a future change that couples cardinality to join type fails here.♻️ Proposed addition
def test_forward_many_to_one(self) -> None: result = DbtToSlayerConverter( project=_foreign_primary_project(), data_source="test_db" ).convert() orders = next(m for m in result.models if m.name == "orders") fwd = next(j for j in orders.joins if j.target_model == "customers") assert fwd.cardinality is JoinCardinality.MANY_TO_ONE + # Cardinality is orthogonal to join type — the join stays LEFT. + assert fwd.join_type is JoinType.LEFTImport
JoinTypefromslayer.core.enumsalongside the existing enum imports.Based on learnings,
tests/files are exempt from the keyword-argument style check, so the positional usage here is fine.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_join_cardinality_producers.py` around lines 100 - 143, Extend the join assertions in TestDbtConverterMirror to verify every FK-derived mirror join retains JoinType.LEFT, importing JoinType from slayer.core.enums alongside the existing enums. Add the checks to test_forward_many_to_one, test_reverse_mirror_inverts_to_one_to_many, and test_peer_mirror_stays_one_to_one without changing their cardinality assertions.Source: Learnings
tests/test_ingestion_cardinality.py (1)
122-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse synchronous test functions when no operation is awaited.
Both test bodies only call synchronous helpers. Define them with
defto avoid unnecessary async-test handling and apython:S7503warning.Proposed fix
- async def test_generate_joins_groups_composite_fk(self, workspace: Path) -> None: + def test_generate_joins_groups_composite_fk(self, workspace: Path) -> None: ... - async def test_build_fk_graph_one_edge_per_group(self, workspace: Path) -> None: + def test_build_fk_graph_one_edge_per_group(self, workspace: Path) -> None: ...Based on learnings: tests that do not await must use
def test_*.Also applies to: 148-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_ingestion_cardinality.py` around lines 122 - 146, Change the affected tests, including test_generate_joins_groups_composite_fk and the test covering lines 148–167, from async def to def because they do not await any operations; preserve their existing synchronous test logic and assertions.Source: Learnings
slayer/engine/query_engine.py (1)
2511-2522: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache side statistics per
(table, key columns).The loop profiles both sides of every join. A hub table is full-scanned once per incoming join, so
customersis scanned twice fororders → customersanduser_profiles → customers. Each scan is two full table scans, one of them aDISTINCT.Memoize
_side_statsresults in a dict keyed by(table, tuple(key_cols))for the duration of one datasource pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/query_engine.py` around lines 2511 - 2522, The _detect_one_join method is recalculating side statistics for the same table and key columns across multiple join calls, causing duplicate full table scans. Create a memoization cache keyed by (table, tuple(key_cols)) before the loop over scope begins, then pass this cache to _detect_one_join so it can reuse _side_stats results for identical table-key combinations within the same datasource pass. Ensure the cache persists across all iterations of the inner join loop so that repeated references to the same hub table are only computed once.tests/test_detect_join_cardinality.py (1)
143-192: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a table with no non-null key rows.
The seed data always has rows on both sides. Add a table whose join key is empty (or all
NULL) and assert the intended behaviour. This pins the zero-row semantics flagged onslayer/engine/query_engine.pyLine 2592-2598, whererow_count == distinct_count == 0currently yieldsobserved_unique=True.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_detect_join_cardinality.py` around lines 143 - 192, Add a new test method in the TestClassification class following the existing pattern that exercises a join where one side has all NULL key values (resulting in zero non-null rows). Use the _build_engine and engine.detect_join_cardinality API with the _find helper to locate the join result, then assert the observed_unique property and row_count/distinct_count values when both are zero to pin the current behavior described in query_engine.py around lines 2592-2598.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/engine/ingestion.py`:
- Around line 900-903: Update _safe_get_pk_constraint so every database branch
validates the inspector result and returns {"constrained_columns": []} whenever
it is None or not a mapping, while preserving valid mappings. Then remove the
now-redundant defensive guard in _pk_key_sets and rely on the normalized return
contract for all callers, including the unique-column flow near
_get_single_column_unique_names.
- Around line 464-477: Update _unique_index_key_sets to preserve only complete
reflected index key-sets: stop filtering out falsy column entries, and require
all entries to be present before appending the set, matching
_unique_constraint_key_sets. Keep valid indexes unchanged while discarding any
set containing a missing expression placeholder.
In `@slayer/engine/query_engine.py`:
- Around line 2592-2598: Add a check after computing row_count and
distinct_count to detect when row_count equals zero. When no rows are present,
the SideStats object should signal no evidence of cardinality rather than
setting observed_unique to true. Return an appropriate "no evidence" result
(such as SKIPPED_UNSUPPORTED if that value exists, or add a dedicated marker
field to SideStats) when row_count is zero. Then update the caller that
processes SideStats results to exclude zero-row findings from persist_entries so
that empty tables do not incorrectly establish ONE_TO_ONE join constraints.
- Around line 2575-2598: Update _side_stats to apply the configured
SessionPolicy to both profiling SQL queries through _apply_policy, and run the
ClickHouse version preflight before executing them. Preserve the existing
row_count, distinct_count, and observed_unique calculations while ensuring both
queries use the policy-filtered SQL.
- Around line 2578-2591: Replace the f-string SQL concatenation in both
client.execute() calls with sqlglot AST construction to comply with coding
guidelines. Build the COUNT query using exp.select() with exp.func("COUNT") and
the subquery using exp.select() with .distinct(), then construct the WHERE
clause using exp.and_() to combine the IS NOT NULL conditions on the key
columns. Render both AST objects to SQL only at the final client.execute() calls
using .sql(dialect=sqlglot_name, identify=True).
In `@slayer/mcp/server.py`:
- Around line 1002-1004: Update the documentation comment describing cardinality
metadata to remove the incorrect statement that joins stay LEFT. Instead,
clarify that cardinality is descriptive metadata that does not change join_type
or query results, ensuring the description accurately reflects that
ModelJoin.join_type supports both LEFT and INNER joins, and that edit_model
preserves or allows specification of either join type.
---
Nitpick comments:
In `@slayer/engine/cardinality.py`:
- Around line 44-56: Annotate the columns and column parameters of
declares_solo_unique with the appropriate Column types, using the module’s
TYPE_CHECKING pattern to import Column from slayer.core.models without creating
a runtime import cycle. Preserve the existing return annotation and uniqueness
logic.
In `@slayer/engine/ingestion.py`:
- Around line 418-429: Update _safe_introspect to accept the appropriate
callable/iterable typing imports and log swallowed reflection exceptions at
debug level, including the exception details, before returning []. Preserve the
existing best-effort fallback and avoid changing successful introspection
behavior.
- Around line 487-491: Update every affected call to _pk_key_sets,
_unique_constraint_key_sets, and _unique_index_key_sets—including the additional
call sites identified in the comment—to pass all arguments by their parameter
names rather than positionally; preserve the existing values and behavior.
- Around line 1106-1107: Move _join_sig above _existing_join_signatures, then
replace the duplicated signature expressions in _existing_join_signatures and
the join-merge logic around the third occurrence with calls to _join_sig. Ensure
all join identity checks use this single helper while preserving the existing
additive merge and duplicate-target conflict behavior.
- Around line 553-563: Cache the result of _get_unique_key_sets in the enclosing
ingestion function by ref_table, so each referenced table is reflected only once
even when multiple FK groups target it. Reuse the cached target_uniques value in
the cardinality calculation while preserving the existing source-column and
target-column logic.
In `@slayer/engine/query_engine.py`:
- Around line 2511-2522: The _detect_one_join method is recalculating side
statistics for the same table and key columns across multiple join calls,
causing duplicate full table scans. Create a memoization cache keyed by (table,
tuple(key_cols)) before the loop over scope begins, then pass this cache to
_detect_one_join so it can reuse _side_stats results for identical table-key
combinations within the same datasource pass. Ensure the cache persists across
all iterations of the inner join loop so that repeated references to the same
hub table are only computed once.
In `@slayer/osi/converter.py`:
- Around line 528-529: Update the OSI relationship conversion around
cardinality=JoinCardinality.MANY_TO_ONE to verify the target model’s
primary_key/unique columns via self._models and is_key_set_unique before
asserting MANY_TO_ONE; leave cardinality unset when uniqueness is not verified.
Update the corresponding OSI behavior row in docs/osi/osi_import.md to document
the new rule.
In `@tests/test_detect_join_cardinality.py`:
- Around line 143-192: Add a new test method in the TestClassification class
following the existing pattern that exercises a join where one side has all NULL
key values (resulting in zero non-null rows). Use the _build_engine and
engine.detect_join_cardinality API with the _find helper to locate the join
result, then assert the observed_unique property and row_count/distinct_count
values when both are zero to pin the current behavior described in
query_engine.py around lines 2592-2598.
In `@tests/test_ingestion_cardinality.py`:
- Around line 122-146: Change the affected tests, including
test_generate_joins_groups_composite_fk and the test covering lines 148–167,
from async def to def because they do not await any operations; preserve their
existing synchronous test logic and assertions.
In `@tests/test_join_cardinality_producers.py`:
- Around line 100-143: Extend the join assertions in TestDbtConverterMirror to
verify every FK-derived mirror join retains JoinType.LEFT, importing JoinType
from slayer.core.enums alongside the existing enums. Add the checks to
test_forward_many_to_one, test_reverse_mirror_inverts_to_one_to_many, and
test_peer_mirror_stays_one_to_one without changing their cardinality assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f553da1a-f1ba-47ce-9062-f74ca15487e3
📒 Files selected for processing (33)
.claude/skills/slayer-models.mdDECISIONS.mddocs/concepts/ingestion.mddocs/concepts/models.mddocs/dbt/dbt_import.mddocs/interfaces/cli.mddocs/osi/osi_import.mddocs/reference/cli.mdslayer/cli.pyslayer/core/enums.pyslayer/core/models.pyslayer/dbt/converter.pyslayer/dbt/entities.pyslayer/engine/cardinality.pyslayer/engine/ingestion.pyslayer/engine/query_engine.pyslayer/facade/catalog.pyslayer/facade/translator.pyslayer/inspect/model_render.pyslayer/mcp/server.pyslayer/osi/converter.pyslayer/search/graph.pyslayer/storage/join_sync.pytests/test_cardinality_helpers.pytests/test_cli_detect_cardinality.pytests/test_detect_join_cardinality.pytests/test_ingestion.pytests/test_ingestion_cardinality.pytests/test_ingestion_cardinality_reingest.pytests/test_join_cardinality_mirror.pytests/test_join_cardinality_models.pytests/test_join_cardinality_producers.pytests/test_join_cardinality_surfacing.py
Codex re-review (valid): _unique_index_key_sets compacted falsy members out of a unique index's column_names instead of rejecting the key-set. SQLAlchemy reports expression-index members as None (the text lives in `expressions`), so a unique index on (email, lower(name)) arrives as ['email', None] and was compacted to ['email'] -- a bogus single-column uniqueness claim that would wrongly stamp Column.unique=True on email and infer a one-side cardinality for any join on it. This bug predates the extractor refactor: both the original _get_unique_key_sets and _get_single_column_unique_names used the same `[c for c in cols if c]` filter, so the refactor preserved it faithfully -- splitting the scans is what made the inconsistency with _unique_constraint_key_sets (which already used `cols and all(cols)`) visible. Both now apply the same rule: any falsy member rejects the whole key-set. Test uses a fake inspector to cover the mixed named/expression index, a genuine single-column unique index, and a non-unique index. Also moved the slayer.engine.cardinality import below the slayer.core block in facade/translator.py so it no longer splits the two slayer.core imports. 7250 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two valid findings from the Codex re-review. 1. Partial unique indexes were treated as whole-table uniqueness. `CREATE UNIQUE INDEX ... ON t (email) WHERE deleted_at IS NULL` -- the common soft-delete pattern -- only guarantees uniqueness among rows matching its predicate, so it is not evidence about the table. Accepting it wrongly stamped Column.unique=True on email and inferred a one-side cardinality for any join on it. New _is_partial_index reads the predicate SQLAlchemy surfaces per dialect under dialect_options as `<dialect>_where` (postgresql_where, sqlite_where, ...); an empty/None predicate is not a predicate and still counts as a full unique index. 2. Cross-schema foreign keys silently bound to the wrong table. Models are keyed by bare table name within a datasource, so an FK into another schema has no model to bind to. `referred_table not in table_set` already skipped most of these, but when a same-named table happened to exist in the ingested schema the FK bound to it -- producing a join to the wrong table and, since DEV-1688, inferring that join's cardinality from the wrong table's key constraints. New _is_cross_schema_fk skips those, applied to BOTH _get_fk_constraint_groups (joins) and _get_fk_relationships (the FK graph) so the two stay consistent. It only skips when both schemas are known AND differ: referred_schema is commonly None for same-schema FKs and always None on schemaless backends like SQLite, and those must keep working. The schema-ignoring in (2) predates this PR, but DEV-1688 compounded it by deriving cardinality from the mis-resolved target, and the safe fix is small -- so it is fixed here rather than left. Full cross-schema join support (which needs a model-naming scheme with a schema component) remains out of scope. 7256 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeRabbit (valid): the helper is annotated `-> dict` and four of its five
callers do an unguarded `result.get("constrained_columns")`, but the SQLite
branch returned `inspector.get_pk_constraint(...)` straight through. The
non-SQLite branch was already safe by accident -- its own `result.get(...)`
raises inside the `try` on a non-mapping and falls through to the
INFORMATION_SCHEMA fallback -- so SQLite was the one unnormalized path.
The inspector is a third-party boundary (dialects outside SQLAlchemy's own
tree can return None or a non-mapping) and this helper is the single place
that should normalize it, so it now does on every path. That let the
defensive `isinstance(pk, dict)` guard in _pk_key_sets drop: the guard now
sits only on the bare-inspector path (sa_engine=None), which genuinely
bypasses the helper. Previously the codebase was inconsistent -- _pk_key_sets
guarded while the two new call sites in the unique-column flow did not.
Behaviour is unchanged for every real backend; this closes the contract so
the annotation is true and callers can rely on it.
7261 passed, 0 failures; ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
slayer/engine/ingestion.py (1)
342-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse keyword arguments for multi-parameter calls.
Use keyword arguments for
_is_cross_schema_fk,_safe_get_pk_constraint, andInspector.get_pk_constraint.Proposed fix
- if _is_cross_schema_fk(fk, schema): + if _is_cross_schema_fk(fk=fk, schema=schema): - pk = _safe_get_pk_constraint(inspector, sa_engine, table_name, schema) + pk = _safe_get_pk_constraint( + inspector=inspector, + sa_engine=sa_engine, + table_name=table_name, + schema=schema, + ) - pk = inspector.get_pk_constraint(table_name, schema=schema) + pk = inspector.get_pk_constraint( + table_name=table_name, + schema=schema, + )As per coding guidelines, use keyword arguments for functions with more than one parameter.
Also applies to: 435-436, 470-472
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/ingestion.py` around lines 342 - 343, Update the multi-parameter calls to _is_cross_schema_fk, _safe_get_pk_constraint, and Inspector.get_pk_constraint to pass arguments by their parameter names rather than positionally, including the additional occurrences noted in the ingestion flow; preserve the existing argument values and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/engine/ingestion.py`:
- Around line 318-323: Update the foreign-key schema validation around
referred_schema so it returns true whenever an explicit referred_schema does not
exactly equal the ingested schema, including when schema is None; preserve
rejection of unresolved cross-schema references. Add a regression test covering
schema=None with referred_schema="archive" and verify the foreign key is
skipped.
---
Nitpick comments:
In `@slayer/engine/ingestion.py`:
- Around line 342-343: Update the multi-parameter calls to _is_cross_schema_fk,
_safe_get_pk_constraint, and Inspector.get_pk_constraint to pass arguments by
their parameter names rather than positionally, including the additional
occurrences noted in the ingestion flow; preserve the existing argument values
and behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94f2217b-e03f-4bda-9115-4e06729a354a
📒 Files selected for processing (3)
slayer/engine/ingestion.pyslayer/facade/translator.pytests/test_ingestion_cardinality.py
🚧 Files skipped from review as they are similar to previous changes (1)
- slayer/facade/translator.py
Both from a Codex re-review of the previous round's fixes. 1. _is_cross_schema_fk did nothing when ingesting the DEFAULT schema. Ingesting the default schema passes schema=None, and the guard required BOTH schemas to be non-None -- so a reflected referred_schema="archive" slipped through unskipped, which is precisely the case the guard was added for. It now falls back to the connection's default_schema_name (read via getattr at both call sites) as the comparison basis: cross-schema is `referred_schema != (schema or default_schema)`. When neither is known the FK is kept, since there is no basis to reject it. A referred_schema of None is still never cross-schema (same-schema FKs and schemaless backends like SQLite report None). 2. _is_partial_index could RAISE instead of classifying. It tested the reflected predicate for truthiness, but a dialect may return it as a SQLAlchemy expression object, and ColumnElement.__bool__ raises TypeError. This runs outside _safe_introspect, so that would abort the entire ingest rather than skip one index. The predicate is now never evaluated for truthiness: strings are checked for non-emptiness, and any other non-None value counts as a predicate by its presence alone. Tests cover the default-schema fallback (skip / keep / both-unknown / explicit-schema-wins) and a predicate object whose __bool__ raises, asserting both that _is_partial_index classifies it as partial and that _unique_index_key_sets skips that index without propagating. 7268 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeRabbit (security, valid): the profiling scans in _side_stats executed raw SQL straight through the client, bypassing _apply_policy entirely. With a SessionPolicy configured (DEV-1578 row-level security), detect_join_cardinality full-scanned EVERY tenant's rows -- leaking cross-tenant cardinality through row_count/distinct_count and reporting an arity that does not describe the caller's own view. It also sidestepped the fail-closed contract: a policy that cannot be applied must raise, not be silently skipped. Both scans now route through _apply_policy, and _preflight_clickhouse_correlated runs first so the correlated-subquery guard has a version to gate on -- the same sequence _execute_pipeline and get_column_types already use. There is direct precedent: the DEV-1587 refresh-key scan applies the policy for exactly this reason. No-op with zero overhead when no policy is configured. `datasource` is threaded through _detect_one_join to reach _side_stats. CodeRabbit (maintainability, valid): the SQL was built by f-string concatenation, against the project rule that SQL generation uses sqlglot AST building. Extracted _side_stats_sql, which builds both statements as ASTs -- table and every key column go through sqlglot, so identifiers are quoted for the target dialect and cannot break out of position. NULL key rows are still excluded from BOTH counts so the two are computed over one population. Tests: the policy rewrite reaching the emitted SQL and shrinking the scanned population; fail-closed propagating out of detect_join_cardinality (the regression guard -- this silently succeeded before); the no-policy path unchanged; and SQL shape incl. composite keys and a hostile identifier (`a"; DROP TABLE users; --`) staying inside its quoting. Re-verified end-to-end against the demo DuckDB: 6 joins, all `confirms`, identical to before the rewrite. 7274 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_detect_join_cardinality.py (1)
431-450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the SQL that reaches the execution boundary.
_spyrecords SQL returned by_apply_policy, not SQL submitted to the datasource. The test checks onlytarget_side.row_count. If the distinct-key scan discards the rewritten SQL and executes the original SQL, this test can pass while the report contains cross-tenant distinct statistics and an incorrect verdict. Capture SQL submitted for both customer-side scans, or assert the scoped distinct statistic and detected verdict. The PR objective requires policy application to reach emitted SQL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_detect_join_cardinality.py` around lines 431 - 450, Strengthen the test around scoped.detect_join_cardinality so it verifies policy-rewritten SQL at the datasource execution boundary, not only SQL returned by scoped._apply_policy. Capture the SQL submitted during both customer-side scans and assert the distinct statistic and detected verdict reflect the US-only scope, ensuring rewritten SQL is actually emitted and executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_detect_join_cardinality.py`:
- Around line 431-450: Strengthen the test around scoped.detect_join_cardinality
so it verifies policy-rewritten SQL at the datasource execution boundary, not
only SQL returned by scoped._apply_policy. Capture the SQL submitted during both
customer-side scans and assert the distinct statistic and detected verdict
reflect the US-only scope, ensuring rewritten SQL is actually emitted and
executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f372ff9e-8130-41c5-a8cd-35121a796628
📒 Files selected for processing (2)
slayer/engine/query_engine.pytests/test_detect_join_cardinality.py
🚧 Files skipped from review as they are similar to previous changes (1)
- slayer/engine/query_engine.py
…dict) CodeRabbit (valid): a table with no non-null key rows gave row_count == distinct_count == 0, so observed_unique came out True on that side. A join between two empty tables therefore "detected" one_to_one, and persist=True wrote that onto the join. An empty scan proves nothing about arity -- this directly contradicted the PR's own evidence model, where profiling may only ever SUGGEST uniqueness. Either side having an empty key population now detects nothing and persists nothing. Note this covers two shapes: a genuinely empty table, and a populated table whose every key value is NULL (NULL rows are excluded from the population, so it profiles as empty too). Both sides' observed stats are still reported for transparency. Reported as a new verdict `no_evidence` rather than reusing `skipped_unsupported`. The two are meaningfully different and consumers should be able to tell them apart: `skipped_unsupported` is a shape that can NEVER be profiled (sql-mode / query-backed model, expression-valued join key), whereas `no_evidence` profiled fine and is worth re-running once data lands. This widens the documented verdict set from five to six; docs/reference/cli.md, docs/interfaces/cli.md and docs/concepts/models.md are updated in step. (The Linear issue's spec section still lists the original five.) Tests: empty-vs-empty detects nothing; all-NULL keys count as empty while the populated far side still profiles; persist=True writes no arity for an empty side; and no_evidence stays distinct from skipped_unsupported in one report. 7278 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1. _is_cross_schema_fk now fails SAFE when the ingested schema is unknown. With an explicit referred_schema but neither `schema` nor the connection's default_schema_name available, the two cannot be confirmed equal -- it previously kept the FK, binding e.g. archive.customers to a bare local `customers` model. The failure modes are asymmetric: wrongly skipping costs a missing join (visible, addable by hand), while wrongly keeping binds to the wrong table and silently derives cardinality from it. The default_schema fallback is retained, so a same-schema FK reflected as referred_schema="public" while ingesting the default schema is still kept. 2. edit_model docstring no longer claims joins "stay LEFT". ModelJoin.join_type supports INNER (join_sync's mirrored reverse edges are INNER), so that was simply wrong. It now says what is actually true: cardinality does not change join_type or query results. 3. _safe_introspect logs the swallowed reflection failure at debug level. An unsupported backend and a permissions error or driver defect otherwise produce an identical silent []; the result is not a crash but thinner metadata -- a missing unique key-set makes infer_structural_cardinality return None, recording a real many_to_one as undetermined with nothing in the logs to say why. 4. Keyword arguments for the multi-parameter calls this PR introduced (_is_cross_schema_fk, _safe_get_pk_constraint, _get_unique_key_sets, _pk_key_sets, _unique_constraint_key_sets, _unique_index_key_sets, _get_single_column_unique_names), per the project convention. Pre-existing call sites are left alone to keep the diff reviewable. 5. The RLS test now asserts at the EXECUTION boundary. It previously spied on _apply_policy's return, which only proves the rewrite was computed -- if the scan then discarded it and submitted the original SQL, the test would still pass while the report carried cross-tenant statistics. It now records what reaches the SQL client and asserts BOTH customer-side scans carry the tenant filter, plus that the scoped row/distinct counts (1 of 3 customers) are what the report actually contains. 7279 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`assert a is None and b is None` reports only "assertion failed" when either side breaks. Split so the failure names which finding regressed. 7279 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_detect_join_cardinality.py (1)
557-587: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest a populated source with an empty target.
The tests cover a zero source population and a zero/zero pair. They do not cover a nonzero source with a zero target population. If the implementation checks only
source_side.row_count, it can infer and persist a cardinality for that case.Add a
persist=Truetest that populatesempty_src, leavesempty_tgtempty, and assertsNO_EVIDENCE,detected is None, and an unchangedModelJoin.cardinality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_detect_join_cardinality.py` around lines 557 - 587, Add a persistence test alongside test_empty_side_is_never_persisted that uses a populated empty_src with empty_tgt still empty, then runs detect_join_cardinality with persist=True. Assert the report has CardinalityVerdict.NO_EVIDENCE and detected is None, and verify the reloaded ModelJoin.cardinality remains unchanged (None).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_detect_join_cardinality.py`:
- Around line 557-587: Add a persistence test alongside
test_empty_side_is_never_persisted that uses a populated empty_src with
empty_tgt still empty, then runs detect_join_cardinality with persist=True.
Assert the report has CardinalityVerdict.NO_EVIDENCE and detected is None, and
verify the reloaded ModelJoin.cardinality remains unchanged (None).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aef31efc-d9d9-4b23-be8d-c81c312a1d7a
📒 Files selected for processing (9)
docs/concepts/models.mddocs/interfaces/cli.mddocs/reference/cli.mdslayer/engine/cardinality.pyslayer/engine/ingestion.pyslayer/engine/query_engine.pyslayer/mcp/server.pytests/test_detect_join_cardinality.pytests/test_ingestion_cardinality.py
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/interfaces/cli.md
- docs/reference/cli.md
- docs/concepts/models.md
- slayer/mcp/server.py
- tests/test_ingestion_cardinality.py
- slayer/engine/cardinality.py
- slayer/engine/query_engine.py
- slayer/engine/ingestion.py
The empty-population tests covered a zero SOURCE (empty table, and all-NULL keys) and a zero/zero pair, but not a populated source pointing at an empty target. The implementation checks both sides, so behaviour is already correct -- but nothing pinned the asymmetric case, so a later change that looked only at source_side.row_count would infer and persist an arity off a target scan that read nothing. New populated_src fixture (3 rows) joined to the still-empty empty_tgt, run with persist=True, asserting NO_EVIDENCE, detected is None, the real source/target row counts (3 and 0), and an unchanged ModelJoin.cardinality. Also regrouped this file's imports (stdlib / third-party / first-party) after earlier insertions split the blocks. 7280 passed, 0 failures; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only DECISIONS.md conflicted — an append-only log both sides had appended to. Resolved per the file's own rule (chronological, newest last, never reorder existing entries): main's entries keep their order, the DEV-1688 entry goes last. That entry is also refreshed to cover what the review rounds added since it was written: the no_evidence verdict for an empty key population, the composite-PK / expression-index / partial-index cases that fail to establish solo uniqueness, cross-schema FK skipping, and profiling scans routing through the RLS session policy. Merged tree: 7884 passed, 0 failures; ruff clean. (The merge adds esprima as a dependency — poetry install required.)
…e refs; trim docstrings
Three reviewer follow-ups.
1. Merge `slayer joins detect-cardinality` into `slayer validate-models`.
The `joins` group is deleted outright (new in this PR, never shipped).
New flags: --model, --cardinality, --persist-cardinality, --format.
Default output is unchanged; the full-scan half stays opt-in. Engine
methods stay separate so MCP / REST validate_models keep their
metadata-only cost profile.
Two robustness fixes surfaced by the plan review:
- An unscoped run now enumerates datasources in the CLI and exits 1 on a
per-datasource failure. validate_models(None) gathers with
return_exceptions=True, so the command used to report "no drift" for a
datasource it never reached.
- A per-join scan failure becomes a `scan_failed` finding instead of
aborting the whole report — except ForcedFilterError, which propagates,
since downgrading a fail-closed policy error to a report line would hand
back unscoped statistics.
--model scopes the report AND --force-clean, and the residual from
apply_drift_deletes (which re-validates whole datasources) is filtered
before both the printout and the exit-code decision.
2. Remove every Linear issue reference from lines this PR adds. DECISIONS.md
is exempt by its own documented convention.
3. Cut PR-added comment/docstring lines 854 -> 555. Line-level traps moved to
one-line comments at the guard they protect; no executable line changed by
that pass.
- Restore concurrency in the CLI's per-datasource drift pass. The explicit loop that replaced validate_models(None) was serial; it now gathers and zips results back to names, keeping per-datasource attribution without making a many-datasource install pay the sum of every latency. (Codex) - Exit 1 when any join reports `scan_failed`. Containment stops one unreadable table from costing the whole report; it must not also make the command claim success — with --persist-cardinality that meant partial writes and exit 0. The full report still prints. (Codex) - Keep stdout parseable. A datasource failure printed the TEXT drift report even under --format json, corrupting the document. Diagnostics now go to stderr and stdout always carries the requested format. Exit-code rule is now explicit: 0 for any report ABOUT the data (contradicts_hard included), 1 whenever the command could not do its job. (self-found while verifying) - Hoist `_args(...)` out of every `pytest.raises` block so only the call under test can raise. (Sonar python:S5778 x5) - Extract `_solo_unique_columns_for_table`, removing the duplicated PK + unique-columns block this PR had added to both `introspect_table_to_model` and `ingest_datasource`. (Sonar python:S3776) CodeRabbit's four review-summary nitpicks were all from the 2026-08-04 rounds and are already addressed in earlier commits (reflection-failure logging, keyword args, execution-boundary assertion, populated-source/empty-target case); the one still-open instance of the keyword-args nitpick is fixed here.
- Re-raise asyncio.CancelledError from the per-datasource gather instead of filing it as a datasource that failed validation. gather(return_exceptions= True) hands a cancelled child back as a CancelledError value, so the broad BaseException check was turning cancellation into a diagnostic (usually with an empty message) plus exit 1. (Codex) - Extract `_collect_fk_columns_by_table` from `ingest_datasource`. The previous commit's extraction moved branch-free code, so cognitive complexity stayed at 17; the triple-nested FK-column loop was the actual driver. Also collapses the innermost loop into a set update. (Sonar python:S3776)
Closes the remaining instance of CodeRabbit's keyword-arguments nitpick on a line this PR added. The other positional call sites of that helper are pre-existing context and stay out of scope.
Five conflicts, all in files this branch and main both restructured. - slayer/engine/ingestion.py — main rewrote ingestion for views and internal tables: `ingest_datasource` is now a thin wrapper over `ingest_datasource_report`, with the per-table body extracted into `_build_one_model`. Took main's structure wholesale and re-wired this branch's two hooks into it: `_generate_joins(..., sa_engine=sa_engine)` for structural cardinality, and `unique_columns=_solo_unique_columns_for_table( ...)` on the `_columns_to_model` call. Main had independently extracted the same FK-column helper as `_collect_fk_columns`, with a per-table guard this branch's version lacked (views have no FKs and some dialects raise rather than return []). Kept main's and dropped ours. `_additive_merge_existing` now returns an `AdditiveMergeResult` instead of a 4-tuple; carried this branch's metadata-only gate across as a `metadata_changed` field, so a cardinality/unique fill still forces a save the way a `kind_changed` view→table flip does. - slayer/core/enums.py, slayer/engine/query_engine.py — both-added imports and declarations; kept both sides. - DECISIONS.md — append-only log, both sides appended; kept both in date order. - docs/reference/cli.md — main added `#### Views` / `#### Recognised internals` / `#### Exit codes` under `slayer ingest`; this branch added a new `### slayer validate-models` section. Main's subsections complete the ingest section and now precede ours. tests/test_join_cardinality_models.py asserted `version == 7` to show the cardinality/unique fields forced no schema bump. Main bumped SlayerModel to v8 for `source_kind`, so the assertion now reads against the model's own default version — the point being made is "not bumped BY these fields", which a literal cannot express across an unrelated bump. Full non-integration suite: 8101 passed. Ruff clean. Smoke-tested end to end: a UNIQUE column still ingests `unique: true`, an FK join still gets `cardinality: many_to_one`, and both sit alongside main's new `source_kind`.
…V-1741) An FK pointing at an object whose name contains `__` produced a join to a model that does not exist. Model names strip `__` (it is reserved for join paths), so `reports__patient__drug` ingests as `reports_patient_drug`, but `_generate_joins` wrote the live name into `target_model`. Join traversal to that model silently found nothing. Pre-existing on main (the same line is there before this merge); it collides with this branch because the join it breaks is now the one carrying `cardinality`. Small enough to fix here rather than leave a known-dangling target in code this branch is actively changing. `_generate_joins` takes the live→model mapping and names the model; a target with no model (sanitizing collided, so it was skipped) drops its join instead of dangling. Column introspection needs the opposite direction — the live object name — so `_introspect_query_columns_via_inspector` takes the inverse lookup and keeps using the model name for the path alias. Also rewrites the additive-fields version test, which Codex correctly called vacuous: it asserted a migrated v7 payload ended up at the current default version, which passes after any bump. Now stamps the payload at the CURRENT version, so nothing migrates and the assertion actually fails if either field ever needs a migration step; a separate test keeps the v7-still-loads case.
… with none Two follow-ups to the previous commit. 1. A model persisted BEFORE the target-name fix keeps a join naming the live object. `_merge_joins_strict` compares (target_model, pairs) signatures, so the corrected fresh join looked like a different join: the dangling legacy one survived and the correct one was appended alongside it. `_repair_legacy_join_targets` rewrites such a target to its sanitized form, but only when a fresh join already targets that name — which proves the model exists under it. Anything else is left alone rather than repointed at a guess. The repair sets `metadata_changed`, so it persists on the normal re-ingest path with no schema version bump; a store that never re-ingests keeps a join that was already dangling. 2. `_introspect_query_columns_via_inspector` treated an EMPTY join list the same as no list, so dropping every candidate join sent it down the fallback and it introspected the skipped object, emitting `a__b.label`-style names. `_columns_to_model` discards dotted names, so no bad model ever reached storage — the cost was pointless introspection round-trips against an object with no model, and a trap if that filter ever moves. Now keyed on `joins is not None`. The test for this asserts at the function that has the behaviour rather than on the resulting model, where the dotted-name filter would make it pass whether or not the fix is present.
…rget Matching on the sanitized target name alone was too loose. `a__b` and `a___b` both sanitize to `a_b`, so any fresh join targeting `a_b` repaired BOTH — leaving two joins with the same target_model, which then trips the duplicate-target guard in `_merge_joins_strict`. A store that merely carried a dangling join would start failing re-ingest outright, which is worse than the problem being fixed. The loose match could also repoint a hand-authored join whose join_pairs differ from the fresh one. The repair now demands the sanitized target AND identical join_pairs to match a fresh join, so it can only rename the join the bug produced, and it refuses a name already claimed by another join on the model. Tests cover the collision case (no duplicate targets, no raise) and idempotence (a repaired target sanitizes to itself, so repeat re-ingests are no-ops).
The standalone idempotence test passed with the fix reverted — a single legacy target is repaired on the first pass and stable thereafter either way, so it guarded nothing. Folded the loop into the collision case instead, which needs the signature match to survive: the second persisted join sanitizes to the same name but has different pairs, so a name-only repair repoints it and the assertion fails. Verified by reverting the fix (fails) and restoring (passes).
|
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.
Closes DEV-1688.
Gives SLayer a first-class, explicit representation of join cardinality, matching every major semantic layer (LookML/Cube
relationship, MetricFlow entity types). Auto-determined structurally at ingest (free) and via opt-in data profiling. Adds auniquecolumn flag as the structural signal, and fixes composite-FK ingestion as a bundled workstream.Represent well; query-time use is a separate ticket. Cardinality is metadata only — it is NOT threaded into SQL generation,
resolved_joins, orCrossModelMeasure. No query results change.Key design decision: LEFT default, cardinality orthogonal
FK-derived joins stay LEFT, never auto-INNER. FK enforcement is DB-specific (Snowflake/BigQuery/Redshift/ClickHouse don't enforce; SQLite is a per-connection pragma) and nullable FKs are common, so INNER is only equivalent in the narrow enforced-and-non-nullable case — and when the assumption is wrong it silently drops base rows. An INNER hop would also break the core invariant that adding a measure/join never changes result cardinality. The "tightness" of reality goes into the metadata, not the join type.
Evidence model
Uniqueness is asymmetric evidence:
So a mismatch is
contradicts_hardonly when observed data disproves a uniqueness the stored value asserted; every other mismatch is a softrefines. Structural inference leaves cardinality unset unless the target key is verified unique. A side counts as unique iff some unique key-set is a non-empty subset of the join key — if(a)is unique so is(a, b), but a constraint on(a, b)does not make(a)unique.What's included
JoinCardinalityenum +invert_cardinality;ModelJoin.cardinality,Column.unique. Both additive/optional — noSlayerModelversion bump, old v7 data validates unchanged.ModelJoinper FK constraint instead of shredding a composite FK into one under-constrained join per column.engine.detect_join_cardinality(*, data_source, model, persist=False)— full-scan profiling, report-only by default. Surfaced asslayer joins detect-cardinality [--datasource X] [--model M] [--persist] [--format text|json].many_to_one, peerone_to_one), OSI (many_to_one), facade dynamic joins (many_to_oneonly when the target column is PK/unique). Mirror inversion on the reverse INNER edge in bothjoin_syncand the dbt in-memory mirror.inspect(joins table + columnunique, md + json) and the search-graphJOINSedge. Excluded from the embedding corpus so no re-embedding churn fires.Notable fix found during review
Running the feature end-to-end against the demo DuckDB (rather than trusting the unit tests) surfaced a false positive:
_unique_contradictionstreated anyprimary_keycolumn as a solo uniqueness claim, so every member of a composite PK was reported as "declared unique but the data has duplicates".jaffle_shop.supplies(PK(id, sku), joined onskualone) tripped it — it would have misfired on every composite-PK table. Fixed to count a PK column as a solo claim only when it is the whole primary key, consistent with the subset rule above. Two regression tests added.Testing
ruff check slayer/ tests/cleanfills_none→--persistwriting back to the correct join while leaving siblings untouchedDocs
DECISIONS.mdentry,docs/concepts/models.md(join-cardinality section +uniquefield),docs/concepts/ingestion.md, both CLI reference pages,docs/dbt/dbt_import.md,docs/osi/osi_import.md,.claude/skills/slayer-models.md, and thecreate_model/edit_modelMCP docstrings.Note:
edit_model'scolumns/joinsparams arelist[dict[str, Any]], so MCP exposes no per-field schema for any of their fields (primary_keyincluded) — the docstring is the only channel that reaches an agent. Giving them typed Pydantic schemas is a worthwhile follow-up but a cross-cutting refactor beyond this issue.🤖 Generated with Claude Code
Summary by CodeRabbit
slayer joins detect-cardinalitywith filtering, JSON/text output, contradiction reporting, and optional persistence.