BE-467: Remove Default impl from QueryTemporalAxesUnresolved#8900
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8900 +/- ##
==========================================
+ Coverage 59.25% 59.27% +0.01%
==========================================
Files 1347 1347
Lines 131082 131052 -30
Branches 5946 5944 -2
==========================================
Hits 77677 77677
+ Misses 52499 52469 -30
Partials 906 906
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:
|
Drop the deletion-test `live_only_axes()` / `find_all_axes()` wrappers in favour of direct `QueryTemporalAxesUnresolved::live_only()` / `::all()` calls. `live_only_axes()` previously pinned the *transaction-time* axis; the decision-time `live_only()` finds the same live entities — verified by running the deletion and integration suites (364 passed) against a clean DB. Also clarify the `all()` doc: it serves any archived-inclusive read, not only removal.
PR SummaryMedium Risk Overview Every former Reviewed by Cursor Bugbot for commit 46c4519. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
This PR removes the Default implementation from QueryTemporalAxesUnresolved (which previously implied an unbounded “find all, including archived” query) and replaces it with two explicit, intent-revealing constructors: all() and live_only(). It then updates call sites across the store, Postgres store, integration tests, and benchmarks to use the new constructors, making temporal intent explicit and reviewable.
Changes:
- Removed
impl Default for QueryTemporalAxesUnresolvedand introducedQueryTemporalAxesUnresolved::all()/QueryTemporalAxesUnresolved::live_only(). - Replaced previous
default()and inline temporal-axes literals at call sites with the appropriate named constructor. - Simplified imports throughout tests/benches and removed deletion test helper functions that duplicated axes construction.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/graph/integration/postgres/sorting.rs | Switches entity query axes to live_only() and simplifies temporal axes imports. |
| tests/graph/integration/postgres/property_type.rs | Uses all() for property type queries and removes now-unused temporal bound/axis imports. |
| tests/graph/integration/postgres/partial_updates.rs | Replaces inline “current snapshot” decision-time axes with live_only(). |
| tests/graph/integration/postgres/multi_type.rs | Replaces inline “current snapshot” decision-time axes with live_only(). |
| tests/graph/integration/postgres/links.rs | Uses all() / live_only() as appropriate; simplifies temporal axes imports. |
| tests/graph/integration/postgres/entity.rs | Uses all() for “find everything” reads and live_only() for current snapshot reads. |
| tests/graph/integration/postgres/entity_type.rs | Uses all() for entity type queries; simplifies imports. |
| tests/graph/integration/postgres/email_filter_protection.rs | Updates helper to return QueryTemporalAxesUnresolved::all(). |
| tests/graph/integration/postgres/data_type.rs | Uses all() for data type queries; simplifies imports. |
| tests/graph/benches/representative_read/ontology/entity_type.rs | Uses all() in ontology read benchmark setup. |
| tests/graph/benches/representative_read/knowledge/entity.rs | Uses live_only() / all() to make benchmark query intent explicit. |
| tests/graph/benches/read_scaling/knowledge/linkless/entity.rs | Uses all() in scaling benchmark query. |
| tests/graph/benches/read_scaling/knowledge/complete/entity.rs | Uses all() in scaling benchmark query. |
| tests/graph/benches/graph/scenario/stages/reset_db.rs | Replaces default() with all() for reset/erase stage. |
| tests/graph/benches/graph/scenario/stages/entity_type.rs | Replaces default() with all() for registry build stage. |
| tests/graph/benches/graph/scenario/stages/entity_queries.rs | Replaces default() with all() for scenario query stage. |
| libs/@local/graph/type-fetcher/src/store.rs | Uses live_only() for type fetcher “current snapshot” reads and simplifies imports. |
| libs/@local/graph/store/src/user_deletion.rs | Uses all() for deletion queries intended to include archived/history; simplifies imports. |
| libs/@local/graph/store/src/subgraph/temporal_axes.rs | Removes Default and introduces all() / live_only() constructors with docs. |
| libs/@local/graph/postgres-store/tests/deletion/validation.rs | Updates deletion tests to use QueryTemporalAxesUnresolved::live_only(). |
| libs/@local/graph/postgres-store/tests/deletion/purge.rs | Updates deletion tests/helpers to use QueryTemporalAxesUnresolved::live_only(). |
| libs/@local/graph/postgres-store/tests/deletion/main.rs | Removes live_only_axes() / find_all_axes() helpers and routes callers to constructors. |
| libs/@local/graph/postgres-store/tests/deletion/links.rs | Updates deletion tests to use live_only() / all() and refreshes commentary accordingly. |
| libs/@local/graph/postgres-store/tests/deletion/erase.rs | Updates erase tests to use QueryTemporalAxesUnresolved::live_only(). |
| libs/@local/graph/postgres-store/tests/deletion/drafts.rs | Updates drafts-related deletion tests to use QueryTemporalAxesUnresolved::live_only(). |
| libs/@local/graph/postgres-store/src/store/validation.rs | Uses live_only().resolve() for validation reads and simplifies imports. |
| libs/@local/graph/postgres-store/src/store/postgres/query/statement/select.rs | Updates SQL compilation tests to use all() instead of default(). |
| libs/@local/graph/postgres-store/src/store/postgres/query/expression/where_clause.rs | Updates where-clause transpilation test to use all() instead of default(). |
| libs/@local/graph/postgres-store/src/store/postgres/ontology/property_type.rs | Uses live_only().resolve() for permission checks and simplifies imports. |
| libs/@local/graph/postgres-store/src/store/postgres/ontology/entity_type.rs | Replaces default() with all() where behavior depended on old default; uses live_only() where appropriate. |
| libs/@local/graph/postgres-store/src/store/postgres/ontology/data_type.rs | Uses live_only().resolve() for permission checks and simplifies imports. |
| libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/mod.rs | Replaces inline “current snapshot” axes with live_only(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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: 1001 | 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: 1526 | 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: 51 | 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: 107 | 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: 1 | 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: 25 | 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: 26 | 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 |
🌟 What is the purpose of this PR?
QueryTemporalAxesUnresolvedimplementedDefault, which silently resolved to the "find all entities, including archived" axes (DecisionTime { pinned: None, variable: [Unbounded, now] }).default()communicated no intent, so reviewers (and automated review tools) couldn't tell whether a given call site deliberately wanted "find all" or had accidentally picked the wrong axes — this was flagged repeatedly during the BE-460 deletion review.This PR removes the
Defaultimpl and replaces it with two intent-revealing named constructors, then routes every call site through them.🔗 Related links
🔍 What does this change?
impl Default for QueryTemporalAxesUnresolved.QueryTemporalAxesUnresolved:all()— pins transaction time at the resolution timestamp, decision time unbounded up to it → reaches all entities incl. archived. For any archived-inclusive read (history-aware queries, erase-all /resetGraph), not only removal.live_only()— pins both axes at the resolution timestamp → only currently-live entities (the conventional "current snapshot" query).QueryTemporalAxesUnresolved::default()calls →all().all()(incl. the productionuser_deletion.rssite that triggered BE-460's review confusion).live_only().live_only_axes()andfind_all_axes(), inlining their ~64 call sites ontolive_only()/all().live_only_axes()previously pinned the transaction-time axis; switching it to the decision-timelive_only()finds the same live entities — verified by the suites below.axes_at_decision_time()(parameterised) is kept.Every replacement of
default()/inline axes is byte-identical to the code it replaces; the only behavioral change is the deletion-test helper's axis, which the tests confirm is equivalent.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:
None.
🐾 Next steps
None.
🛡 What tests cover this?
Existing test suites, all green:
hash-graph-postgres-storeunit tests — 136/136 passed (incl. theselect.rsSQL-compilation tests that resolveall()).all()andlive_only()in real queries.clippy --all-features --all-targetsclean acrossstore,postgres-store,type-fetcher,integration,benches,api.No new tests added — behavior is unchanged, so existing coverage applies.
❓ How to test this?
cargo clippy --all-features --all-targets -p hash-graph-store -p hash-graph-postgres-store -p hash-graph-type-fetcher -p hash-graph-integration -p hash-graph-benches -p hash-graph-apicargo nextest run -p hash-graph-postgres-store -p hash-graph-integration --all-features(against a freshly-migrated, unseeded graph DB)