Resolve a search page's argMax for its own keys, not every row past the cursor - #139
Merged
Merged
Conversation
…he cursor search() ran one GROUP BY ... ORDER BY ... LIMIT over the raw capture table, so ClickHouse built the full 27-column argMax resolution tuple for every group past the cursor before LIMIT kept limit + 1 of them. The keyset comparison `(tenant_id, experiment_id, run_id, captured_at_ns, capture_id) > (...)` is not usable by the primary-key index (EXPLAIN: 26/26 granules), so a 38-row page over a 198k-row catalog read all 198k rows and aggregated all of them: ~150 ms per page whatever the page size. Filtering alone was 22 ms and grouping the five key columns 37 ms; the argMax tuple was the other three quarters. The page's keys are now chosen first, by an inner query that groups only the key columns under the same filters and LIMIT, and the argMax is resolved for those keys alone. Both queries carry every filter, so the groups and each group's resolution are unchanged. Python and native readers take the same shape. Measured on a 204k-capture catalog, paging through one 196,608-capture run: limit 10000: 9.12s -> 6.51s (1.4x) limit 2000: 23.92s -> 10.56s (2.3x) limit 500: 87.29s -> 26.68s (3.3x) with identical ids in identical order at every page size. Not removed: each page still scans the rows past the cursor, because the keyset tuple remains invisible to the index; that floor (~68 ms/page here) needs index-usable cursor bounds, a separate change. The reader suites -- cpu, live reader, snapshot, end-to-end and native/Python reader parity -- pass 155/155 (154 before, plus the new guard), with the conformance drivers rebuilt so no parity case skipped.
The design doc described search as one argMax over the non-grouped columns. That is still the resolution, but a page now computes it only for the keys an inner, key-only query selects under the same filters and LIMIT. This records why (the per-page cost measured before), why it is safe (the same immutability rule that already justifies the pre-aggregation filters), and what it leaves: each page still scans the rows past the cursor.
The Python storage capture path is being removed, so the Python half of this change is dropped: clickhouse_reader.py and its cpu shape test are back to main, and only the native reader keeps the two-phase page query. That also makes the guard stronger, not weaker. The new parity test walks the native reader in pages of 3 across a superseded version -- four captures re-described from a different pack at a later index_version, the case where argMax resolution decides the answer -- and compares every row with the Python reader, which still uses the single-phase shape. So the suite now checks the new query against the old one rather than against itself. It then reads the queries ClickHouse actually received from the native driver (HTTP, interface 2) out of system.query_log and requires the key-tuple subquery on each. Red first: with reader.cpp back to main the test fails on the missing `(keys) IN (SELECT keys FROM` subquery. An earlier draft asserted a bare ") IN (SELECT " and would have passed against the old query too, because the snapshot-membership filter already contains that text. Reader suites (cpu, live reader, snapshot, end-to-end, native parity) 155/155; cpu suite 2113 passed.
The two-phase page is correct only while its inner key query and its outer
resolution query carry the same filters, and nothing tested that. With the
inner query's snapshot membership dropped, every live test still passed, yet
on a catalog holding an unpublished pack a walk ended after one page: the
unpublished keys filled the inner LIMIT, the outer query discarded them, and
a short page issues no cursor.
The new parity test stages a published pack of ten captures with alternating
hooks and an unpublished one -- written at version 8, never published, as a
crashed indexer leaves it -- holding two keys between each pair of member
keys, plus later re-descriptions of two members. Walked in pages of 2, with
and without a hook filter, the native reader must return exactly the member
captures in order, resolved to the published pack, and agree with the Python
reader.
Red first, each mutation applied to reader.cpp with conformance_catalog
rebuilt:
inner query without membership: unfiltered walk returned capture-0
alone, of 10
inner query without the hook filter: filtered walk returned 2 of 5
and in both the other 48 tests in the file still passed. Reverted: 49/49.
Also:
- reader.cpp no longer says the Python reader shares the shape; it has been
single-phase since 02b70ff.
- reader.cpp and the design doc state the read-guard cost. max_rows_to_read
limits the whole statement and both queries read capture_raw, so with a
selective filter a page can read up to twice the rows the single-phase
query did: 1,102,131 -> 2,203,414 on the review's corpus, where a limit of
1,500,000 now refuses the page with Code 158; 34 -> 68 on the test corpus
here. The query shape is unchanged.
claude Bot
pushed a commit
that referenced
this pull request
Sep 26, 2026
Brings in #149-#152 and #139 (two-phase native search page). One conflict, native/csrc/catalog/reader.cpp search(): #139 split the page into an inner key-only query and an outer argMax query over the same filters, while this branch moved snapshot membership out of `clauses` into the FROM clause (snapshot(), the join that supplies member_version). Resolved by running BOTH phases over snapshot() with the same caller `clauses` (possibly empty), so the inner LIMIT only counts member keys (#139's walk-ends-early guard) and the outer argMax still ranks on (member_version, store_id, pack_id, index_version). The design doc's two-phase paragraph now says both queries read the same snapshot join. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019oKb8SWwCwPRAtxuWLQSXt
zaoxing
added a commit
that referenced
this pull request
Sep 28, 2026
#150 was squash-merged as 7419fd0, and main then took #151, #152 and #139. The specs cited #150's head c0361d7; they now cite main at 71be2af. - storage_service.cpp moved up two lines (#151 builds the ClickHouse client from one ClickHouseConnection), native_capture.py moved with #149/#151/#152, and deciding_read() is now clickhouse_client.cpp:374. - #151 also made execute() retry a read after a transient failure, up to max_attempts (3 by default), and never a write that may have reached the server. The README's O1 caveat, its Limitations entry and LIMITS 3 in LeaseLifecycle.tla now say a lease request's reads can take up to three request timeouts, and RenewIfDue says the quarantining exception is the first to outlast those retries. No modelled outcome changes. - Refs that missed the code they describe, in files main did not change: the O1a quote is storage_service.h:233-234, not storage_service.cpp; the renewal in publish_snapshot is catalog_writer.cpp:490 and :579; publish_snapshot ends at :669; the config check with the quorum rule is :148-169; the chunk loop is :531; the watermark read-back is :608-628 (:611-627 for its refusal); the version allocator's statement lines; reject_live's comparison is lease_coordinator.cpp:222; and the start wait's knob checks are native_capture.py:351-354. - The README says what 204a8d2 is now that #150's branch is squashed. Comment and prose changes only; every verdict is unchanged.
This was referenced Sep 28, 2026
Merged
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
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.
What
Every
search()page cost about as much as the whole catalog, whatever its size. A 38-row page over a 198k-row catalog read all 198,207 rows: 59 MiB read, 263 MiB of memory, ~150 ms.Cause. Search ran a single
GROUP BY … ORDER BY … LIMITover*_capture_raw. That meant ClickHouse built the full 27-columnargMaxresolution tuple for every group past the cursor, and only then didLIMITkeeplimit + 1of them. The keyset comparison below is not usable by the primary-key index:(tenant_id, experiment_id, run_id, captured_at_ns, capture_id) > (…)EXPLAIN indexes = 1reports 26/26 granules, so nothing was pruned.Attributing the time in one page:
count())GROUP BYof the 5 key columnsThe
argMaxtuple accounts for about three quarters of it.The change
The native reader now chooses a page's keys first, then resolves the aggregate for those keys only:
Both queries carry every filter, so the groups and each group's resolution are unchanged. This relies on the design doc's existing rule that every descriptor field except the locator is immutable per capture, which already justifies the pre-aggregation filters. The design doc now describes the shape.
Native only. The Python storage capture path stays in the tree only as an unused reference, so this PR doesn't touch the Python reader. That turns out to strengthen the test: the Python reference reader keeps the single-phase shape, so the parity suite now checks the new native query against the old one.
Evidence
Paging through one 196,608-capture run on a 204k-capture catalog with the native reader, via the
conformance_catalogdriver built frommainand from this branch:Identical ids in identical order at every page size, by checksum over the full id sequence. The checksum also matches the Python reader's walk of the same run, so both implementations agree on all 196,608 ids.
The reader suites pass 155/155, up from 154 on
mainplus the new guard: cpu, live reader, snapshot, end-to-end, and native/Python reader parity. The conformance drivers were rebuilt first, so no parity case skipped.The guard,
test_native_pages_resolve_argmax_for_their_own_keys_and_match_python, lives in the parity suite and fails onmain. It checks two things:index_version, whereargMaxdecides the answer) and compares every row with the single-phase Python reader.system.query_log, and requires the key-tuple subquery on each.What it doesn't do
Each page still scans the rows past the cursor. The keyset tuple stays invisible to the index, which leaves a floor of about 61 ms per page here. I measured that a redundant bound such as
experiment_id = …orcaptured_at_ns >= …does prune (14/26 granules), but it didn't reduce the time, because aggregation dominated. Removing the floor needs index-usable cursor bounds, which is a separate change to cursor encoding.After independent review (5106083)
The reviewer walked a 1.1M-row catalog with unpublished packs, supersession, a mirror store, pins and filters. Old and new queries gave identical results in every walk. The new query was 2.5–4× faster on unfiltered and cursor walks (for example 127.8 s → 31.9 s).
test_a_page_walk_skips_unpublished_keys_without_ending_early, unpublished keys sort between member keys across page boundaries. A full walk of 2-row pages must return exactly the member captures, with and without a hook filter. Two mutations each fail it: dropping snapshot membership from the inner query, and dropping the hook filter from it. The previous tests passed both mutations, and the walk silently ended early.max_rows_to_readcovers the whole statement, and both queries readcapture_raw, so a page can read up to twice the rows it did before. This is documented indocs/capture-storage-design.mdand inreader.cpp. Sizemax_rows_to_readfor two passes.max_query_size.Brought up to date with main (#149–#152):
pytest -m cpu2520 passed. The reader parity, capture storage, snapshot and chain live suites gave 100 passed, plus the lease suite (54) before the last merge.For #156: it moves snapshot membership into
FROM snapshot(). When it is rebased onto this, the inner key query must also read fromsnapshot(), and addWHERE clausesonly when there are filters. KeepingFROM capture_raw WHERE clauseswould silently drop the snapshot, or fail with no filters. The new test catches both.