refactor: replace user-facing "connection" with "catalog" (keep wire contract) - #245
Conversation
…contract) "Connection" was overloaded in the user-facing surface: it named the attachable/queryable thing (`databases attach`, cross-source joins), duplicated the ingest "datasource" concept in stale comments, and even appeared as a `hotdata connections list` command that does not exist. A connection is a distinct server concept from a datasource, so the fix is to rename what users see to "catalog" (how it's already addressed in SQL: `<catalog>.schema.table`, and via `--catalog`/`--alias`) — not to "datasource". Scope is user-facing only. The wire contract is untouched: `/connections/...` endpoints, the `connection_id`/`default_connection_id` JSON keys, the `data_refresh_connection` job-type value (still shown/typed in `jobs --job-type` because the server defines it), and `connection_string` credential keys all stay. Internal identifiers and private-fn rustdoc that say "connection" are left as-is (they track the wire concept). CLI (user-facing): - `databases attach`/`detach`: help, printed output, and the positional arg (`<CONNECTION>` -> `<CATALOG>`); `create --attach` help; `fork` help. - `databases delete` arg help; `databases show` label `default_connection_id:` -> `catalog id:` (the `-o json` key is unchanged). - `indexes delete --catalog` help note; two `query` cross-catalog hints (one had pointed at the non-existent `hotdata connections list`). - Removed the deprecated hidden `ingest *-connection` aliases; corrected the ingest-client comments that called a datasource a "connection". Skills: - Renamed "connection" -> "catalog" throughout (attach, cross-catalog joins, data-model template); renamed the "Querying across connections" anchor to "-catalogs-" and updated all links. - Corrected the earlier connections->datasources mis-mapping: onboarding / model-building now discover queryable catalogs+tables (`databases list`, `tables list`), with ingest datasources as a separate optional step.
88d3ffa to
a70f17d
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| **Skill:** **`hotdata`** | ||
|
|
||
| A `hotdata query` runs inside **one** managed database; its scope sees that database's own catalog plus **attached** connection catalogs only. To query a connection's tables — or join a managed table against a live connection table in one query — attach the connection. (No managed database set → *"a database is required."*; an unattached catalog → *"table not found."*) | ||
| A `hotdata query` runs inside **one** managed database; its scope sees that database's own catalog plus **attached** catalog catalogs only. To query a catalog's tables — or join a managed table against a live catalog table in one query — attach the catalog. (No managed database set → *"a database is required."*; an unattached catalog → *"table not found."*) |
There was a problem hiding this comment.
nit: (not blocking) The mechanical rename garbled this sentence — "attached catalog catalogs", and "join a managed table against a live catalog table" now reads as a tautology (the managed database is itself a catalog). Since skills are agent-facing instructions, ambiguity here has a real cost.
| A `hotdata query` runs inside **one** managed database; its scope sees that database's own catalog plus **attached** catalog catalogs only. To query a catalog's tables — or join a managed table against a live catalog table in one query — attach the catalog. (No managed database set → *"a database is required."*; an unattached catalog → *"table not found."*) | |
| A `hotdata query` runs inside **one** managed database; its scope sees that database's own catalog plus any **attached** catalogs only. To query another catalog's tables — or join a managed table against a live attached table in one query — attach that catalog. (No managed database set → *"a database is required."*; an unattached catalog → *"table not found."*) |
| ``` | ||
|
|
||
| - **Fully qualify tables** as `<connection>.<schema>.<table>` (or `<catalog>.<schema>.<table>` for a managed database) — every `<table>` placeholder below means a qualified name. | ||
| - **Fully qualify tables** as `<catalog>.<schema>.<table>` (or `<catalog>.<schema>.<table>` for a managed database) — every `<table>` placeholder below means a qualified name. |
There was a problem hiding this comment.
nit: (not blocking) After the rename both halves of this sentence are identical, so the parenthetical says nothing: "as <catalog>.<schema>.<table> (or <catalog>.<schema>.<table> for a managed database)".
| - **Fully qualify tables** as `<catalog>.<schema>.<table>` (or `<catalog>.<schema>.<table>` for a managed database) — every `<table>` placeholder below means a qualified name. | |
| - **Fully qualify tables** as `<catalog>.<schema>.<table>` (for a managed database, `<catalog>` is its SQL alias) — every `<table>` placeholder below means a qualified name. |
| ``` | ||
|
|
||
| For each datasource, record `id`, `name`, and `source_type`. | ||
| For each catalog, record its name and the tables it exposes. (Pulling *new* external data into a managed database is a separate step — see the `ingest` datasource commands in the core skill.) |
There was a problem hiding this comment.
super nit: (not blocking) The old step 1 was the only place that told the agent to record source_type, but section 3 still keys off it ("Use connector and tooling docs when source_type (or table shapes) match", line 53). Now nothing in the walkthrough produces it. Worth adding a clause here — e.g. "for ingest-backed catalogs, also record source_type from hotdata ingest list-datasources (used in step 3)".
| hotdata databases attach <connection_id|name> [--database <id>] [--alias <alias>] | ||
| hotdata databases detach <connection_id|name|alias> [--database <id>] | ||
| # Attach a catalog so its tables are queryable (enables cross-catalog queries — see below) | ||
| hotdata databases attach <catalog|name> [--database <id>] [--alias <alias>] |
There was a problem hiding this comment.
super nit: (not blocking) <connection_id|name> → <catalog|name> loses the id/name distinction the old form carried — as written, name looks like an alternative to a catalog rather than one way to name one. <catalog_id|name> would keep the parallel (same for detach on the next line: <catalog_id|name|alias>).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Rename is consistent; wire contract (connection_id, /connections/*, data_refresh_connection, connection_string) is correctly left alone, main.rs destructuring matches the renamed clap fields, and no stale #querying-across-connections-attach anchors or references to the removed ingest *-connection aliases remain outside historical CHANGELOG entries. Four non-blocking doc nits inline.
1 participant
What
Remove the user-facing concept of a "connection" from the CLI and skills, renaming it to "catalog". Scope is user-facing only — the wire contract is untouched.
Stacked on #244 (this PR's base is
refactor/skills-nested-subskills); GitHub will retarget it tomainonce #244 merges.Why
"Connection" was overloaded in what users see:
databases attach, cross-source joins)./ingest/sourcesvs/connections, different id-spaces).queryhint even pointed athotdata connections list, a command that does not exist.Since a connection is distinct from a datasource, the right user-facing term is catalog — it's already how the thing is addressed in SQL (
<catalog>.schema.table) and via--catalog/--alias, and the resolver error already said "no catalog with id".Wire contract — deliberately unchanged
/connections/...endpoint paths, and theconnection_id/default_connection_idJSON keys (still emitted by-o json).data_refresh_connectionjob-type value — still shown in thejobsTYPE column and typed via--job-type, because the server defines that enum string. This is the one place the word remains user-visible; removing it needs a backend change.connection_stringcredential keys (Postgres DSNs).Changes
CLI (user-facing)
databases attach/detach: help, printed success/failure, and the positional arg (<CONNECTION>→<CATALOG>);create --attachhelp;forkhelp.databases deletearg help;databases showhuman labeldefault_connection_id:→catalog id:(the-o jsonkey is unchanged).indexes delete --cataloghelp note; the twoquerycross-catalog hints (one had referenced the non-existenthotdata connections list).ingest *-connectionaliases; fixed the ingest-client comments that called a datasource a "connection".Skills
connection→catalogthroughout (attach, cross-catalog joins, data-model template); renamed the "Querying across connections" heading/anchor to-catalogs-and updated every link.connections→datasourcesmis-mapping from refactor(skills): bundle search/analytics/geospatial under hotdata + fix CLI drift #244: onboarding / model-building now discover queryable catalogs + tables (databases list,tables list), with ingest datasources as a separate optional step.Verification
cargo build+cargo testpass (updated the twodatabases_cli.rshelp assertions);cargo fmt --checkclean.--helpoutput: the only remaining user-facing "connection" is the wire-lockeddata_refresh_connectionjob value.data_refresh_connectionandconnection_string(both wire).Note (not addressed here)
tests/connections_read.rsandtests/auth_unknown_workspace.rsinvokehotdata connections list— a command that doesn't exist in the CLI. These are production scenario tests mirroring a server-sidetest-scenarios.yaml. They're pre-existing and out of scope for this PR, but worth a look: either the scenario contract expects aconnectionscommand the CLI never implemented, or the tests are stale.