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

Add optional graph argument to Graph.cbd, use for DESCRIBE queries #2322

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

mgberg
Copy link
Contributor

@mgberg mgberg commented Mar 27, 2023

Summary of changes

I often find myself wanting to create subgraphs including descriptions of multiple resources, so I end up doing some pattern like this:

src_graph: Graph  # A large graph for which I want a subgraph
out_graph = Graph()
for uri in resources:
    out_graph += src_graph.cbd(uri)

While this works fine, there is overhead every time cbd is called:

  1. A new Graph object is created each time.
  2. Once triples are added to that new graph, they must be copied over to out_graph, i.e. added to a graph a second time.

This PR adds an optional target_graph argument to the cbd method. If a Graph is provided for that argument, the computed CBD is added directly to that provided graph instead of creating a new graph. Consequently, the above snippet could be written like this to avoid that extra overhead:

src_graph: Graph  # A large graph for which I want a subgraph
out_graph = Graph()
for uri in resources:
    src_graph.cbd(uri, out_graph)

If the target_graph argument is not provided, then it behaves the same as it currently does- it should not break any existing code.

One place where this situation occurs is in the SPARQL DESCRIBE implementation. This PR tweaks that implementation to use this new argument to improve performance.

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • For changes that have a potential impact on users of this project:
    • Updated relevant documentation to avoid inaccuracies.
    • Considered adding additional documentation.
    • Considered adding an example in ./examples for new features.
    • Considered updating our changelog (CHANGELOG.md).
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@coveralls
Copy link

coveralls commented Mar 27, 2023

Coverage Status

Coverage: 90.839% (+0.003%) from 90.836% when pulling cc1e3f5 on corning-incorporated:add-graph-arg-to-cbd into 081a974 on RDFLib:main.

@aucampia
Copy link
Member

@mgberg this seems pretty useful. Another way to approach this may be to extract something which computes CBDs into a separate function freestanding, function, and then have that operate on a source and target graph. The benefit there would be that the graph interface remains smaller.

I will think about it a bit.

@westurner @niklasl any feedback from you would be appreciated here.

@westurner
Copy link
Contributor

Maybe def cbd(self, *, target_graph=None)?

@aucampia
Copy link
Member

aucampia commented Apr 9, 2023

@mgberg it is slightly better to make pull requests from your personal account if you are making them across organizations, that way we can update your PR branch directly with suggestions.

The pull requests mostly looks fine, but I think making target_graph a keyword only argument is probably best.

I will make a PR against your branch doing that and adding some unit tests.

Also added some tests for `Graph.cbd` with `target_graph`
@aucampia
Copy link
Member

aucampia commented Apr 9, 2023

@mgberg it is slightly better to make pull requests from your personal account if you are making them across organizations, that way we can update your PR branch directly with suggestions.

The pull requests mostly looks fine, but I think making target_graph a keyword only argument is probably best.

I will make a PR against your branch doing that and adding some unit tests.

PR here: corning-incorporated#2

…-to-cbd

Make `target_graph` a keyword only arg
@mgberg
Copy link
Contributor Author

mgberg commented Apr 10, 2023

@mgberg it is slightly better to make pull requests from your personal account if you are making them across organizations, that way we can update your PR branch directly with suggestions.

I'll do that in the future!

Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

Change looks good to me, thanks @mgberg

@aucampia aucampia added ready to merge The PR will be merged soon if no further feedback is provided. review wanted This indicates that the PR is ready for review labels Apr 11, 2023
@aucampia aucampia requested a review from a team April 11, 2023 17:47
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR will be merged soon if no further feedback is provided. review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants