Merge a warehouse catalog into an authored dictionary - #275
Merged
Conversation
jat255
marked this pull request as draft
September 4, 2026 05:49
jat255
added
needs-manual-review
jat255
force-pushed
the
jat255/xp65-catalog-merge
branch
from
September 5, 2026 04:58
0fa5884 to
730db47
Compare
jat255
force-pushed
the
jat255/xp65-catalog-merge
branch
from
September 6, 2026 02:07
730db47 to
52cd57c
Compare
Collaborator
Author
|
Reviewed 2026-09-05 23:24 MDT (final manual review pass). Findings from the review were addressed in c494065:
Deliberately not changed: exclusion globs still match the bare table name case-sensitively (R agrees, and the fixture now pins it — a schema-qualified or case-folding exclude would be a cross-package decision, flagged for follow-up). Checks on the pushed head: ruff clean, pyrefly 0 errors, 761 tests pass; the new R fixture runner passes (57 expectations). Note: the full R suite could not run locally (ellmer 0.4.2 installed, pkg-r requires >= 0.5.0) — CI will be its first full run. |
jat255
removed
the
needs-manual-review
jat255
marked this pull request as ready for review
September 6, 2026 05:31
Second of the catalog import stack (kata xp65). Ports catalog.R: selection into labelled relations, the exclusion globs and object cap, the manifest that decides whether a listing is small enough to put in a prompt, catalog search, and the dictionary merge. The rules the R side settled and this keeps: authored prose wins, warehouse types and nullability are authoritative, identifier case normalizes per backend, and an ambiguous relative name is an error rather than a guess. Discovered columns come first in the order the warehouse reported them, and authored columns with no counterpart follow. Every function here is pure over the rows a listing returns, so none of it needs a warehouse. Running the queries that produce those rows belongs to the per-backend readers, which is the next two PRs. Two departures from R, both because Python's types differ. A selection entry is a Selector rather than a TableId, because a selection may stop at a catalog or a schema and a TableId always names a table; that also lets id_type() be one dialect-independent function rather than an injected backend one. And the authored table alias is a declared `authored_name` field on Table rather than an attribute set on the fly, since the model is pydantic. The merge rules were confirmed to bite by letting an authored type win over the warehouse and by leaving glob metacharacters unescaped.
…y both names Two defects in the catalog core. An entry naming a table whose relation the warehouse did not return was dropped, including from the validation list, so asking for a table that is not there became a quietly smaller selection instead of an error the backend reports. R keeps an undiscovered stub and validates it; that is restored, and an excluded entry is still dropped from both. The authored_name field added in the previous commit was never read. Relationship prose says what the author wrote, and the merge re-keys a table to its warehouse label, so a relationship on `orders` stopped reaching the first-touch entry of ANALYTICS.PUBLIC.ORDERS. Rendering now matches either name. Found by roborev job 331.
…red contract Review follow-ups: - The merge dropped Relation.kind where R carries it onto the merged table; Table gains a kind field and the merge sets it, the warehouse's kind winning over an authored one. - The search() queryable probe path and merge_dictionary's access_check had no coverage; both are now exercised, as are the object-cap and prompt-threshold boundaries (exactly at the limit is allowed). - Duplicate-label refusal now reports every duplicated label, matching R, and a non-whole limit is refused rather than dying in a slice. - tests/shared/catalog-merge.json pins the cross-language contract: the three limits, exclusion-glob behavior over bare table names (case-sensitive by design), and seven merge scenarios with expected tables, columns, and definition bindings. Both suites run it; the R runner is new and passes all cases. - Docstrings on table_registry, search, and merge_dictionary now state their raise conditions, and excluded() documents that matching is bare-name and case-sensitive.
jat255
force-pushed
the
jat255/xp65-catalog-merge
branch
from
September 6, 2026 06:29
c494065 to
73f1c1e
Compare
|
Preview deployed to Connect ( Deployed from commit 73f1c1e. |
|
Preview deployed to Connect ( Deployed from commit 73f1c1e. |
simonpcouch
approved these changes
Sep 6, 2026
|
Cleaned up 1 preview bundle(s) on https://dogfood.team.pct.posit.it: 368187 |
|
Cleaned up 1 preview bundle(s) on https://connect.staging.pct.posit.it: 2646 |
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
2 participants
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.
Second PR of the catalog import stack (kata task
xp65). Stacked on #274. Portscatalog.R: selection into labelled relations, exclusion globs and the object cap, the manifest that decides whether a listing belongs in the prompt or behind search, catalog search, and the dictionary merge.The python side follows the same rules used on the R side: authored prose wins, warehouse types and nullability are authoritative, identifier case normalizes per backend, and an ambiguous relative name is an error rather than a guess. Discovered columns come first in the order the warehouse reported them; authored columns with no counterpart follow.
R changes
@simonpcouch
The changes on the R side are tests only —
pkg-r/R/is untouched.tests/shared/catalog-merge.jsonis a new cross-language contract: the three catalog limits (object cap, prompt threshold, search probe bound), exclusion-glob semantics, and seven merge scenarios with expected merged tables, columns, and definition bindings.pkg-r/tests/testthat/test-catalog-merge.Ris the R runner for it, covering the limits,catalog_excluded, andcatalog_merge_dictionaryagainst fixture-built relations.pkg-r/tests/testthat/fixtures/shared/catalog-merge.jsonis the synced copy fromscripts/sync-shared-fixtures.sh.kindexpectations match whatcatalog_merge_dictionary_tablealready does; addingkindto the merged table was a Python-side gap this PR closes.