Skip to content
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

Feature/latest and active #1786

Merged
merged 8 commits into from
Sep 24, 2024
Merged

Feature/latest and active #1786

merged 8 commits into from
Sep 24, 2024

Conversation

miratepuffin
Copy link
Collaborator

@miratepuffin miratepuffin commented Sep 21, 2024

What changes were proposed in this pull request?

  1. Added a latest() function on all classes which equates to x.at(graph.latest_time)
  2. Exposed is_active on edge, edges, and node as a short hand to see if the entitiy has any updates within the window - this is not added only nodes as they are filtered through windows so it would basically always return true.
  3. Add event_graph function to Graph and persistent_graph function to PersistentGraph - these are basically just noops, but make it so in python you can call them without knowing what type you current have.

TODO:

  • Need to add all of the above to graphql

Why are the changes needed?

  • It is a massive pain (expecially in graphql) to always look up the current time or latest time in the graph
  • It can often be the case (say when doing a rolling) that you end up with an 'Empty' edge/node - this is done becuase dealing with Nones is a pain in the ass - so is_active() allows you to check this.

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

Yes and yes

How was this patch tested?

New tests

Issues

Created #1787

Are there any further changes required?

This has opened a couple of questions on if other filters should apply a window, and how should the DSL filtering be read.

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: f5885c2 Previous: b992030 Ratio
lotr_graph_subgraph_10pc/max_degree 48261 ns/iter (± 6862) 22968 ns/iter (± 1168) 2.10
lotr_graph_subgraph_10pc_materialise/materialize 825371 ns/iter (± 87894) 225687 ns/iter (± 4926) 3.66
lotr_graph_window_50_layered/max_id 49804 ns/iter (± 3959) 22137 ns/iter (± 1414) 2.25

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

Copy link
Collaborator

@ljeub-pometry ljeub-pometry left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -111,6 +111,14 @@ macro_rules! impl_timeops {
self.$field.at(time)
}

#[doc = concat!(r" Create a view of the ", $name, r" including all events at the latest time.")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

latest time in the Graph

@ljeub-pometry ljeub-pometry merged commit 5f0ae40 into master Sep 24, 2024
19 checks passed
@ljeub-pometry ljeub-pometry deleted the feature/latest_and_active branch September 24, 2024 11:06
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.

2 participants