Repository navigation
Preserve declared joins on saved query-backed models - #508
Conversation
|
Warning Review limit reachedThis review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Next included review available in 17 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
Most changes support #507, including production join planning, regression tests, schema-drift handling, and related documentation. However, tests/integration/test_notebooks.py adds `remote file has … |
Remove the notebook network-classification changes from this pull request or move them to a separate pull request. Keep only changes that implement or validate issue #507. |
| Docstring Coverage | |
Docstring coverage is 32.35% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 11 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the main change: preserving declared joins on saved query-backed models. Linked Issues check Issue #507requires saved query-backed joins to survive materialization and remain usable for filtering and traversal, while ordinary stages stay flat. The change preserves joins in `_split_query_back…Full details: Linked Issues check
-
Explanation
Issue
#507requires saved query-backed joins to survive materialization and remain usable for filtering and traversal, while ordinary stages stay flat. The change preserves joins in_split_query_backed, carries saved joins through source and sibling construction, and usessame_edgefor repeated extension joins. The added SQLite and DuckDB tests cover filtered totals, aggregates, traversal, incoming edges, extensions, stage isolation, and legal stage targets. No unmet coding requirement is shown.
-
Full details: Out of Scope Changes check
-
Explanation
Most changes support
#507, including production join planning, regression tests, schema-drift handling, and related documentation. However,tests/integration/test_notebooks.pyaddsremote file has changedto DuckDB transient network signatures, andtests/test_notebook_network_guard.pyadds ETag and hostless network cases. These changes concern notebook network-error classification and have no demonstrated connection to saved query-backed joins.Resolution
Remove the notebook network-classification changes from this pull request or move them to a separate pull request. Keep only changes that implement or validate issue
#507.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Move the exact-repeat extension-join dedupe from the query-backed-only planner branch into apply_extension_overlay, keyed on a semantic edge identity (target, name, join type, cardinality, pair set), and mirror it in the schema-drift stage graph. Attach saved joins after the stage-model rename with an explicit key check, route the saved-joins lookup through one bundle helper, and shorten the models-guide addition to one sentence.
Replace design decision 5's saved-relation-only workaround with the one cross-model rule, and add the models/join-traversal delta for it.
DuckDB reports an ETag change across CDN edges as 'this likely means the remote file has changed'; classify it as a transient outage of the named remote host so the notebook skips instead of failing.
|
|
@mattfaltyn Thanks for the PR! |
2 participants
Closes #507.
A query over a saved customer-total model could return 150 instead of 120 for a gold-customer filter. The model's declared join was saved correctly, then discarded when its result became a relation; the filter consequently became an uncorrelated existence check.
Preserve the saved relation's own joins through source, extension and joined-target materialization. Ordinary stages remain flat and do not inherit backing-source joins. An extension join repeating a stored join of its base (ignoring description, meta and pair order) is that stored edge for every model kind, in both the runtime overlay and the schema-drift stage graph, while differing or distinct named edges retain their meaning. No dependency, persisted-format or public request-schema change.
SQLite/DuckDB regressions cover filtered totals, aggregate attribution, multi-hop and joined-target traversal, incoming reverse edges, extensions, repeated stored joins on table and saved models, legal target names and stage isolation. The DEV-1918 strict expected failure now passes. The models guide and OpenSpec change describe the corrected boundary.
Validation
la-arch-check, LikeC4 validation and strict OpenSpec validation pass.Credential-dependent BigQuery/Snowflake jobs are intentionally skipped for fork PRs. Docker dialect and Metabase suites are outside the documented always-on selection and their path-gated workflows are unaffected.
Codex assisted with reproduction, implementation and independent review.