diff --git a/llm/cli.py b/llm/cli.py index 0234fdd0c..4d460b9fd 100644 --- a/llm/cli.py +++ b/llm/cli.py @@ -1415,12 +1415,15 @@ def load_conversation( migrate(db) if conversation_id is None: # Most recent conversation from either generation of tables - - # thread ids are conversation ids, so the union dedupes rows - # from the dual-write era. + # thread ids are conversation ids, so a dual-write era id can + # show up on both sides, but the outer limit 1 makes that + # harmless. "union all" lets SQLite walk each primary key + # backwards and stop at the first row, where a distinct + # "union" would first materialize every id from both tables. matches = list(db.query(""" select id from ( select id from threads - union + union all select id from conversations ) order by id desc limit 1 """)) diff --git a/llm/logs.py b/llm/logs.py index 138d6334a..3db92470b 100644 --- a/llm/logs.py +++ b/llm/logs.py @@ -1119,7 +1119,10 @@ def _needs_model_replacements(condensed: str) -> bool: # with a newline between each, so the words either side of a part # boundary stay separate tokens. Each group_concat reads from an # ordered subquery because that is what fixes its order: an order by -# alongside the aggregate would not. +# alongside the aggregate would not. The response side cross joins from +# output_messages: SQLite cannot tell the recursive chain is short, and +# with plain joins it puts parts in the outer loop and scans the whole +# table for every logged turn. TURN_SEARCH_INSERT_SQL = """ with recursive input_messages(hash, parent_hash, role, depth) as ( select messages.hash, messages.parent_hash, messages.role, 0 @@ -1161,8 +1164,8 @@ def _needs_model_replacements(condensed: str) -> bool: select group_concat(text, char(10)) as text from ( select nullif({LITERAL}, '') as text from output_messages om - join messages on messages.hash = om.hash and messages.role = 'assistant' - join parts on parts.message_hash = om.hash + cross join messages on messages.hash = om.hash and messages.role = 'assistant' + cross join parts on parts.message_hash = om.hash where parts.type = 'text' order by om.depth desc, parts.position ) diff --git a/llm/migrations.py b/llm/migrations.py index bc26a5759..ed84df841 100644 --- a/llm/migrations.py +++ b/llm/migrations.py @@ -732,3 +732,46 @@ def m027_turns_response_json(db): # response. NULL for turns logged before this column existed and # for models that expose no raw payload. db["turns"].add_column("response_json", str) + + +# FORK-LOCAL migration (rs) - note for future rebases onto upstream: +# a same-numbered upstream m028_* does NOT clash (migrations are +# recorded by function name, and these indexes depend only on tables +# from m010/m017/m023, so ordering against future upstream migrations +# is irrelevant). The clash to watch for is upstream shipping +# equivalent indexes: sqlite-utils auto-generates the same +# idx__ names, so if_not_exists below keeps this +# migration a no-op when upstream's version ran first. If upstream's +# equivalent lacks that guard and would run AFTER this one, drop this +# migration during the rebase instead. +@migration +def m028_log_lookup_indexes(db): + # Indexes for the columns `llm logs` looks rows up by but + # previously had to scan for. + # The legacy tool tables are only ever read through correlated + # subqueries on response_id - without an index that is one full + # scan per listed response, and tool_results rows carry the actual + # tool outputs, so those scans read the payloads too. + # tool_responses' primary key starts with tool_id, so it cannot + # serve a response_id probe either. + db["tool_calls"].create_index(["response_id"], if_not_exists=True) + db["tool_results"].create_index(["response_id"], if_not_exists=True) + db["tool_responses"].create_index(["response_id"], if_not_exists=True) + # -c/--cid filtering on the legacy side was a full responses scan: + # turns got idx_turns_thread_id in m023, but the equivalent + # responses.conversation_id was never indexed. + db["responses"].create_index(["conversation_id"], if_not_exists=True) + # The -T/--tool filter resolves "which messages carry a tool_result + # part (with this tool name)" - previously a full scan of parts, + # the fastest-growing table in the store, plus a transient + # automatic index sqlite built on every query. Trailing + # message_hash makes the index covering for exactly that subquery. + # Partial on purpose: without sqlite_stat1, an index that leads with + # type gets picked for every "type = ?" lookup, which then reads all + # parts of that type instead of one message's parts through + # idx_parts_message_hash_position. Only queries that spell out + # type = 'tool_result' can use this one. + db.execute( + "create index if not exists idx_parts_tool_result_tool_name" + " on parts (tool_name, message_hash) where type = 'tool_result'" + ) diff --git a/tests/test_migrate.py b/tests/test_migrate.py index 8e467116d..abb7b7f6b 100644 --- a/tests/test_migrate.py +++ b/tests/test_migrate.py @@ -56,6 +56,31 @@ def test_migrate_blank(db_factory): } +def test_migrate_creates_log_lookup_indexes(): + """m028: the columns `llm logs` filters and joins on are indexed, + so listings never scan the tool, parts or responses tables.""" + db = sqlite_utils.Database(memory=True) + migrate(db) + indexed = { + (table.name, tuple(index.columns)) + for table in db.tables + for index in table.indexes + } + for expected in ( + ("tool_calls", ("response_id",)), + ("tool_results", ("response_id",)), + ("tool_responses", ("response_id",)), + ("responses", ("conversation_id",)), + ("parts", ("tool_name", "message_hash")), + ): + assert expected in indexed + # Partial, so lookups that bind type as a parameter cannot pick it + sql = db.execute( + "select sql from sqlite_master where name = 'idx_parts_tool_result_tool_name'" + ).fetchone()[0] + assert sql.endswith("where type = 'tool_result'") + + @pytest.mark.parametrize("has_record", [True, False]) def test_migrate_from_original_schema(db_factory, has_record): db = db_factory(memory=True)