BE-705: Add a dedicated entities-table query endpoint to the Graph - #9094
BE-705: Add a dedicated entities-table query endpoint to the Graph#9094TimDiekmann wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
Bot
added
area/deps
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9094 +/- ##
==========================================
+ Coverage 59.36% 59.55% +0.19%
==========================================
Files 1406 1408 +2
Lines 136519 137734 +1215
Branches 6407 6418 +11
==========================================
+ Hits 81042 82027 +985
- Misses 54487 54712 +225
- Partials 990 995 +5 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
In-depth review of the full diff against the actual base branch t/be-598-investigate-fetch-keys-then-hydrate-projection-for-the (this PR is stacked on #9048; stack-wide context noted where relevant), focused on correctness and performance as requested, with Linear BE-705 as the requirements baseline. No blocking defects on the default path; the Important items below are worth addressing before (or immediately after) merge, everything under Suggestions is non-blocking.
Important
-
includeDrafts: truebreaks the keys-first precondition: link-endpoint hydration joins fan out over draft editions and can mix editions in one row. The comment abovecompiler.set_statement_shape(StatementShape::KeysFirst)(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:524) vouches that hydration joins match at most one row per key — but that only holds when drafts are excluded.temporal_metadata_conditions(libs/@local/graph/postgres-store/src/store/postgres/query/compile/mod.rs:526) addsdraft_id IS NULLtoentity_temporal_metadatajoin conditions only when!self.include_drafts, compiler-wide — including the endpoint hydration joins (visible in thetable_page_statementgolden). WithincludeDrafts: true, an endpoint entity with a live edition plus drafts matches multiple temporal-metadata rows, and the golden shows the right endpoint'sedition_id, label,versioned_urls, anddirect_typesare read through independent duplicate join chains (entity_temporal_metadata_2_2_2/_2_2_3/_2_2_4) — so a single product row can combine the label from one edition with the type arrays of another, after whichLinkEndpointIndices::decode's.take(direct_type_count)(table.rs:191) truncates the wrong array and hardcodesdraft_id: Noneeven for a draft edition. The outerDISTINCT ONcollapses the fan-out arbitrarily, so the endpoint shown (and the edition fed tofilter_knowledge_edges) is nondeterministic. The stacked frontend (#9095) hardcodesincludeDrafts: false, but the endpoint acceptstrueand the cursor pins it for whole page sequences; the only draft test (drafts_are_hidden_unless_requested,tests/graph/integration/postgres/table.rs:1126) uses a link-less draft, so the path is untested. Fix: pin endpoint hydration joins to the non-draft edition regardless of the root query's draft visibility (drafts of root entities are rows; drafts of their endpoints are not addressable link targets), or include the endpointdraft_idin the join key. -
The derived type universe is uncapped — in the page statement's parameter list and in the cursor token. When no explicit type filter is given,
type_universecollects every distinct visible type id from the summary with no bound (libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:440), then (a)type_include_filter(table.rs:996) binds one text parameter per versioned URL (the compiler merges theFilter::Anyinto a single GIN-indexableversioned_urls && ARRAY[$..]clause, but each element is still a bind parameter), and (b) the whole list is embedded verbatim inEntityTableCursor(libs/@local/graph/store/src/entity/table.rs:261), base64-JSON'd into every response and re-sent, re-parsed, and re-compiled on every continuation. BE-705 explicitly specifies the cursor carries the universe "up to a practical cap" and notes the include clause is result-neutral (the scope filter alone is authoritative), so the graph is "free to cap it … or drop it per request" — the implementation has no cap anywhere. At realistic scale (hundreds–thousands of visible type versions) that means tens-of-KB opaque tokens both ways per page and O(#types) bind parameters per statement; at the extreme the extended-protocol Bind limit (65,535 parameters) fails the page query outright. Because the clause is a planner aid, dropping (or truncating) the universe past N types is a safe, purely planner-side guard that bounds both the token and the statement. -
The new
/entities/query/tablehandler bypassesQueryLogger, unlike every sibling read handler in the same file.query_entities,query_entity_subgraph, andsummarize_entitiesall takeOption<Extension<QueryLogger>>,capture(...)the raw request, andsend()after the store call;query_entities_table(libs/@local/graph/api/src/rest/entity/query/mod.rs:252) omits the extension entirely, and noQueryEntitiesTablevariant was added toOpenApiQuery(libs/@local/graph/api/src/rest/mod.rs:356). This matters beyond consistency: #9095 moves the entities-table view offqueryEntitySubgraph(which is logged) onto this endpoint, so a class of production query traffic that is captured today silently vanishes from the query log with no signal that logging stopped. The fix is mechanical — add the variant and wire the capture/send pair like the siblings. (Notingsearch_entities/cluster_entitiesalso skip the logger, so the convention isn't universal — but all three direct siblings of this handler log, including the lightweight summarize endpoint.) -
Snapshot pinning — the endpoint's headline invariant — has zero integration coverage. The entire cursor design (pinned
transaction_time/decision_time, the sealed type universe,instant_axes()atlibs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:901) exists so that follow-up pages continue on the first page's database state, per the PR description and BE-705. No test exercises it: every paging test intests/graph/integration/postgres/table.rs(e.g.collect_all_pages,a_continuation_reads_its_sort_from_the_tokenattests/graph/integration/postgres/table.rs:865) runs against a static dataset, with all mutations before page 1. If the continuation branch regressed toTimestamp::now(), or the universe were re-derived per page, every current test would still pass because each page sees identical data anyway. A test that takes page 1, then creates an entity / patches an existing one / creates an entity of a brand-new type, then continues from the cursor — asserting the new entity is absent, the patched entity shows its old edition, and the pinned universe excludes the new type — would cover the single most consequential untested behavior in the PR. -
No test verifies the summary count and type pills respect policy restrictions.
link_endpoints_hide_entities_the_actor_cannot_view(tests/graph/integration/postgres/table.rs:951) inserts the suite's only ForbidViewEntitypolicy but never requests the summary, and no table test queries as a second actor with reduced visibility; theemail_filter_protection.rsadditions cover the count only through the property-filter rewrite (thetable_countpath). That leavesscope_summary(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:710) — a brand-new statement path with its own JIT-off and fold machinery, and the sole source of the pills and of the count on the common no-filter path — untested for the property that it excludes entities the actor cannot view. Dropping the singleadd_filter(policy_filter)call from the summary compiler would leak other webs' counts and types with every current test still green (the page query applies its own policy filter, hiding the forbidden row). Extending the Forbid-policy test to requestincludeSummaryand assert count == 6 (not 7; the test also creates a friend-of link) and person pill == 3 (not 4) would pin it.
Suggestions
-
A well-formed cursor token whose
positionmismatches its own sort silently degrades the keyset instead of erroring. The continuation buildsEntityQuerySorting { paths: sorting_records(sort, include_drafts), cursor: position }(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:529) without checking arity, andEntityQuerySorting::compilezipspathswithcursor.values(libs/@local/graph/postgres-store/src/store/postgres/query/mod.rs:169), truncating silently: an emptypositioncompiles with no keyset condition (and no ordering), a short one drops the uuid tiebreaker, and the weakened cursor is re-emitted in every subsequent token; a wrongCursorFieldtype surfaces as a Postgres bind error (500) rather than a 400. The token design otherwise takes rejection seriously (deny_unknown_fields, thegarbage_cursor_tokens_are_rejectedtest atlibs/@local/graph/store/src/entity/table.rs:521), but only shape-garbage is rejected — semantic mismatches pass serde. Sincesortandinclude_draftsare both known from the token, validatingposition.values.len()against the sorting-record count (and rejecting as an invalid cursor) closes this, plus a test pinning the intended behavior. Tamper-only blast radius (all filters still apply), hence non-blocking. -
The cursor payload has no version discriminator and rejects unknown fields.
EntityTableCursorPayloadisdeny_unknown_fieldswith no version marker (libs/@local/graph/store/src/entity/table.rs:312). Because clients hold tokens opaquely across requests, any future payload change breaks in-flight pagination in both directions during a rolling deploy (old instance 422s a new token; new instance 422s an old one). Blast radius is "restart from page one", but a one-byte version field — or defaulted optional fields instead ofdeny_unknown_fields— makes the sealed shape evolvable, and this PR mints the format, so now is the cheapest moment. -
includeDraftssits insideEntityTableFilterbesideincludeArchivedbut obeys the opposite continuation semantics. On a continuation, draft visibility is read from the token and the re-sent request is ignored (libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:353), whileinclude_archived— like webs, type excludes, and property filters — is honored from the re-sent request viascope_filter. Two adjacent booleans (libs/@local/graph/store/src/entity/table.rs:231,:233) with different trust models is a client foot-gun: toggling one mid-scroll takes effect, toggling the other is silently ignored. Sinceinclude_draftsis pinned because it shapes the keyset (the thirddraft_idsort column), it behaves likesort— which the PR already places top-level. Moving it next tosortbefore clients depend on the wire shape would make the pinning legible. -
Re-sending
includeSummaryon a continuation silently re-runs the O(N) summary scan every page.let scope_summaries = if params.include_summary || needs_universe(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:425) honors the re-sent flag even with a cursor present, where the pinned instants make the result byte-identical to page 1's — so a client that naively keepsincludeSummaryin its request state pays the PR's acknowledged O(N) hot spot on every "load more". #9095 guards this client-side (includeSummary: !cursor), but SDK/GraphQL/REST consumers aren't protected, and the field has no doc (libs/@local/graph/store/src/entity/table.rs:388area). At minimum document that the summary is intended for cursor-less requests; acursor.is_none()short-circuit is defensible too (with the caveat that it forecloses deliberately re-requesting the summary later). -
The universe-only summary scan aggregates type titles nobody reads.
scope_summaryalways requeststype_ids: true, type_titles: true(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:731). In theneeds_universe && !include_summarypath only the type-id keys are consumed, yet requesting titles adds a secondunnest(type_titles[1:direct_types])SRF and amin(t.title)per group inside the very scan the PR flags as the O(N) hot spot (and whose SRF misestimation is the stated reason for disabling JIT). Passingtype_titles: params.include_summarywould trim it —EntitySummaryQuery::new/statement/decodeneed a small tweak sincetype_ids || type_titlesshareTypeColumnstoday. -
Full inheritance-depth type arrays are transported and parsed per row, then truncated client-side.
RowIndices::decodereads the wholeentity_edition_cache.versioned_urls/type_titlesarrays and then.take(direct_type_count)(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:262); the same pattern runs twice more per link row inLinkEndpointIndices::decode(table.rs:191). Every inherited entry is shipped and parsed into a validatedVersionedUrlonly to be discarded — the summary branch already slicesversioned_urls[1:direct_types]server-side. Per-page cost is bounded, so this is an optimization, not a blocker (and the genericSelectCompilerlacks array-slice expressions today, so it's not a one-liner). -
The pinned snapshot instant is minted from the wall clock, not the snapshot.
Timestamp::now()(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:356) is taken after the REPEATABLE READ snapshot was established, so a writer whose recordedtransaction_timestart is <= now but which commits after the snapshot is invisible to page 1's summary/count/universe yet matchestransaction_time @> nowin the fresh transactions serving continuations — rows can bleed into later pages uncounted, and a bled-in entity with a type outside the pinned universe is silently dropped, contradicting the documented "follow-up pages continue on the first page's database state". Since writers stamp their own pre-commit start times, no reader-side instant fully closes this; the actionable ask is a code comment documenting the accepted residual window. -
Type exclusion matches inherited types, but the doc and the test only cover direct types.
Excludecompiles toNOT(base_urls @> ...)(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:861), and per the V51 migrationbase_urlscovers all inheritance depths — so excluding a base URL hides entities whose inherited types include it, while the summary pills are direct-type-only. Likely intended (it matches the legacy noisy-system-types semantics), but the variant doc ("Entities carrying an excluded type are left out entirely",libs/@local/graph/store/src/entity/table.rs:205) never says inheritance counts, and the integration test only excludes a directly-carried type. Document the semantics on the variant and pin them with a subtype test so the behavior is a contract rather than an accident of the cache layout. -
The multi-type exclusion case the code specifically claims to handle is untested.
scope_filter's comment says placing the exclusion in the shared scope "catches multi-type entities that also carry a universe type", and the store docs promise excluded entities leave the rows, count, and summary alike — butexcluded_type_base_urls_leave_the_universe_and_the_summary(tests/graph/integration/postgres/table.rs:913) uses only single-type entities. If the exclusion migrated into the page's type clause (the natural refactor the comment warns against), a person+page entity would leak back into the rows via its person type with no test failing. Reusing the multi-type fixture withpage/excluded and asserting rows == 3, count == 3, person pill == 3 would pin it. -
Direct-vs-inherited truncation in row decode is exercised but never asserted. The seeded friend-of link type inherits from the block-protocol link type, so
link_rows_carry_their_endpointsdoes run the.take(direct_type_count)path — but it asserts only the endpoints'entity_type_ids(inheritance-free persons), never the link row's own. Delete the.take(...)calls (libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:264,:269) and the whole suite still passes while rows show inherited types. One assertion —link_row.entity_type_ids == vec[friend_of_type]— pins the contract; the summary's[1:direct_types]slice has the same gap (no test asserts inherited types are absent from the pills). -
Cursor pinning of
include_draftsis untested — only sort pinning has a contradiction test.a_continuation_reads_its_sort_from_the_tokensharply verifies a contradicting re-sent sort is ignored, butdrafts_are_hidden_unless_requested(tests/graph/integration/postgres/table.rs:1164) re-sendsinclude_drafts: trueon every continuation, so token and request always agree. A regression that read draft visibility from the request would silently desync the keyset arity (three sort columns vs. a two-value position — and the zip truncates rather than erroring, see Suggestion 1), producing wrong pages with no failure. A one-page test flippingincludeDraftson the continuation and asserting the sequence is unchanged would mirror the sort test. -
The exact-fill test passes even if the behavior it names regresses.
a_page_exactly_filling_the_limit_hands_out_a_final_empty_pagerelies oncollect_all_pages, whoselet Some(new_cursor) = response.cursor else { break; }(tests/graph/integration/postgres/table.rs:461) silently tolerates a full page returning no cursor — with total == limit, the loop would break after page 1 with rows.len() == 5 and still pass, which is exactly the boundary the documented contract ("a page that exactly fills the limit still hands one out") is about. Assertingresponse.cursor.is_some()inside the helper wheneverpage_len == limitcloses it and strengthens every other paging test. -
The
TimeIntervalcursor never executes against Postgres.EditionCreatedAtDecisionTime,TypeTitle, andArchivedexist only as ORDER BY goldens (libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:1234); no test pages them against the database. The tie/keyset machinery itself is base-branch code already DB-tested elsewhere (sorting.rspages tied non-NULL values with chunk size 1), andTypeTitle's column expression mirrors the DB-testedLabel— so the one gap with real exposure isEditionCreatedAtDecisionTime: it maps toEntityQueryPath::DecisionTime, whose keyset value is atstzrange(CursorField::TimeInterval), and no test anywhere pages by a TimeInterval cursor. Onecollect_all_pagesrun under that key would give the ToSql/comparison path its first-ever execution. -
statement_count_without_types_keeps_the_count_branchduplicates the pre-existingstatement_count_onlyverbatim. The PR-added test (libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/summary.rs:469) constructs the identical count-onlyEntitySummaryQueryand asserts the identical SQL string as the existing test atsummary.rs:494. Its name suggests it was meant to vary something — e.g. count + web_ids without types, a distinct un-goldened shape. Repurpose or delete. -
EntityTableRowuses parallel arrays for type ids and titles.entity_type_ids: Vec<VersionedUrl>/entity_type_titles: Vec<String>(libs/@local/graph/store/src/entity/table.rs:425) make the invalid state (mismatched lengths) representable on the wire and force clients to zip by index; a singleVec<{ entityTypeId, title }>rules it out by construction (the same PR'sEntityTableSummaryalready uses the mismatch-free map shape). The inconsistency is visible in-PR:EntityTableLinkEndpoint(table.rs:403) carries ids with no titles, so endpoint chips resolve throughclosedMultiEntityTypeswhile row chips read inline titles — two rendering paths for the same concept in one response. Painful to change once clients depend on it. -
[Stack context] The endpoint hard-codes
KeysFirstwith no runtime kill-switch. Carrying over the base-branch (#9048) observation, newly relevant here:compiler.set_statement_shape(StatementShape::KeysFirst)(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:527) is unconditional, and no env var orPostgresStoreSettingsfield can forceSinglePassat runtime — the only fallbacks are structuralShapeFallbackconditions this endpoint never triggers. The generic read path at least conditions its opt-in onhas_embeddings_filter(); and #9095 deletes the frontend's previous table query path, so a keys-first plan regression in production (the compiler's own doc admits "a misestimated filter can still bait it into an ordered scan that never fills the limit") has no mitigation short of revert-and-redeploy. Thread a settings/env override mirroring howsettings.filter_protectionis consulted a few lines above, or document the intended operational rollback.
Strengths
- The one-transaction REPEATABLE READ design (summary → derived universe → page) genuinely closes the staleness window BE-705 describes, and pinning bare instants in the cursor makes the interval axes that would break the one-row-per-entity shape unrepresentable — the continuation correctly survives concurrent edition rewrites because closed decision intervals land in new row versions the pinned
transaction_timeexcludes. - Keyset pagination is unusually careful: the token seals sort, draft visibility, and snapshot so a contradicting re-sent request cannot desynchronize the keyset (adversarially tested in
a_continuation_reads_its_sort_from_the_token), and NULL sort boundaries are handled in both orderings including the exhausted-trailing-NULL-group case — with the label-sort test paging limit-2 across the NULL keyset boundary, the hardest path of the cursor machinery. - Permission and protection parity with the generic read path is real, not asserted: user property filters pass through the same
transform_filterrewrite before they can influence rows or the count (closing the enumeration oracle, re-verified end-to-end for this endpoint inemail_filter_protection.rs), rows are property-masked, and link endpoints are nulled via the samefilter_knowledge_edgespolicy check the subgraph traversal uses — batched over the whole page, applied before the closed-type maps are assembled. - The keys-first shape is exactly right for the misestimating planner: the
roots/limitedCTEs carry only narrow key columns through filter+sort+LIMIT, wide jsonb/array columns are hydrated for at mostlimitrows, and both CTEs stay single-reference so PG12+ inlines them — pinned by thetable_page_statementgolden. - The count fold (
count(*) FILTER (WHERE ord = 1)) is correct given every edition has at least one direct type, decode initializes to 0 so empty scopes are right, and the ride-along is gated precisely oncount_matches_scope, where the derived universe clause is provably result-neutral — saving a second full scan. SET LOCAL jit = offfor the summary scan is a targeted fix for a real Postgres failure mode (SRF row-count misestimates blowing past JIT cost thresholds), correctly transaction-scoped with a comment explaining why the remaining limited keyset reads are unaffected.- The derived universe compiles to a single GIN-indexable
versioned_urls && ARRAY[...]overlap and base-URL exclusions merge into oneNOT(base_urls && ...)— estimable, index-friendly shapes rather than OR chains; instant-only axes let the summary skipDISTINCT ON(Deduplication::Skip) unless a to-many join actually exists. - No N+1 on the hot path: one
filter_knowledge_edgescall per page,get_closed_multi_entity_types/resolve-definitions run once over deduplicated type ids, and the endpoint types of both link sides are verifiably included in the response's closed-type maps. - The test suite bakes paging invariants into every test via
collect_all_pages(no duplicate rows across pages, page never exceeds limit, short page carries no continuation), and SQL goldens pin the page statement, exclusion scopes, all 13 property operators, sort keys, and the summary fold, so statement-shape regressions fail loudly. Boundary coverage is broad: empty include lists match nothing, excluded web yields zero count and empty pills, exact page size, archived/draft visibility both ways, garbage cursor tokens rejected, and the multi-type count fold.
Generated by Claude Code
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
PR SummaryMedium Risk Overview Graph store & Postgres: New API layers: REST Tests & misc: Integration tests for paging, cursors, scoping, link permissions, and email filter protection on the table path; marks generated OpenAPI as linguist-generated. Frontend entities visualizer TODO for BE-705 is removed (client wiring is follow-up). Reviewed by Cursor Bugbot for commit 8cfdd9f. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The entities table no longer assembles its rows from a subgraph: the new /entities/query/table endpoint serves flat rows built from materialized columns, with the type summary, the count, and the page read in one repeatable-read transaction. When no explicit type filter is given, the visible-type universe is derived from the summary and drives the page query as an indexable include clause. The count rides along in the summary scan when the page carries no narrowing filters. Follow-up pages continue on the first page's database state through an opaque cursor sealing two snapshot instants, the type universe, the sort, the draft visibility, and the keyset position. Property filters pass through the filter-protection rewrite, rows are masked like the generic read path, and link endpoints pass the knowledge-edge permission check. The endpoint is exposed through the Node API as queryEntitiesTable and branded in the TypeScript SDK.
Eq derives for the property-filter types, a slice pattern instead of repeated indexing, expects for the long test bodies and the impl, and a reasoned expect for the summary request's independent include switches.
The dedicated endpoint this PR adds is what the note asked for, and the table view moves onto it in the follow-up PR.
…draft rows A selection used to replace the base-URL exclusions rather than narrow within them, so picking a type pill brought the noisy system types back into the rows, the count, and the pills. The exclusions move to their own filter field that always applies, and the selection is a plain list of versioned ids: absent for no narrowing, empty to match nothing. The visible-type universe stays a Graph-internal notion. Draft entities are no longer rows. Including them dropped the compiler's draft_id IS NULL from every temporal-metadata join, including the ones hydrating a link row's endpoints — an endpoint with drafts matched one row per edition, letting a product row mix columns from different editions while the outer DISTINCT ON collapsed the fan-out arbitrarily. Excluding drafts makes the one-row-per-key invariant structural. The derived universe is capped: past the limit it is dropped rather than grown into the cursor token and the statement's parameter list, which the scope filter makes safe. Cursor positions of the wrong arity are rejected instead of compiling into a silently weakened keyset. New tests pin the snapshot a continuation reads, the summary's policy scope, and that a selection cannot bring back an excluded type.
A continuation whose sequence had no universe pinned — because it exceeded the limit — read that as "not derived yet" and paid the full summary scan again, on every page, only to drop the universe again each time. The token already says what its sequence runs on, so only a first page derives one. The cap moves into a function of its own and gets a test: the boundary and the deterministic order it needs for the cursor were unpinned.
84f441b to
8cfdd9f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8cfdd9f. Configure here.
| let type_filter = type_selection | ||
| .as_ref() | ||
| .or(type_universe.as_ref()) | ||
| .map(|entity_type_ids| type_include_filter(entity_type_ids)); |
There was a problem hiding this comment.
Pinned universe loses to type selection
Medium Severity
type_selection.as_ref().or(type_universe.as_ref()) prefers a re-sent entity_type_ids over a cursor-pinned type_universe. Sort is read from the token and ignores a contradicting request (and has a test for that), but a pinned universe is not. A continuation that also carries a type selection drops the sealed include clause, and the next cursor then omits type_universe entirely, so later pages can skip, repeat, or widen relative to the original sequence.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8cfdd9f. Configure here.
There was a problem hiding this comment.
Verified against 8cfdd9f — this is the documented contract rather than a bug, so no change needed:
- A sequence started with a type selection never pins a universe (
needs_universerequirestype_selection.is_none()), and the client re-sendsentityTypeIdson every continuation to keep the narrowing. The request's selection winning over the pinned universe is what makes that legitimate flow work; ignoring it when a cursor is present would break it. See thetype_universedoc inlibs/@local/graph/store/src/entity/table.rs("the sequence runs on a type selection the client re-sends"). - The pinned universe is a result-neutral planner aid — the scope filter alone decides row membership, and the snapshot instants + sort + keyset position always come from the token. So a continuation that swaps in its own
entityTypeIdscan't repeat rows, and the rows it gets are exactly what its own filter asked for at the pinned snapshot. - Changing any filter mid-cursor (
webs,propertyFilters,includeArchived, …) has the same effect;entityTypeIdsisn't special. The sort field is token-protected only because an honored contradicting sort would flip the keyset comparison and make the position meaningless — there's no equivalent hazard for a changed type filter.
The one real gap here is defense-in-depth: uniformly rejecting (or documenting) filter changes alongside a cursor would make the contract explicit. That's a design tightening, not a fix for this pair specifically.
Generated by Claude Code
Benchmark results
|
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2002 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1002 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 3314 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 1527 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 2078 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 1033 | Flame Graph |
policy_resolution_medium
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 102 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 269 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 108 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 133 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 63 | Flame Graph |
policy_resolution_none
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 8 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 3 | Flame Graph |
policy_resolution_small
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 26 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 94 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 27 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 66 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 29 | Flame Graph |
read_scaling_complete
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id;one_depth | 1 entities | Flame Graph | |
| entity_by_id;one_depth | 10 entities | Flame Graph | |
| entity_by_id;one_depth | 25 entities | Flame Graph | |
| entity_by_id;one_depth | 5 entities | Flame Graph | |
| entity_by_id;one_depth | 50 entities | Flame Graph | |
| entity_by_id;two_depth | 1 entities | Flame Graph | |
| entity_by_id;two_depth | 10 entities | Flame Graph | |
| entity_by_id;two_depth | 25 entities | Flame Graph | |
| entity_by_id;two_depth | 5 entities | Flame Graph | |
| entity_by_id;two_depth | 50 entities | Flame Graph | |
| entity_by_id;zero_depth | 1 entities | Flame Graph | |
| entity_by_id;zero_depth | 10 entities | Flame Graph | |
| entity_by_id;zero_depth | 25 entities | Flame Graph | |
| entity_by_id;zero_depth | 5 entities | Flame Graph | |
| entity_by_id;zero_depth | 50 entities | Flame Graph |
read_scaling_linkless
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | 1 entities | Flame Graph | |
| entity_by_id | 10 entities | Flame Graph | |
| entity_by_id | 100 entities | Flame Graph | |
| entity_by_id | 1000 entities | Flame Graph | |
| entity_by_id | 10000 entities | Flame Graph |
representative_read_entity
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph |
representative_read_entity_type
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| get_entity_type_by_id | Account ID: bf5a9ef5-dc3b-43cf-a291-6210c0321eba
|
Flame Graph |
representative_read_multiple_entities
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_property | traversal_paths=0 | 0 | |
| entity_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=0 | 0 | |
| link_by_source_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true |
scenarios
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| full_test | query-limited | Flame Graph | |
| full_test | query-unlimited | Flame Graph | |
| linked_queries | query-limited | Flame Graph | |
| linked_queries | query-unlimited | Flame Graph |
2 participants
🌟 What is the purpose of this PR?
The entities table currently assembles its rows client-side from a subgraph query, which pays for edge resolution and closed-schema transport the table never renders. This PR adds a dedicated
/entities/query/tableendpoint to the Graph that serves flat, render-ready rows built from materialized columns — with the type summary, the count, and the page read in one repeatable-read transaction, so follow-up pages continue on the first page's database state.🔗 Related links
🔍 What does this change?
hash-graph-store):QueryEntitiesTableParamswith tagged web/type scopes (include/exclude, where type exclusion works on base URLs so noisy system types stay hidden across versions), a flat 13-operator property-filter enum, and a closed sort-key enum over the materialized, indexable columns.count(*) FILTER (WHERE ord = 1)) when the page carries no narrowing filters, saving a second full scan.queryEntitiesTablethrough the Node API/GraphQL, and branded types in the TypeScript SDK.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
🐾 Next steps
archived(and latercreated_by_id) intoentity_edition_cacheto drop two joins from the summary scan — measured to cut its cost roughly in halfSelectCompiler🛡 What tests cover this?
tests/graph/integration/postgres/table.rs: paging invariants (no duplicate rows, short page ends the sequence), label sort across the NULL keyset boundary, cursor sort pinning against a contradicting re-sent request, link-endpoint permission nulling, property filters, conversions, archived/draft visibility, web/type scoping incl. empty include, and the multi-type count foldemail_filter_protection.rs: equals/startsWith enumeration via rows and count, response masking❓ How to test this?
cargo run --bin hash-graph --all-features -- serverPOST /entities/query/tablewith{"filter": {}, "limit": 10, "includeSummary": true}and anX-Authenticated-User-Actor-Idheader"cursor"and confirm the next page continues the sequence📹 Demo
Covered by the frontend PR stacked on this one.