Match species/approach/technique against assetsSummary instead of per-asset metadata

[bendichter]

Problem

The advanced search operators released in v1.0.0 are slow. Timed against production:

query	time
species:mouse created_after:2026-01-01	14.7s
species:mouse	13.4s
created_after:2026-01-01	0.26s
mouse (free text)	0.63s

All of the cost is in species:, approach:, and technique:. Those matched per-asset metadata in the asset_search materialized view with jsonb_path_exists and a jsonpath built at runtime. A runtime-built jsonpath cannot use any of the view's GIN indexes, so each query sequential-scanned one row per asset and evaluated a regex against every row's metadata blob. The visible_to() embargo check added a correlated subquery per row on top of that. A comment in filters.py had flagged this risk when the operators were written.

Change

Per review feedback this PR is now pared down to the minimal fix for the slow queries, and rebased onto master so it no longer depends on the unmerged operator stack (#2821, #2822, #2827).

species, approach, and technique now match the version-level assetsSummary aggregation, which already rolls up exactly these three fields. The scan moves from one row per asset to one row per dandiset version, three to four orders of magnitude fewer rows. On @yarikoptic-gitmate's production-scale replica this measured 197 ms against the 13.4 s above. No visibility subquery runs on this path: the filter only ever restricts the caller's queryset, list and search responses are built from get_visible_dandisets, and detail routes re-check embargo_status in get_object, which is the same posture as the free-text and date-operator paths.

file_type: is untouched here and still matches per-asset metadata through AssetSearch; it was not one of the slow operators. Its removal in favor of standard: is proposed separately in #2884.

Consequences to be aware of:

• Multiple operators now AND on a single version's summary rather than requiring a single asset to satisfy every clause, so a dandiset whose mouse assets and ephys assets are different files now matches species:mouse approach:electrophysiological. The tests pin the new semantics.
• assetsSummary is recomputed during metadata aggregation rather than on every asset write, so results can lag slightly for a dandiset whose assets changed very recently.
• assetsSummary is aggregated only over assets with status=VALID, whereas asset_search included every asset, so assets that fail validation are invisible to these operators. In the other direction, asset_search excluded Zarr assets (its inner join on AssetBlob skips rows with blob IS NULL), and the summary includes them.

Follow-Ups

The features that were previously part of this PR now have their own PRs, stacked on this branch: #2884 re-adds standard: and removes file_type:, and #2885 adds variable:.

🤖 Generated with Claude Code

[bendichter]

demo on local seed data:

Screen.Recording.2026-08-05.at.3.33.30.PM.mov

[yarikoptic-gitmate]

I ran this on a local instance seeded with the version metadata of 220 real dandisets pulled from production, and drove it through both the API and the browser. The core change is correct and the performance argument holds — approve from me.

Details below; nothing blocking.

Performance, measured

You noted latency wasn't re-measured. I measured it. The plan is a seq scan on api_version; I cloned the table to 10,304 rows to approximate production scale:

Seq Scan on ver_scale (actual rows=3744 loops=1)
  Filter: jsonb_path_exists(metadata, '$.assetsSummary.species[*].name ? (@ like_regex "mouse" flag "i")')
  Rows Removed by Filter: 6560
Execution Time: 196.773 ms

197 ms versus the 13.4 s you measured for the old path. Version metadata averages 2.5 KB (max 9.5 KB), so there's no TOAST detoasting cost hiding in there. The row-count argument is sound.

It's linear in version count, though — if published versions grow an order of magnitude this comes back. The follow-up would be a generated text column plus a trgm GIN index, roughly what the comment you removed suggested.

On dropping the user parameter

Agreed it's safe, but I'd correct the reasoning slightly, because the stated justification doesn't cover every path.

get_queryset() returns get_visible_dandisets only for list and search; detail routes get bare Dandiset.objects (dandiset.py:191). DRF runs filter backends inside get_object() (generics.py:87), so on retrieve/users/uploads/star these filters do run against a queryset containing embargoed dandisets. Confirmed against an embargoed dandiset whose assetsSummary I seeded with mouse/LFP/NWB, anonymous:

GET /api/dandisets/<embargoed>/?search=species:musculus      -> 401
GET /api/dandisets/<embargoed>/?search=species:zzzznomatch   -> 404

That's a substring oracle over embargoed assetsSummary. It doesn't expose anything that wasn't already exposed, though, and I don't think it should hold the PR up — the same oracle already exists on master through two paths you didn't touch:

?search=Secret                     -> 401     free text, dandiset.py:170-177,
?search=zzzznomatch                -> 404       Version.objects with no visibility filter at all
?search=created_after:2000-01-01   -> 401     date operators, filter the Dandiset qs directly
?search=created_after:2099-01-01   -> 404

The free-text path casts the whole metadata blob to text, so it already probes assetsSummary and strictly more besides. And no data escapes either way — dandiset.py:277 re-checks embargo_status before returning the object. So dropping user= widens nothing.

The cleanest support for the change is further down the same file: DandisetViewSet.search has been doing AssetSearch.objects.all() intersected with get_visible_dandisets (dandiset.py:376-380) all along. This PR just adopts the pattern the search endpoint already used.

Worth swapping that in, because the current wording isn't only in the PR description — it's in the source at filters.py:142-144:

# ... and the view's base queryset is already limited to dandisets visible to the user, so embargoed dandisets can't surface.

That comment will outlive the PR description, and it's the one someone will cite in a year before adding a sixth operator or reusing apply_search_filters somewhere new.

Suggestions

1. _jsonpath_match's new column parameter. It's only ever called with the literal 'metadata' (filters.py:164). Master had the column hardcoded; making it an f-string argument (filters.py:82) turns a fixed piece of raw SQL into a second interpolation point protected only by a docstring convention. The path argument has to stay dynamic, but the column doesn't — I'd drop the parameter.

2. Recall changes worth knowing about, in both directions. assetsSummary is aggregated only over status=VALID assets (services/metadata/__init__.py:99), whereas asset_search included every asset. Assets that fail validation are now invisible to these operators. Going the other way, asset_search INNER JOINs AssetBlob (search/migrations/0002:24-25), so Zarr assets (blob IS NULL) were never in it and now are. Probably a net win, but neither the tests nor my instance can surface it, since both seed assetsSummary directly. Might deserve a line in the description next to the staleness caveat you already flagged.

3. Operator lists (pre-existing, and you've improved it). Master carried _DATE_OPS/_ASSET_OPS/_NAME_PATH_OPS/_FILE_TYPE_ALIASES; this consolidates to _DATE_OPS/_SUMMARY_OPS/_SUMMARY_PATHS, alongside OPERATOR_KEYS and operatorHelp. The remaining sharp edge, also pre-existing: apply_search_filters is if key in _DATE_OPS / elif key in _SUMMARY_OPS with no else (filters.py:153-168), so a key added to OPERATOR_KEYS but not the filter sets is silently ignored — unfiltered results rather than a 400. Since _SUMMARY_OPS has exactly one use site, deleting it and testing key in _SUMMARY_PATHS removes one list for free. A test asserting OPERATOR_KEYS == _DATE_OPS | frozenset(_SUMMARY_PATHS) would pin three of the four; operatorHelp is TS and stays manual, and the e2e toHaveCount(11) is a hardcoded literal rather than derived from it.

4. file_type: removal has no fallback message. Verified there's no near-miss suggestion — get_close_matches('file_type', OPERATOR_KEYS, cutoff=0.6) returns [], best ratio is modified_after at 0.522 — so the user gets a dead end:

Unknown search operator "file_type". Wrap the term in double quotes (e.g. "foo:bar") to search for it as text.

To be clear, the removal itself looks right and not really optional: AssetsSummary has no encodingFormat or MIME field at all, so keeping file_type: would have meant keeping the AssetSearch dependency and the scan you're removing. And the faceted sidebar parameter still covers the use case (I checked, it's untouched). The exposure window is small too, since this shipped in v1.0.0 yesterday. It's just that a one-line special case pointing at standard: and the sidebar would land better than a generic unknown-operator error, and a test pinning the 400 would be cheap.

5. Nits. The suggestion dropdown is max-height: 320px against a scrollHeight of 536px, and onKeydown doesn't scroll the highlighted item into view — arrowing to variable: marks it active while the card stays at scrollTop: 0. Pre-existing (file_type: sat below the fold too) and mostly theoretical, since typing v or stan filters the list to one entry, but the two entries you're adding are the two at the bottom. Needs a ref on the v-list-item v-for plus scrollIntoView({ block: 'nearest' }), not just a watcher. Separately, test_search_parser.py has no standard:/variable: cases.

What else I checked (all clean)

• All 22 advanced_search tests pass locally.
• Operators against real production metadata: species:mouse (67), approach:electrophysiological (91), technique:"spike sorting" (61), standard:nwb (137), standard:bids (8), variable:LFP (22). variable:LFP returns exactly the right dandisets — sharp-wave ripple, Neuropixels, "Local Field Potential Recordings…".
• Injection: species:'); DROP TABLE api_version; --, species:.*, species:%, species:\, species:[pilot] all return 0 rows, table intact. The re.escape + parameterization holds.
• Malformed assetsSummary (null, string, int, object-instead-of-array, non-string name, null array element, missing key) inserted via raw SQL: all queries 200, no 500s. Lax jsonpath mode is tolerant, so a single bad legacy row can't break search.
• variableMeasured being bare strings is guaranteed by dandischema (Optional[List[str]]), and a 100-dandiset production sample across schema 0.4.4/0.6.0/0.6.4 found no rows using the older PropertyValue shape, so the variable: path won't silently miss historical versions.

---

Generated by Claude Code

[yarikoptic-gitmate]

Followed up on suggestions 1–3 from my review. I intended to send these as a PR against this branch, but the automation account I'm running as has read + review permissions only — no branch, push, or fork access to this repo — so here are the patches inline instead. All three are rebased onto the current head (884d1e9), not the head I originally reviewed.

Verified locally: test_search_parser.py 20 passed, test_dandiset.py -k advanced_search 32 passed.

1. Drop the column parameter from _jsonpath_match

Only ever called with the literal 'metadata', and _contributor_where / _affiliation_where right below it already hardcode the column. As written it turns fixed raw SQL into a second interpolation point guarded only by a docstring.

-def _jsonpath_match(column: str, path: str, value: str) -> tuple[str, list[str]]:
-    """Build a parameterized Postgres `jsonb_path_exists` predicate.
+def _jsonpath_match(path: str, value: str) -> tuple[str, list[str]]:
+    """Build a parameterized Postgres `jsonb_path_exists` predicate on `metadata`.
 
     Matches `value` case-insensitively as a substring against any node
-    selected by `path`. `column` and `path` MUST come from trusted
-    constants; `value` is parameterized and regex-escaped.
+    selected by `path`. `path` MUST come from a trusted allowlist; `value`
+    is parameterized and regex-escaped.
     """
     where = (
-        f'jsonb_path_exists({column}, '
+        'jsonb_path_exists(metadata, '

-            where, params = _jsonpath_match('metadata', SUMMARY_PATHS[key], value)
+            where, params = _jsonpath_match(SUMMARY_PATHS[key], value)

2. Derive SUMMARY_OPS from SUMMARY_PATHS

It currently repeats that dict's keys verbatim in operators.py. Move the definition below the table and derive it:

-SUMMARY_OPS = frozenset({'species', 'approach', 'technique', 'standard', 'variable'})
-
 OWNER_OPS = frozenset({'owner'})

 SUMMARY_PATHS = {
     ...
 }
 
+# Derived from SUMMARY_PATHS rather than repeated, so the dispatch key set and
+# the jsonpath table can't drift apart.
+SUMMARY_OPS = frozenset(SUMMARY_PATHS)

3. Guard the dispatch against silent fall-through

Making OPERATOR_KEYS a derived union was the right call — the parser can no longer drift. But apply_search_filters still dispatches through six if key in <FAMILY> branches with no else, so an operator added to a family table but to no dispatch branch parses cleanly and is then silently dropped, returning an unfiltered queryset rather than a 400. This asserts each key measurably narrows the query:

@pytest.mark.django_db
def test_every_operator_key_actually_filters():
    """Every key the parser accepts must be handled by `apply_search_filters`."""
    base = Dandiset.objects.all()
    unfiltered_sql = str(base.query)

    for key in sorted(OPERATOR_KEYS):
        if key in DATE_OPS:
            value = '2024-01-01'
        elif key in COUNT_OPS:
            value = '10'
        else:
            value = 'x'
        filtered = apply_search_filters(base, parse_search(f'{key}:{value}'))
        assert str(filtered.query) != unfiltered_sql, (
            f'Operator {key!r} is in OPERATOR_KEYS but did not affect the '
            'queryset — apply_search_filters has no branch handling it, so it '
            'is silently ignored.'
        )

I mutation-tested it: adding a bogus_drift key to OPERATOR_KEYS alone fails with Operator 'bogus_drift' is in OPERATOR_KEYS but did not affect the queryset.

Also worth folding in

The visibility comment in apply_search_filters states something that isn't true, and unlike the PR description it will outlive this PR:

# ... and the view's base queryset is already limited to dandisets visible to the user, so embargoed dandisets can't surface.

get_queryset only returns get_visible_dandisets for the list and search actions, and DRF runs filter backends inside get_object(), so on detail routes these filters do run over embargoed rows. It's safe — get_object re-checks embargo_status, and the free-text and date-operator paths already behave the same way — but the stated reason is wrong, and it's the sentence someone will cite when adding operator number seven.

Unrelated heads-up

ruff check dandiapi/api/services/search/ fails on the current branch head with PLR0912 Too many branches (13 > 12) on apply_search_filters — it carries a # noqa: C901 but not PLR0912. Present before my changes; flagging in case the stacked merges hid it from a lint run.

---

Generated by Claude Code

[bendichter]

Per Yarik's request I have pared this PR down to the minimal fix for the slow queries and moved everything else out. The branch is rewritten on top of master, so it no longer sits on the advanced-search stack, and it now contains a single commit repointing species:, approach:, and technique: at assetsSummary. The standard: re-add and the file_type: removal moved to #2884, and the variable: operator moved to #2885, both stacked on this branch.

Of the review follow-ups: the corrected visibility reasoning is folded into the comment on _apply_summary_filters here. The column parameter on _jsonpath_match and the derived SUMMARY_OPS set are moot in the pared-down version, since the column stays hardcoded and the dispatch tests membership in _SUMMARY_PATH_OPS directly. The dispatch drift-guard test makes more sense once the operator families multiply, so I would rather land it with the stack (#2821, #2822, #2827) than here.

🤖 Generated with Claude Code

[yarikoptic]

I approve as I was told it to solve a real performance problem. But review pointed again to IMHO pretty much reducing SNR in our code with all that unverified or unnecessary documentation and hypothetical comments.

[yarikoptic]

IMHO this is an example on where AI makes code less readable with all kinds of either duplicated (here in docstring of a short 5 lines function; better if just brief to the point inline commends like .all() # AND chaining of queries)) or hypothetical (here is this # NOTE perf which might or not be true, since nobody really confirms it or assessed .. I do not even know what GIN index is which was added as a term in ca7f283 .

I think we need to improve our instructions to agents to be more concise and not add hypotheticals....

[bendichter]

Fair point, and agreed that this kind of context belongs in the PR thread rather than in the code. 11e9901 trims the file back down: the docstrings are one to three lines, the perf note is gone, and what remains in the code is only what a future editor needs to avoid breaking it (the jsonpaths must stay trusted constants, the user value is parameterized and regex-escaped, and the metadata column has to stay unqualified).

Recording the removed context here instead, since it is review material rather than code:

• The perf claim was measured rather than hypothetical: on a clone of api_version at production scale (10,304 rows), species:mouse runs in 197 ms on this branch versus 13.4 s for the old per-asset path. The scan is linear in version count, so if published versions grow by an order of magnitude the follow-up would be a generated text column per field with a trigram index. (GIN is Postgres's index type for jsonb columns; the point of the removed note was that this query shape cannot use one.)
• Visibility: this filter only ever restricts the caller's queryset. List and search responses are built from get_visible_dandisets, and detail routes re-check embargo_status in get_object, which is the same posture as the free-text and date-operator paths.
• Semantics: clauses are AND'd on a single Version row, so species:mouse approach:ephys matches a dandiset whose summary lists both even when the mouse assets and the ephys assets are different files. The tests pin this.

On the broader point, I have updated my agent instructions to keep code comments to constraints the code cannot show and to put discussion like the above in the PR instead.