Fix AttributeError in port_column_knowls - #7178
Open
roed-math wants to merge 3 commits into
Open
Conversation
db.<table>.port_column_knowls asked knowldb for get_column_description, but the backend has only ever defined get_column_descriptions (plural), so every call raised AttributeError before doing any work. The typo has been there since the function was added in b354cb5; the method was not renamed out from under it. Add tests that run the function with the knowl writes mocked out, so both the copying and the renaming branch are exercised without touching the database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Now that port_column_knowls runs, two problems with what it does are reachable. get_column_descriptions picked its row with DISTINCT ON over an ascending timestamp, which keeps the version a description was created with rather than the current one, so both branches of the port could carry stale content. save inserts a new version without retiring the old ones, so any edited description has several rows to choose from. The same query shape, and the same fix, applies to get_table_description, which backs db.<table>.description(). For keep_old=False the port renamed with actually_rename, which is built for normal knowls: it updates only id and cat, then clears source and source_name. Those are rename markers on a normal knowl, but on a column description they record the table and column, and the stored title and defines are derived from them. The renamed knowl was left describing the table it came from: its edit page took the table-description branch and showed "Edit description for 'None'", and knowl search still had the old title and keywords. Add rename_description_knowl, which rewrites the metadata derived from the new id on every version and recomputes their search keywords, preserving the edit history as actually_rename does, and refusing to move onto an id that already has rows rather than interleaving two histories. actually_rename now rejects knowls that are not normal instead of corrupting them. The tests run the port against an in-memory kwl_knowls rather than mocking the knowl backend, so they cover which version is read and what metadata ends up stored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rename_description_knowl recomputed the keywords of the renamed knowl before rewriting the content of the knowls referring to it, so those referrers were left indexed under the old id, and a description that refers to its own old id was reindexed from content it was about to change. Knowl search matches on _keywords, so the index disagreed with the stored content until the next edit of each knowl. Do all the content rewriting first, then recompute the keywords of every version of the knowl and of its referrers from what is now stored, which also drops the two passes down to one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
No issue to close; found while working on the knowl code.
db.<table>.port_column_knowls(other_table)callsknowldb.get_column_description(other_table), but the knowl backend only ever definedget_column_descriptions(plural,lmfdb/knowledge/knowl.py:495). Every call raisedbefore doing any work, so the function has never run. It is reachable from the command line and is advertised to editors on the defunct-column-knowls page (
knowl-columns.html).This is a typo, not a stale caller left behind by a rename:
get_column_descriptionswas already plural in 017ab53, one day before b354cb5 addedport_column_knowlscalling the singular. Nothing else in the repo references the singular name, and psycodict defines neither name nor any__getattr__fallback onPostgresBase.The plural version is clearly the intended target. It returns a dict mapping column name to
Knowl, which is exactly how the caller consumes it (for col, knowl in knowls.items()).Tests
PortColumnKnowlsTestinlmfdb/tests/test_dynamic_knowls.pyrunsport_column_knowlswith the knowl writes mocked out, following the pattern used elsewhere in that file:knowldb.get_column_descriptions,knowldb.save,knowldb.actually_rename,knowldb.deleteanddb.loginare patched, so nothing touches the read-only devmirror connection.Three tests cover the backend call itself, the
keep_old=Truecopying branch (including that a column the target table lacks is skipped, and that the old knowl is passed asmost_recentso its authors survive), and thekeep_old=Falsebranch (rename the shared column, delete the one this table does not have).All three fail with the
AttributeErrorabove on the unfixed code and pass with the fix.sage -python -m pytest -q lmfdb/tests/test_dynamic_knowls.pygives 12 passed; pyflakes and pylint are clean on both changed files.Ported from roed-math#47, where the comment history lives.
🤖 Generated with Claude Code