🧹 chore: Drop legacy facets table from storage schemas#177
🧹 chore: Drop legacy facets table from storage schemas#177yeazelm wants to merge 2 commits intopapercomputeco:mainfrom
Conversation
The facets feature was removed in papercomputeco#155 but the baseline schema still created an empty `facets` table on every new database. Strip it from both the SQLite and Postgres baseline migrations and add a post-migrate cleanup that drops a pre-existing empty `facets` table from databases that already ran the old baseline. Tables that still contain rows are left untouched so any captured data is preserved. Signed-off-by: Matt Yeazel <matt@papercompute.com>
After papercomputeco#173 removed the only `log.Warn` call site in NewLLMCaller, the local `log` variable became an ineffectual assignment that golangci-lint flagged. Nothing sets `Logger` on LLMCallerConfig, so remove the field, the local, and the now-unused log/slog and pkg/logger imports. Signed-off-by: Matt Yeazel <matt@papercompute.com>
26e36c5 to
8e551b7
Compare
jpmcb
left a comment
There was a problem hiding this comment.
Ahh good catch: I think this table is abit of technical debt that we never did anything with when we had the direct deck TUI --> database client (which should have been a materialized view from an API to begin with).
I'm of 2 thoughts:
-
we could completely drop
CREATE TABLE IF NOT EXISTS facetsand not attempt to clean it up (which would be leaving abit of cruft around for very very early adopters but honestly probably not worth too much backwards compatibility woes) -
we write a new
migrations/002_...sqlto correctly handle non-breaking backwards migrations: we do have the mechanism in place to automatically apply new migrations with an embedded filesystem pointed at themigrations/...sqlfiles
tapes/pkg/storage/sqlite/sqlite.go
Lines 21 to 22 in 3bd28e4
When the migrations are applied, all the .sql files get applied in order. So, I think all you'd need to do is add a new 002_drop_facets.sql
-- 002_drop_facets: removes unused facets table
-- this table was technical debt and unneeded
DROP TABLE IF NOT EXISTS facetsThat way, we don't have to have bespoke code paths in Go that act as their own migration handlers/checks.
|
I started off a bit more conservative in case there was something I missed in the clean up but I think we can just stop creating the table and leave the table for early installs. This allows us to avoid carrying the notion of the table forward which is probably the right call if it wasn't widely used. |
Summary
tapes deckbut left thefacetstable behind in both the SQLite and Postgres baseline migrationsCREATE TABLE facets(and its unique index) frompkg/storage/sqlite/migrations/001_baseline_schema.sqlandpkg/storage/postgres/migrations/001_baseline_schema.sqlso new databases never createthe table
Migrate()cleanup on each driver (dropEmptyFacetsTable) that drops a pre-existingfacetstable only when it exists and is empty — databases that still hold captured facet rows are leftuntouched
Test plan
pkg/storage/sqlite/legacy_facets_test.gocover all three cases: fresh DB has nofacetstable, pre-existing empty table gets dropped, pre-existing populated table is preserved (rowcount verified)
pkg/storage/postgres/legacy_facets_test.go, gated onTAPES_TEST_POSTGRES_DSNlike the rest of the postgres suitemake formatcleanmake unit-testpasses (all suites green; postgres legacy specs skip withoutTAPES_TEST_POSTGRES_DSN, as expected)facetsat this build and confirm the table is dropped on next startupTAPES_TEST_POSTGRES_DSNset