Skip to content

Time semantics overhaul #1969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 173 commits into
base: master
Choose a base branch
from
Open

Time semantics overhaul #1969

wants to merge 173 commits into from

Conversation

ljeub-pometry
Copy link
Collaborator

@ljeub-pometry ljeub-pometry commented Mar 6, 2025

What changes were proposed in this pull request?

  • refactor of the time semantics to simplify history filtering
  • filtering layers/edges now filters nodes as well if all the edge updates that added them are filtered out and the node is not added explicitly via add_node.
    • as a result, subgraph filters nodes that don't have edges in the subgraph and were not explicitly added via add_node.
  • changes latest_time semantics for the PersistentGraph to return the time of the last update for the node/edge/graph in the current view or the start of the window if there are no updates.
  • The earliest_time and latest_time within a filtered Event Graph will now reflect the updates within the graph view instead of just window bounds.
  • adds a ValidGraph filter that only keeps edges that are currently valid without removing their history
  • is_valid and is_active are no longer the same for the persistent graph
    • active means there is an update during the period (addition or deletion)
    • valid means that the edges most recent update is an addition (persistent semantics).
    • deleted means that the edges most recent update is a deletion.
  • the event graph preserves deletions if created from a persistent graph
    • an edge is included in a window if it is active in the window (has an addition or deletion event)
    • an edge is valid if it has an addition event in the current view
    • an edge is deleted if it has an addition event in the current view
  • The default layer only exists if it has updates on it.

Why are the changes needed?

Fixes #1050, fixes #1787, fixes #1191
Fixes #1779

Does this PR introduce any user-facing change? If yes is this documented?

How was this patch tested?

  • adds tests for the new semantics
  • more proptests to check that materializing views gives consistent results

Are there any further changes required?

@ljeub-pometry ljeub-pometry force-pushed the feature/IsValidFilter branch from 7f2d03d to dfbb7a8 Compare March 10, 2025 12:58
@ljeub-pometry ljeub-pometry force-pushed the feature/IsValidFilter branch from 14b8b49 to bab1997 Compare March 24, 2025 13:11
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 816506e Previous: c8f2a0f Ratio
lotr_graph_window_50_layered/iterate edges 117827 ns/iter (± 327) 50232 ns/iter (± 81) 2.35
lotr_graph_persistent_window_50_layered/has_node_existing 117 ns/iter (± 77) 27 ns/iter (± 0) 4.33
lotr_graph_persistent_window_50_layered/iterate nodes 29082 ns/iter (± 94) 9018 ns/iter (± 13) 3.22

This comment was automatically generated by workflow using github-action-benchmark.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2025

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants