SCRUM-1078-maintain verify: report joins whose keys have no overlap - #473
Open
emnarahmouni-exmergo wants to merge 3 commits into
Open
emnarahmouni-exmergo wants to merge 3 commits into
emnarahmouni-exmergo wants to merge 3 commits into
Conversation
The model's own compiled SQL was already a real join-contract candidate (customers inner-joined to events), which the pinned-DuckDB parse bug this branch already fixed had been silently masking: every prior sweep passed only because the join was never detected, not because the fixture correctly answered a probe. Update the offer axes assertion to name both axes now that both have real work, and teach the fake BigQuery client's row_resolver to answer the overlap-probe query shape alongside the row-count one it already knew, reporting the join healthy so it adds no finding these tests are not about. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sqlglot now parses from_json() into a dedicated FromJson class instead of falling through to Anonymous, which the name-based allowlist no longer catches. Uses the same getattr/filter pattern _QUERY_ROOTS already relies on, so it's a no-op on the pinned version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This branch has not been deployed
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.
Summary
maintain verifyreports a selected model's own equality joins whose keys have no (or too little) value overlap, reusing the exact overlap probeexplore relationships --verifyalready runs (explore.relationships.verify_relationships) rather than authoring a second one.join_zero_overlap(high severity, the most damaging and least visible defect this sweep can report); one with real but non-catastrophic orphans isjoin_orphans(medium) — also where a left join whose right side is legitimately sparse lands, reported at its measured fraction rather than characterized as broken.JOIN ... USING (col)is recognized as the equality it implies, the same as a writtenON, but only turned into a candidate when exactly one relation is already in scope to pair it against — a chainedUSINGwhere the column was introduced by an earlier join (not the originalFROM) is skipped rather than guessed against the wrong table.OR, a range predicate, or any other non-equality conjunct causes the whole join to be skipped (never probed on a partial subset of its real condition, which can under- or over-report the true orphan rate), and the skip is named inwarningsrather than silent.ON a.x = b.x AND a.y = b.y) is measured as one combined key, not two independent single-column probes. A self-join is only excluded when both sides are the literal same key on the same relation (t.col = t.col); one on different columns (employees.manager_id = managers.id) is a real, checkable join.SELECT *— refusing (and skipping the whole join) rather than guessing wherever that isn't safely determinable: a computed projection, an ambiguous star, a qualified table reference sharing a CTE's short name, a scope that filters or aggregates before the join reads it (which would measure the probe against the wrong row set), or a column read directly off a set-operation CTE's output (which has no single physical source across its branches).UNION/INTERSECT/EXCEPTare all checked independently.join_contract_planparses each model's compiled SQL in the connector's actual dialect (matchingrow_population_plan's existing convention), not hardcoded to DuckDB.VerifyResult.pending_offerholds only one offer and both row population and join contract can each need a scan on a metered connector, the two share one combined handshake naming both axes, so confirming buys exactly what the estimate described instead of a metered connector silently seeing only whichever axis asked last.sql_shape.py:equality_column_pairs(raw columns behindequality_columns's existing rendered-text pairs) andconjunctive_equality_pairs(also respects the condition's Boolean structure).Test plan
_model_joins) before touching code, and re-verified against the same snippet after the fixJOIN ... USING(simple and ambiguous-chained), self-joins (real and trivial), joins inside joined CTEs, joins inside inline subqueries, qualified tables sharing a CTE's short name, filtered/aggregated CTEs, set operations at the top level/inside a CTE/inside a subquery, the combined-handshake pricing and caching, and the connector-dialect parsetests/maintain/test_verify.py— 118/118 passedtests/maintain/ tests/transform/ tests/explore/ tests/test_sql_shape.py— clean through the prior four rounds (1998 passed/30 skipped pre-existing/0 failed most recently); sweep after the dialect fix is finishing now, will confirm before mergeruff check/ruff formatclean on all touched filesCloses : #228