feat(migration): implement --drop-code-tokens DELETE pass - #212
Conversation
Replace the --drop-code-tokens stub in canonical_migration.py with a real targeted set-based DELETE of the code-token / shell-command / stopword RELATION edges (the predicates CanonicalMapper.map_predicate drops to None via mempalace.kg_predicate_norm: CODE_TOKEN_BLOCKLIST + SHELL_COMMAND_BLOCKLIST + STOPWORD_BLOCKLIST + digit/code-look heuristic). - build_plan now captures the dropped raws into plan["drops"] (full) and plan["top_drops"] (preview) — the SAME set it counts as dropped_edges, so the deleted rows are byte-for-byte what the dry-run reports (no drift between a second "junk" definition and the headline number). - _drop_code_tokens() runs a TEMP drop_predicate table (COPY of blocklisted raws) + a single DELETE keyed on coalesce(raw_relation_type, relation_type), catching both never-migrated edges (junk in relation_type) and already- migrated ones (junk preserved in raw_relation_type). Runs WITHOUT re-executing the ~11-min embedding remap plan; idempotent (re-run deletes 0). - Reachable standalone: --drop-code-tokens still hits the DELETE even when there are no remaps (graph already canonical). Same --apply + --i-have-a-backup gating as the remap UPDATE; unlike that UPDATE this is irreversible, hence opt-in behind the backup gate. - Dry-run now prints a TOP CODE-TOKEN DROPS section; --json keeps top_drops but excludes the full drops list (as it already did for remaps). Tests: +7 in test_canonical_migration.py (drop-set capture invariant, DELETE SQL shape + COPY rows, no-op-when-empty, runs-after-remap, standalone-no-remaps, absent-without-flag, and a real-CanonicalMapper integration test asserting the drop set matches the blocklists). 25/25 pass; ruff check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new capability to the canonical migration script, allowing for the permanent deletion of 'junk' predicates from the knowledge graph. These predicates, identified as content-free code tokens, shell commands, or stopwords, are now explicitly removed via a robust, idempotent, and safely-gated database operation. This enhancement improves data quality by eliminating irrelevant edges and ensures consistency with the dry-run reports by deleting exactly what was identified as droppable. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the --drop-code-tokens option in the canonical migration script, enabling the deletion of blocklisted junk-predicate edges (such as shell commands and stopwords) from the database. It introduces the _drop_code_tokens function to perform a targeted set-based DELETE using a temporary table, updates the plan generation and display logic, and adds comprehensive unit and integration tests. The reviewer suggested optimizing the DELETE query by using a LATERAL join to avoid casting and parsing the properties column to JSONB twice per row, which will improve performance on large tables.
| cur.execute( | ||
| """ | ||
| DELETE FROM mempalace_kg."RELATION" e | ||
| USING drop_predicate d | ||
| WHERE COALESCE( | ||
| (e.properties::text::jsonb)->>'raw_relation_type', | ||
| (e.properties::text::jsonb)->>'relation_type') = d.raw | ||
| """ | ||
| ) |
There was a problem hiding this comment.
Evaluating (e.properties::text::jsonb) twice per row in the COALESCE expression can be highly CPU-intensive, especially during a full table scan on a large table like RELATION (e.g., 1.76M rows).
Using a LATERAL join allows PostgreSQL to cast and parse the properties field to jsonb exactly once per row, which can significantly improve the performance of this bulk DELETE operation.
cur.execute(
"""
DELETE FROM mempalace_kg."RELATION" e
USING drop_predicate d,
LATERAL (SELECT e.properties::text::jsonb AS j) p
WHERE COALESCE(p.j->>'raw_relation_type', p.j->>'relation_type') = d.raw
"""
)
What
Replaces the
--drop-code-tokensstub inscripts/canonical_migration.pywith a real targeted set-based DELETE of the code-token / shell-command / stopword RELATION edges — the predicates the canonical mapper drops toNone(viamempalace.kg_predicate_norm:CODE_TOKEN_BLOCKLIST+SHELL_COMMAND_BLOCKLIST+STOPWORD_BLOCKLIST+ the digit/code-look heuristic). These are content-free junk mis-extracted as relations (cd,ls,grep,can,for, …) carrying no entity→entity semantics — deletion, not remap, is the correct disposition. Closes the--drop-code-tokens: NOT YET IMPLEMENTEDfollow-up (#72b).Expected affected rows on prod familiar: ~48,135 edges (335 distinct predicates) — exactly the figure the dry-run already reports as
code tokens (NOT touched unless --drop-code-tokens).DELETE mechanism
Targeted, not a remap-plan piggyback. The drop set is computed once during
build_plan(plan["drops"]) — the same set counted asdropped_edges, so the deleted rows are byte-for-byte what the dry-run reports (no second definition of "junk" that could drift from the headline number)._drop_code_tokens()then:CREATE TEMP TABLE drop_predicate+COPYthe blocklisted raws into it.DELETE ... USING drop_predicate d WHERE coalesce((props)->>'raw_relation_type', (props)->>'relation_type') = d.raw.Keying on the coalesced original predicate catches both never-migrated edges (junk in
relation_type) and already-migrated ones (junk preserved inraw_relation_type). Runs without re-executing the ~11-min embedding remap plan. Idempotent — once deleted, a re-run matches 0 rows.Gating
Same as the remap UPDATE: defaults to DRY-RUN (prints the count + a
TOP CODE-TOKEN DROPSpreview that would be deleted); the actual DELETE requires--applyand--i-have-a-backupand a host-side--dsn/MEMPALACE_POSTGRES_DSN. Both gates verified to refuse (exit 1) without their precondition.Unlike the remap UPDATE (relabel, reversible via
raw_relation_type), this DELETE is irreversible — that's why it stays opt-in behind the backup gate. Reachable standalone:--drop-code-tokensstill hits the DELETE even when there are no remaps (graph already canonical).Tests
+7 in
tests/test_canonical_migration.py(mirrors the existing mocked-psycopg style):sum(drops) == dropped_edges)CanonicalMapper(lexical, no model) integration test asserting the drop set matchesSHELL_COMMAND_BLOCKLIST/STOPWORD_BLOCKLISTand excludes real relations25/25pass;ruff checkclean.🤖 Generated with Claude Code