[jonathan-bird]

Description

This PR optimises the IteratorBuilder to avoid loading all items when a limit() is applied without orderBy() or inRandomOrder(). This significantly improves search query performance, particularly for sites with large datasets.

Fixes #13215

Problem

Previously, IteratorBuilder::getFilteredItems() loaded ALL items before applying limits. For a ->limit(10) query on 10,000 results, it would hydrate all 10,000 items before taking the first 10.

Solution

Added a new abstract method getBaseItemsLazy(): Generator that yields items lazily, enabling early termination when the limit is reached.

Three optimisation paths in getFilteredItems():

• No limit, orderBy, or randomise: Falls back to loading all items (unchanged behaviour)
• Limit without wheres: Loads only offset + limit items
• Limit with wheres: Batches items and stops early when enough matches are collected

Benchmark Results

I wrote a simple benchmark script separately to test the difference in performance between current code and new code for various scenarios (eg wheres, where + limit, limit etc.). I skipped orderBy as this remains the same, but I did test just in case and it's the same.

Scenario	Limit	Old Time	Old Hydrated	New Time	New Hydrated	Improvement
No wheres	10	0.70ms	10,000	0.04ms	10	99.9% fewer
No wheres	50	0.68ms	10,000	0.05ms	50	99.5% fewer
No wheres	100	0.68ms	10,000	0.06ms	100	99% fewer
50% match rate	10	9.13ms	10,000	0.10ms	50	99.5% fewer
50% match rate	50	9.14ms	10,000	0.15ms	100	99% fewer
50% match rate	100	9.12ms	10,000	0.26ms	200	98% fewer
10% match rate	10	9.01ms	10,000	0.18ms	100	99% fewer
10% match rate	50	8.99ms	10,000	0.57ms	500	95% fewer
10% match rate	100	8.98ms	10,000	1.08ms	1,000	90% fewer

Safety checks

• Queries with orderBy() or inRandomOrder() load all items (sorting/shuffling requires all)
• Queries without limit() load all items (no early termination possible)
• Results are identical to previous behaviour, just faster

Files Changed

• src/Query/IteratorBuilder.php - Core optimisation logic
• src/Query/ItemQueryBuilder.php - Implements getBaseItemsLazy()
• src/Search/QueryBuilder.php - Implements getBaseItemsLazy() with batch hydration

Tests

• tests/Query/IteratorBuilderTest.php - 13 tests covering optimisation paths
• tests/Search/QueryBuilderPerformanceTest.php - 14 tests for search-specific behaviour
• tests/Fakes/Query/TestIteratorBuilder.php - Test helper
• tests/Fakes/Query/HydrationTrackingQueryBuilder.php - Test helper

Note: These tests all pass, it's just the UTF-8 which Duncan is fixing in another PR that are failing.

Potential Future Optimisation

This PR optimises hydration by stopping early once the limit is reached. However, the search drivers still fetch all raw results from the index before we apply the limit.

A possible future optimisation could pass the limit down to getSearchResults($query, $limit = null) so drivers can fetch fewer results at the source:

• Algolia: Could use the hitsPerPage API parameter to request fewer hits
• Comb: Could limit raw results before mapping scores/snippets

This would be most beneficial for large indexes where the initial lookup is expensive.

In my testing, this PR still reduces my search from 3.5s down to 600ms using Comb, and with this future optimisation would reduce down to about 400ms.

[vitalijalbu]

any news about this task guys?
@jonathan-bird

[jonathan-bird]

any news about this task guys?

@jonathan-bird

@jasonvarga would be the man to ask. I imagine they're very busy with the v6 launch.

Hopefully this can merge in soon - I wonder whether it could merge into v6 instead.

[vitalijalbu]

any news about this task guys?
@jonathan-bird

@jasonvarga would be the man to ask. I imagine they're very busy with the v6 launch.

Hopefully this can merge in soon - I wonder whether it could merge into v6 instead.

I wish it will be on v5 as well... i count 10k+ entries at the moment 🥲

[jonathan-bird]

@jasonvarga @duncanmcclean Is this still on the cards or should I close it? It's a significant performance improvement and I personally had to change my client Statamic to use my code so it's been running live since this issue.

[duncanmcclean]

Keep it open! We're always reviewing and merging PRs, but there's a lot of them for us to work through. We'll get to this PR when we can. 🙂

I assume you already are, but if you need to use this fix in the meantime, consider applying this PR as a composer patch.

[jasonvarga]

Thanks for this, and sorry it's taken so long to get to. The underlying problem in #13215 is real, the diagnosis is right, and the benchmarks are convincing — hydrating every search result before applying the limit is exactly the sort of thing we want to fix.

I've merged 6.x into your branch and pushed it so CI can run against something current (it was ~10 months behind, and had been retargeted from 5.x to 6.x in the meantime). The one conflict was in tests/Search/QueryBuilderTest.php, where your new limit tests landed next to results_are_found_using_where_status from #14387 — I kept both.

With that done I ran the full suite locally, and there are a few things that need fixing before this can land. Details are inline, but the headlines:

1. The optimised paths return a plain Illuminate\Support\Collection instead of a DataCollection, which fatals any paginated IteratorBuilder query. tests/Tags/Dictionary/DictionaryTagTest::it_can_paginate errors with Method Illuminate\Support\Collection::supplement does not exist.
2. paginate()->total() under-reports when a limit() is already set on the builder.
3. The new getBaseItemsLazy() is abstract, which is a breaking change for any addon extending IteratorBuilder.

On the red CI: the original run's logs have expired, so I checked out your pre-merge head f6343c4 with its own dependencies and reproduced the failure directly — it's the dictionary pagination error in (1), not the UTF-8 charset issue from #13213 that the description mentions. That fix was already an ancestor of your branch when CI ran, so it was never the cause. Good news is it's a small fix.

A couple of smaller notes that aren't blocking: $batchSize = max(50, $this->limit * 2) is unbounded, so a very large limit collapses the batching back to loading everything, and $i is unused in the withoutData loop in Search\QueryBuilder::getBaseItemsLazy().

[jasonvarga]

This needs to be $this->collect() rather than collect() — same on line 79 in getFilteredItemsInBatches().

Every pre-existing path through getFilteredItems() returns a Statamic\Data\DataCollection, either from getBaseItems() or via Search\QueryBuilder::collect(). These two new paths return a plain Illuminate\Support\Collection, so the moment a limit() is set, get() silently changes return type and loses supplement(), actions(), toArrayWith() and preProcessForIndex().

That breaks core today. Builder::paginate() calls forPage(), which sets limit/offset, so any paginated IteratorBuilder query now takes an optimised path — and OutputsItems::paginatedOutput() immediately calls ->supplement('total_results', ...) on the result:

Tests\Tags\Dictionary\DictionaryTagTest::it_can_paginate
Error: Method Illuminate\Support\Collection::supplement does not exist.
  src/Tags/Concerns/OutputsItems.php:60
  src/Tags/Dictionary/Dictionary.php:60

This passes on 6.x and fails here. Statamic\Search\Tags uses the same trait, so {{ search:results paginate="..." }} hits the identical path — we just have no test covering it.

[jasonvarga]

getCountForPagination() (just above, line 18) also calls getFilteredItems(), and Builder::paginate() uses it for the total record count. Previously that always returned every match regardless of any limit, so the total was accurate. Now, if a limit() is already set on the builder, this returns an early-terminated set capped at offset + limit, and the total silently under-reports.

Reproduced with 1000 items, 333 of them matching:

(new ItemQueryBuilder)->withItems($items)
    ->where('type', 'b')
    ->limit(10)
    ->paginate(10)
    ->total(); // 16, should be 333

That gives a wrong last_page and drops results from later pages, with no error to indicate anything went wrong. getCountForPagination() should ignore limit/offset when counting, so it keeps its previous semantics.

[jasonvarga]

This can't be abstract. IteratorBuilder is a public abstract class, and 6.x is a shipping minor line (v6.32.0 is out), so adding an abstract method fatals any addon that extends it as soon as they upgrade.

Giving it a concrete default keeps it backwards compatible, and subclasses can still override it to get the lazy behaviour:

protected function getBaseItemsLazy(): Generator
{
    foreach ($this->getBaseItems() as $item) {
        yield $item;
    }
}

That would also let you drop the ItemQueryBuilder implementation entirely — withItems() takes an already-materialised Collection, so iterating it lazily saves no work there. The real win in this PR is the search hydration batching.

[jasonvarga]

transformResults() ends with ->sortByDesc->getScore(), so calling it per chunk sorts each chunk independently. Global relevance order only survives here because both shipped drivers already return raw results pre-sorted by score (Comb's usort in Comb.php, and Algolia setting search_score = count - $i).

That's fine today, but it's an invariant a custom driver has no way of knowing about — one that doesn't pre-sort would return results that are locally ordered within each chunk and globally wrong. Worth either sorting once after collection, or documenting the requirement on getSearchResults().

[jasonvarga]

This override (and FakeQueryBuilder in QueryBuilderTest.php) replaces getBaseItemsLazy() wholesale with a fake that yields in input order, so the real Search\QueryBuilder::getBaseItemsLazy() — the chunking plus the per-chunk transformResults() re-sort — never runs under test.

it_respects_search_score_ordering_after_optimization passes because the fixture's insertion order already matches score order, not because the batching preserved anything. It'd be better to fake getSearchResults() only and let the real lazy method run, which is also what would have caught the two bugs above.