fix(python): route path-synthetic module batches through the shared query - #6
Open
sontek wants to merge 8 commits into
Open
fix(python): route path-synthetic module batches through the shared query#6sontek wants to merge 8 commits into
sontek wants to merge 8 commits into
Conversation
…et query has_path_synthetic_module_units() returning true for Python forced set_exact_definition_values, set_normalized_definition_values, and set_structural_member_values to bail on every batch, regardless of size, sending each request through relational_definition_values's one-request-at-a-time point-query fallback instead. That fallback exists for small batches; Python asks about thousands of names at once and got routed there anyway (bifrost issue #20). Both set-query functions that can answer with a path-synthetic module now also query the same wide view's path arm that the point-query path already reads through path_units, merging the result in before dedup. set_structural_member_values only returns non-module members, so it drops the bail with no matching query added. Adds a regression test proving a full-size Python exact-name batch resolves entirely through the batched path (zero point queries) including a path-synthetic module name in the same batch, and a round-trip-count benchmark test documenting why the production regression is disk- and concurrency-bound rather than visible on a small in-cache SQLite fixture.
…e a lean view for it review-code found that set_structural_member_values dropped the || has_path_synthetic_module_units() bail without adding the batched_path_units_sql merge its two siblings got, silently losing path-synthetic submodules as structural members of their package once a batch crosses SET_QUERY_MIN_REQUESTS. AnalyzerDefinitionLookup queries a package's own relational name as a StructuralMembers owner specifically to find its submodules, so this was reachable in production. perf-reviewer found the merge itself (in all three functions) queried the wide live_definition_exact_names/live_definition_normalized_names views. Those views are a three-way UNION ALL; SQLite materializes all three arms before a `source_kind = 'path'` filter can discard the other two, the same compound-view tax this codebase already measured at 89.4 minutes on an 802K-row table elsewhere. All three call sites now read the lean, path-only workspace_path_symbol_exact_names / workspace_path_symbol_normalized_names views instead, which never join code_units or the anchor tables. Adds a regression test proving the submodule now resolves inside the batched path, and an EXPLAIN QUERY PLAN pin proving the path-units query never touches code_units or workspace_file_anchor_rows.
…param code-simplifier found set_exact_definition_values and set_structural_member_values building the byte-identical batched_path_units_sql call; factored into batched_exact_name_path_units_sql, reused by both call sites and the EXPLAIN QUERY PLAN pin's own construction of the same query. Also drops the now-unused adapter parameter (and its generic bound) from set_queries_need_live_unit_counts, left over from removing its has_path_synthetic_module_units() early return, and switches the two new tests off a hardcoded 64 onto the real SET_QUERY_MIN_REQUESTS constant (re-exported test-only) so they can't drift from it silently.
code-simplifier caught two doc comments crediting the wide-view materialization cost to query_view_candidates's doc comment. The real measurement is issue #2794's mounted_declaration_scan_seeks_live_workspace_files in store/mod.rs, a different caller of the same wide view.
…short_name) Ran a live single-case check of this branch against PostHog, a real large mixed-language monorepo. AnalyzerDefinitionLookup::prefetch_fqn_in_language went through the new query_batched_path_units path as expected, then stayed there: sampling the process twice, 13 minutes apart, caught it in the exact same call both times. workspace_file_path_symbol_rows only indexes (exact_fqn) and (normalized_fqn); there is no index on (package_name, short_name). The exact-name and structural-member path-arm queries matched on that unindexed pair, so SQLite had to build its own covering index over every path-symbol row in the whole workspace, every language, before answering a single request. The normalized-name query already matched on the indexed normalized_fqn and was never affected. Both queries now serialize the full exact_fqn to match at request key index 3 and use it directly: set_exact_definition_values already has the request's own rendered tail (equal to its exact_fqn, since a path-arm match requires an empty prefix); set_structural_member_values builds the member's exact_fqn from its owner's tail and the member's identifier, joined with the language's own package separator. Extends the EXPLAIN QUERY PLAN regression pin to assert the seek instead of only the absence of code_units/anchor references, which said nothing about whether the path-arm predicate itself was indexed.
Live-tested the branch against a real large mixed-language monorepo (the same kind of repository this issue's own evidence came from) and it was still stuck after 45+ minutes. Sampling the process found it inside path_units, not the code this issue's fix already touched: below SET_QUERY_MIN_REQUESTS, or for any query shape with no set executor at all, definition_values still falls back to path_units on the wide view for the path-derived answer -- unconditionally, on every single such call. path_arm_lean_units gives that point-query fallback the same exact_fqn/normalized_fqn-indexed route the batched functions already have, for ExactName, NormalizedName, StructuralMembers, and VisibleMembers -- the shapes that carry (or can construct) a single full name. StructuralChildren, Identifier, IdentifierPrefix, PackageTypes, and PackageTypesInPackage only carry a bare identifier or a package name; workspace_file_path_symbol_rows has no index on either, so those need a real new index, not a smarter predicate -- they keep using path_units on the wide view unchanged. Adds a point-query correctness test and an EXPLAIN QUERY PLAN pin mirroring the batched-path ones already in this file.
…ean too AnalyzerDefinitionLookup::prefetch_fqn_in_language batches every exact-name lookup into one call, then batches its identifier-candidate fallback for every miss into one more -- both real, single round trips, not the per-name-call problem this issue started from. But RelationalDefinitionQuery Identifier has no batched set executor at all, by original design: it always takes definition_values's per-request point-query path, no matter how large the Rust-level batch handed to the store is. Its content-side lookup already has a lean-view treatment from a prior fix; its path-arm lookup did not, and workspace_file_path_symbol_rows had no index to give it one -- only (exact_fqn) and (normalized_fqn), from migrations built for shapes that carry a full name. A miss-heavy fallback on a real large Python monorepo turned that into path_units's wide-view scan once per identifier miss. Migration 0037 adds an index on (short_name, file_version_id). path_arm_lean_units now serves Identifier's two shapes (with or without a file) and IdentifierPrefix's range form from it. StructuralChildren, PackageTypes, and PackageTypesInPackage only carry a package name, which this index does not cover, and keep using the wide view unchanged.
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
1 participant
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.
Python's relational-lookup batches for exact-name, normalized-name, and structural-member queries always fell through to the one-request-at-a-time point-query path, no matter how large the batch.
has_path_synthetic_module_units()returning true for Python made all three batched set-query functions bail unconditionally, so a whole-workspace fan-out asking about thousands of names took thousands of sequential SQLite round trips instead of one shared query. On a real large repository this showed up as a worker thread pinned inside that point-query path for an entire 30-second profile sample while every other worker sat idle waiting on the same connection pool.The three batched functions now merge in the path-synthetic-module answer themselves, reading a lean, path-only view rather than the wide compound view the point-query path uses, so a full batch resolves in one query instead of forcing a bail. That view choice matters: the wide view is a three-way UNION ALL, and joining it here would make SQLite materialize all three arms before the path-only filter could discard the other two, the same compound-view cost this codebase had already measured elsewhere.
Includes two follow-up commits from review: a fix for a path-synthetic module silently dropping out of structural-member lookups, and a cleanup that deduplicates identical SQL construction the fix introduced and drops a parameter left dead by the change.
Refs BrokkAi/bifrost#20