Skip to content

fix(storage): dedupe edge endpoint ids before AGE batch lookup - #522

Open
pcbeingused333 wants to merge 1 commit into
raphaelmansuy:edgequake-mainfrom
pcbeingused333:fix/projection-dedup-edge-endpoints
Open

pcbeingused333 wants to merge 1 commit into
raphaelmansuy:edgequake-mainfrom
pcbeingused333:fix/projection-dedup-edge-endpoints

Conversation

@pcbeingused333

Copy link
Copy Markdown
Contributor

Description

AgeGraphProjectionApplier::flush_graph_mutations builds the list of endpoint ids for get_edges_for_nodes_batch by flat-mapping the source and target of every pending edge upsert. A node used by k edges therefore appears k times in the list. On the AGE path the query unnests that array and joins "EDGE" to it on both ends, so an edge between two repeated nodes comes back k_src * k_tgt times: on an entity-dense document the lookup result (and memory) grows with the square of the repetition instead of with the number of edges, which is the first half of #511.

The fix moves the collection into edge_upsert_endpoint_ids, which sorts and dedups the ids before the lookup, the same thing resolve_edge_deletes already does for the delete path. Behaviour for distinct ids is unchanged.

This only addresses the duplicate-id amplification. The missing retry ceiling in the projection worker (second half of the issue) is not touched here.

Refs #511

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (describe below)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes

Additional context

New edge_upsert_endpoint_tests in projection/appliers.rs: edges A-B, B-A, A-C and A-B with another relation must yield ["A", "B", "C"], plus the empty case. Without the sort/dedup the first test gets 8 ids back and fails; with it both pass (cargo test -p edgequake-storage --lib --features postgres edge_upsert_endpoint). rustfmt --check is clean and the crate builds with no new warnings.

This change was prepared with AI assistance (Claude Code).

flush_graph_mutations collected the source and target of every edge
upsert into one list. A node shared by k edges appeared k times, and the
get_edges_for_nodes_batch join returned an edge between two such nodes
k_src * k_tgt times, inflating memory on dense documents. Sort and
dedup the ids first.

Refs raphaelmansuy#511

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@pcbeingused333

Copy link
Copy Markdown
Contributor Author

Hi, friendly ping on this one when you have a moment. Happy to adjust anything or rebase if needed. Thanks for maintaining the project!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant