Skip to content

Graphql refactor + rolling/expanding #2090

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

Merged
merged 45 commits into from
May 28, 2025
Merged

Conversation

miratepuffin
Copy link
Collaborator

@miratepuffin miratepuffin commented May 9, 2025

What changes were proposed in this pull request?

Removed functions/API changes

  • Removed _id functions from graphql as they are no longer needed
  • Removed GqlGraphs as we now have Namespace support
  • Added Namespaces and MetaGraphs so that large collections of graphs/namespaces can be paged/listed/counted

New functions

  • Added rolling and expanding to Graph, Node, Nodes, PathFromNode, Edge and Edges

Refactor

  • Renamed all graphql structs that started with GQL to make the user facing schema a bit cleaner
  • Started running queries in blocking threads to improve multi query execution and ui interactivity for large graphs

Bug fixes

  • Fixed a problem with namespaces returning loads of null paths and not returning root
  • Fixed issue with recursive writing of indexes causing the server to crash
  • changed caching policy to never kick out graphs after some timeout by default
  • Changed WindowSet to not allow zero size step
  • Fixed issue in rolling where if the step was bigger than the window size the final window would be empty.

Testing

  • Added namespaces tests to check for correct output
  • Added better testing for namespaces and metagraphs
  • Need to add tests for GQL rollings/expanding

Why are the changes needed?

Improvements in the UI timeline and also useful for Graphql users.
Multiple queries being blocked as queries were being run on the main thread.

Are there any further changes required?

The spawn blocking is probably more of a patch than a final solution, but we need to get some benchmarks going first to know what works best.

# Conflicts:
#	python/tests/test_base_install/test_filters/semantics/test_edge_property_filter_semantics.py
#	python/tests/test_base_install/test_filters/semantics/test_node_property_filter_semantics.py
#	python/tests/test_base_install/test_filters/test_edge_composite_filter.py
#	python/tests/test_base_install/test_filters/test_edge_filter.py
#	python/tests/test_base_install/test_filters/test_edge_property_filter.py
#	python/tests/test_base_install/test_filters/test_node_composite_filter.py
#	python/tests/test_base_install/test_filters/test_node_filter.py
#	python/tests/test_base_install/test_filters/test_node_property_filter.py
@miratepuffin miratepuffin changed the title initial graph and node version Graphql refactor + rolling/expanding May 27, 2025
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.

Some things to tidy up

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.

Some things we can fix later, doesn't really introduce new issues.

self_clone.graph.write_updates()?;
let self_clone_2 = self.clone();

let nodes: Vec<Result<NodeView<GraphWithVectors>, GraphError>> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should just change the type here to a HashSet instead to fix the FIXME below :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also swap the Result and Vec to get early failure? Right now it will run through all the updates even if there is failures and add everything that works (maybe that is what we want?) but then the embedding part will fail early on the first broken update which doesn't make sense if we did actually want the current behaviour. We probably actually want to return the list of all failures in this case.

Comment on lines +395 to +411
impl<'graph, T: TimeOps<'graph> + Clone + 'graph> ExactSizeIterator for WindowSet<'graph, T> {
//unfortunately because Interval can change size, there is no nice divide option
fn len(&self) -> usize {
let mut cursor = self.cursor;
let mut count = 0;
while cursor < self.end + self.step {
let window_start = self.window.map(|w| cursor - w);
if let Some(start) = window_start {
if start >= self.end {
break;
}
}
count += 1;
cursor = cursor + self.step;
}
count
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we can't do better, we shouldn't implement this as it is no better than count on the iterator but I think we could do better (the months would need some special handling). At least implement the fast option when the interval is simple?

@miratepuffin miratepuffin merged commit c8f2a0f into master May 28, 2025
28 checks passed
@miratepuffin miratepuffin deleted the feature/rolling_graphql branch May 28, 2025 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.

3 participants