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

GH-1374: add copyWithRegisties Context helper method #1375

Merged
merged 1 commit into from Jul 8, 2022
Merged

GH-1374: add copyWithRegisties Context helper method #1375

merged 1 commit into from Jul 8, 2022

Conversation

sszuev
Copy link
Contributor

@sszuev sszuev commented Jun 12, 2022

Feature request: a convenient way to execute a query against graph in a dedicated context.

If we want to execute a query in isolated context with unique set of independent functions and p-functions,
we will encounter some difficulties:

  • the method org.apache.jena.sparql.util.Context#copy performs only shallow copying, not deep
  • there is no straightforward way to pass the context while constructing org.apache.jena.query.QueryExecution

So we have to spend some time to find the proper way.
I think this can be simplified by adding two new helper-methods:

  • Context#copyWithFunctionRegistries(Context) - the method to create a copy of Context with copied function and p-function registries
  • QueryExecutionFactory#create(Query, Graph, Context) - the method to create QueryExceution for a Graph with the given context

  • Tests are included.
  • Documentation change and updates are provided for the Apache Jena website
  • Commits have been squashed to remove intermediate development commit messages.
  • Key commit messages start with the issue number (GH-xxxx or JENA-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.


See the Apache Jena "Contributing" guide.

@afs afs changed the title GH-1374: add copiyng Context helper method and new QueryExcecution factory method GH-1374: add copying Context helper method and new QueryExcecution factory method Jun 12, 2022
@afs afs self-requested a review June 12, 2022 10:21
@Aklakan
Copy link
Contributor

Aklakan commented Jun 13, 2022

I suppose the ARQ method to copy the function registries should eventually also copy the service executor registries (one is already available and another for bulk requests is in the works) - they are typically very small and I don't think it makes sense to exclude those when one explicitly asks for a deep copy of the context in order to add isolated extensions.

Maybe instead of naming the method copyWithFunctionRegistries it should be
more generally copyWithRegistries? (Alternatively something like copyWith{User,Plugin,Extension,...}Registries).
So the naming should be clarified with the service executor registry in mind. Are there any further plugin registries in the context by default that would benefit from deep copying too?

For consistency, the ServiceExecutorRegistry registry should provide a createFrom method too but that I can add with a separate PR.

Comment on lines 88 to 87
PropertyFunctionRegistry res = new PropertyFunctionRegistry();
if (from == null) {
return res;
}
res.registry.putAll(from.registry);
return res;
Copy link
Contributor

@Aklakan Aklakan Jun 13, 2022

Choose a reason for hiding this comment

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

Redundant return can be removed if written that way:

        if (from != null) {
            res.registry.putAll(from.registry);
        }
        return res;

@sszuev
Copy link
Contributor Author

sszuev commented Jun 13, 2022

ServiceExecutorRegistry

I suppose the ARQ method to copy the function registries should eventually also copy the service executor registries (one is already available and another for bulk requests is in the works) - they are typically very small and I don't think it makes sense to exclude those when one explicitly asks for a deep copy of the context in order to add isolated extensions.

Maybe instead of naming the method copyWithFunctionRegistries it should be more generally copyWithRegistries? (Alternatively something like copyWith{User,Plugin,Extension,...}Registries). So the naming should be clarified with the service executor registry in mind. Are there any further plugin registries in the context by default that would benefit from deep copying too?

For consistency, the ServiceExecutorRegistry registry should provide a createFrom method too but that I can add with a separate PR.

I think this is a good idea.

@afs
Copy link
Member

afs commented Jun 13, 2022

I suppose the ARQ method to copy the function registries should eventually also copy the service executor registries

Discussion on #1374.

Context should be intent neutral and not know the value of any symbols.

@sszuev
Copy link
Contributor Author

sszuev commented Jun 13, 2022

Context should be intent neutral and not know the value of any symbols.

What is appropriate place to such functionality ?
org.apache.jena.sparql.util.Contexts ?
org.apache.jena.sparql.util.ContextUtils ?

This is static helper, instance of Context itself does not refer to any particular symbol.
There are already static helpers like fromDataset(DatasetGraph).
I don't quite understand why import org.apache.jena.sparql.core.DatasetGraph is ok, but import org.apache.jena.sparql.function.FunctionRegistry is not?
If we move it to some other place it would be harder to find it.

Maybe, in the case of ContextUtils, all other static-helpers should also be moved to that new class (with duplication and @Deprecate for original methods). Such a way makes more sense for me, then the helper-class for a single helper-method.

@afs afs mentioned this pull request Jun 13, 2022
@afs
Copy link
Member

afs commented Jun 14, 2022

ContextUtils makes sense even if in sparql.util

Context is also used in RIOT - it was originally query-specific and mostly internal use but having such a mechanism, it get reused and escaped.

@sszuev sszuev changed the title GH-1374: add copying Context helper method and new QueryExcecution factory method GH-1374: add copyWithRegisties Context helper method Jun 15, 2022
@sszuev
Copy link
Contributor Author

sszuev commented Jun 17, 2022

ContextUtils makes sense even if in sparql.util

Context is also used in RIOT - it was originally query-specific and mostly internal use but having such a mechanism, it get reused and escaped.

Is there something else I can do in additional in the bounds of this PR ? Maybe marking deprecated all other static-helpers of Context with replacing and moving?

@afs
Copy link
Member

afs commented Jun 17, 2022

Is there something else I can do in additional in the bounds of this PR ?

Thanks for asking.

My first step is to look at #1381 to understand that - and it does currently look like there is a bug managing contexts in QueryExec. A fix may well have knock on-effects.

At the moment, there are several active and time-consuming PRs in flight in unrelated areas.

…onFactory#create(Query, Graph, Context) helpers
Copy link
Member

@afs afs left a comment

Choose a reason for hiding this comment

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

Only thing is {@code null) => {@code null} each time in javadoc but I'll fix that.

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.

None yet

3 participants