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

Post cache graphql fixes #1709

Merged
merged 17 commits into from
Aug 19, 2024
Merged

Post cache graphql fixes #1709

merged 17 commits into from
Aug 19, 2024

Conversation

miratepuffin
Copy link
Collaborator

@miratepuffin miratepuffin commented Aug 8, 2024

What changes were proposed in this pull request?

  • I think we're using the wrong kind of Result in this file, probably Result<_, GraphError> is more appropriate
  • GqlGraph doesn't need a name it can be created from the Path
  • save_graphs_to_work_dir can be moved into the test module
  • get_graph_name needs to user .display.to_string to remove the Result and swap to file_stem to remove the file extension
  • Any references to work_dir inside of Data can just used the stored value
  • On currently dead load_graph_from_path add the following comments
    • Can we change this to an Option which would also allow renaming of the graph on copy - for namespace
    • We can remove is_disk_graph_dir as we check if the path is a valid diskgraph within load_disk_graph
  • On currently dead load_disk_graph_from_path need to change the output to just Result
  • update get_graph as per @fabianmurariu comment
    • I think you want to call get_with(&name, || get_graph_from_path..) here this will make sure you only load the graph once when called by multiple threads
  • Is there a nicer way to check for Path correcteness within contract_graph_full_path

as per #1705

Why are the changes needed?

Required as part of the continued cleanup of the graphql server

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

No, were changed previously

How was this patch tested?

Via current and new unit tests

Are there any further changes required?

Yes see issue

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.

I think name on DiskGraph is currently returning the full path instead of only the last directory, add a test and fix

python/tests/test_graphql.py Show resolved Hide resolved
raphtory-graphql/src/data.rs Outdated Show resolved Hide resolved
raphtory-graphql/src/data.rs Outdated Show resolved Hide resolved
raphtory-graphql/src/data.rs Outdated Show resolved Hide resolved
raphtory-graphql/src/model/graph/graph.rs Show resolved Hide resolved
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

@miratepuffin miratepuffin merged commit 64a8223 into master Aug 19, 2024
19 checks passed
@miratepuffin miratepuffin deleted the fix/graphql branch August 19, 2024 11:31
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