feat: managed database demo flow — explicit flags, catalog resolution, BM25 search#127
Conversation
…list - Replace dot-notation positional <TARGET> on `databases load` with explicit --catalog, --schema, --table flags - Add `databases unset` to clear the active database from config - Show * marker on the active database in `databases list` - Remove parse_db_target and its tests (no longer needed)
…e on load - try_resolve_database now falls through to match by default_catalog when neither id nor name match, so --catalog works as a lookup key everywhere - databases load auto-recovers from "not declared": deletes the empty database, recreates it with the table declared, then retries the load - Add default_catalog to DatabaseSummary so the list response can be matched without a per-row fetch
…x duplicate score column - resolve_connection_id falls back to managed database catalog lookup so `airbnb4.listings[description]` works in indexes create and search - BM25 search no longer appends 'score' when --select already includes it
…og/--schema/--table/--column flags Removes the positional `connection.table[col1,col2]` target argument and parse_index_target helper. All index creation now uses named flags, consistent with databases load and search.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Blocking Issues
- src/databases.rs (auto-declare-on-load, ~823-857): the
not declaredrecovery deletes the entire managed database and recreates it declaring only the single table being loaded. This silently destroys any other declared tables and their loaded data, orphans indexes tied to the olddefault_connection_id, and discardsexpires_at. See inline comment for a suggested additive-declare alternative.
Action Required
- Replace delete+recreate with an additive table declaration (e.g. PATCH
/databases/{id}), or — if recreate is unavoidable — preserve the existing declared tables andexpires_atand warn/confirm before deleting.
Non-blocking
- src/api.rs:184 —
workspace_id()splitstest_new's doc comment (see inline comment). - src/databases.rs:656 — the
databases createsuccess hint still prints the old dot-notationdatabases load --file <path.parquet> {}.<table_name>, which no longer works after this PR's switch to--catalog/--schema/--table.
- databases load: explicit --catalog/--schema/--table flags (no more dot-notation) - databases list: note * marker on active database - databases set/unset: documented - indexes create: --catalog option for managed databases (in addition to --connection-id) - search: --type and --column are now optional (inferred from indexes) - workflows: updated examples throughout
There was a problem hiding this comment.
Review
Blocking Issues
src/databases.rs:826-857— The auto-declare path still deletes the entire managed database and recreates it with only the single target table declared andexpires_at: None. This drops any other declared tables (and their loaded data), orphans indexes bound to the olddefault_connection_id, discards the original expiry, and does so transparently with no warning. This was flagged in cycle 1 and remains unaddressed in the code.
Action Required
- Make table declaration additive (e.g. PATCH
/databases/{id}via the existingApiClient::patch) instead of delete+recreate. If delete+recreate must stay, preserve the existing declared tables andexpires_atin the recreate request, and warn/confirm before deleting.
- auto-declare: collect existing tables before delete+recreate so they are preserved in the new database; also pass expires_at through - databases create hint: update to new --catalog/--table flag syntax - api.rs: fix workspace_id() doc comment placement
There was a problem hiding this comment.
Review
Blocking Issues
src/databases.rs(load auto-declare path, ~L826-874) — The undeclared-table load path still deletes the entire managed database and recreates it. This cycle correctly preserves the table declarations,name,default_catalog, andexpires_at, but re-declaring a table does not restore its loaded data: any table already populated is left empty after the delete+recreate. It also still runs with no warning/confirmation, and mints a newdefault_connection_id(orphaning prior indexes). See inline comment.
Action Required
Either declare the new table additively via PATCH /databases/{id} (no data destroyed), or — if delete+recreate must stay — detect when any existing table is synced and warn/confirm before deleting, since that data will be lost.
When a 'not declared' error triggers delete+recreate, check if any existing tables are synced. If so, show a yellow warning listing the tables whose data will be lost and prompt for y/N confirmation. In non-interactive mode (CI, piped stdin, --no-input) the command errors out with a clear message instead of silently destroying data.
There was a problem hiding this comment.
Cycle 4 — all prior blocking feedback resolved.
The auto-declare path now implements the accepted fallback from cycle 3: it detects synced tables, warns explicitly which tables' data will be lost, prompts for [y/N] confirmation in interactive mode, and aborts in non-interactive mode rather than destroying data silently (databases.rs:842-880). The api.rs doc-comment nit is also addressed. No new blocking issues.
There was a problem hiding this comment.
Cycle 4 — all prior blocking issues resolved.
The data-loss concern is now addressed: the auto-declare path preserves existing table declarations, name, catalog, and expires_at, detects tables with synced data, warns listing exactly which tables would lose data, confirms before deleting in interactive mode, and refuses (aborts) in non-interactive mode rather than destroying data silently. This matches the previously-stated minimum acceptance criteria for the delete+recreate path.
The api.rs doc-comment nit is also resolved. Approving.
1 participant
Summary
databases load: replaced positionaltargetwith--catalog,--schema,--table(plus--file/--url/--upload-id)databases unsetcommand: clears the active database from configdatabases list: highlights the active database (mirrors active workspace UX)try_resolve_database: catalog alias matches are preferred over display name matches to avoid stale name collisionsresolve_connection_id: checks the active database first (before scanning the connections list) so stale managed connections with the same catalog name are bypassed — fixesindexes createandsearchtargeting the wrong connectioninfer_for_searchusesresolve_connection_id: catalog fallback + active-database preference flow through consistentlyscorecolumn in BM25 search: when--selectalready includesscore, it is not appended againindexes create: replaced dot-notation with--catalog,--schema,--table,--columnDemo flow (now works end-to-end)
hotdata databases create --catalog airbnb hotdata databases load --catalog airbnb --schema public --table listings --url https://hotdata.dev/data/sf-airbnb-listings.parquet hotdata indexes create --catalog airbnb --schema public --table listings --column description --type bm25 hotdata search "cozy apartment with a view" --table airbnb.listings --column description --select name,descriptionTest plan
cargo test)databases create --catalog airbnb→ creates DB, sets activedatabases load --catalog airbnb --table listings --url ...→ auto-declares + loads 7,535 rows with correctfull_name: airbnb.public.listingsdatabases list→ active DB marked with*indexes create --catalog airbnb ...→ creates index on the correct (active) connection, not a stale onesearch ... --table airbnb.listings→ returns ranked BM25 results with scores