You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by GitBox <gi...@apache.org> on 2022/06/12 06:36:08 UTC
[GitHub] [jena] sszuev opened a new pull request, #1375: GH-1374: add copiyng Context helper method and new QueryExcecution factory method
sszuev opened a new pull request, #1375:
URL: https://github.com/apache/jena/pull/1375
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](https://github.com/apache/jena-site/)
- [ ] 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](https://www.apache.org/licenses/contributor-agreements.html).
----
See the [Apache Jena "Contributing" guide](https://github.com/apache/jena/blob/main/CONTRIBUTING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] Aklakan commented on a diff in pull request #1375: GH-1374: add copying Context helper method and new QueryExcecution factory method
Posted by GitBox <gi...@apache.org>.
Aklakan commented on code in PR #1375:
URL: https://github.com/apache/jena/pull/1375#discussion_r895257601
##########
jena-arq/src/main/java/org/apache/jena/sparql/pfunction/PropertyFunctionRegistry.java:
##########
@@ -77,7 +78,20 @@ public synchronized static PropertyFunctionRegistry get()
}
return reg ;
}
-
+
+ /**
+ * Copies the origin registry into a new one, or makes a fresh instance if the specified registry is {@code null).
+ * @param from {@link PropertyFunctionRegistry} or {@code null}
+ * @return {@code PropertyFunctionRegistry} a new instance
+ */
+ public static PropertyFunctionRegistry createFrom(PropertyFunctionRegistry from) {
+ PropertyFunctionRegistry res = new PropertyFunctionRegistry();
+ if (from == null) {
+ return res;
+ }
+ res.registry.putAll(from.registry);
+ return res;
Review Comment:
Redundant return can be removed if written that way:
```
if (from != null) {
res.registry.putAll(from.registry);
}
return res;
```
##########
jena-arq/src/main/java/org/apache/jena/sparql/pfunction/PropertyFunctionRegistry.java:
##########
@@ -77,7 +78,20 @@ public synchronized static PropertyFunctionRegistry get()
}
return reg ;
}
-
+
+ /**
+ * Copies the origin registry into a new one, or makes a fresh instance if the specified registry is {@code null).
+ * @param from {@link PropertyFunctionRegistry} or {@code null}
+ * @return {@code PropertyFunctionRegistry} a new instance
+ */
+ public static PropertyFunctionRegistry createFrom(PropertyFunctionRegistry from) {
+ PropertyFunctionRegistry res = new PropertyFunctionRegistry();
+ if (from == null) {
+ return res;
+ }
+ res.registry.putAll(from.registry);
+ return res;
Review Comment:
Redundant return can be removed if written that way:
```java
if (from != null) {
res.registry.putAll(from.registry);
}
return res;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] Aklakan commented on pull request #1375: GH-1374: add copying Context helper method and new QueryExcecution factory method
Posted by GitBox <gi...@apache.org>.
Aklakan commented on PR #1375:
URL: https://github.com/apache/jena/pull/1375#issuecomment-1153336489
I suppose the ARQ method to copy the function registries should eventually also copy the service executor registries (one is already [available](https://github.com/apache/jena-site/blob/main/source/documentation/query/custom_service_executors.md) and another for bulk requests is [in the works](https://github.com/apache/jena/issues/1314)) - 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 service executor registry in mind.
For consistency, the `ServiceExecutorRegistry` registry should provide a `createFrom` method too but that I can add with a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1375: GH-1374: add copying Context helper method and new QueryExcecution factory method
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1375:
URL: https://github.com/apache/jena/pull/1375#issuecomment-1154091290
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs merged pull request #1375: GH-1374: add copyWithRegisties Context helper method
Posted by GitBox <gi...@apache.org>.
afs merged PR #1375:
URL: https://github.com/apache/jena/pull/1375
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1375: GH-1374: add copying Context helper method and new QueryExcecution factory method
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1375:
URL: https://github.com/apache/jena/pull/1375#issuecomment-1155532416
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1375: GH-1374: add copyWithRegisties Context helper method
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1375:
URL: https://github.com/apache/jena/pull/1375#issuecomment-1159024589
>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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] sszuev commented on pull request #1375: GH-1374: add copyWithRegisties Context helper method
Posted by GitBox <gi...@apache.org>.
sszuev commented on PR #1375:
URL: https://github.com/apache/jena/pull/1375#issuecomment-1158581322
> `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 all other static-helpers of `Context` with replacing and moving?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] sszuev commented on pull request #1375: GH-1374: add copying Context helper method and new QueryExcecution factory method
Posted by GitBox <gi...@apache.org>.
sszuev commented on PR #1375:
URL: https://github.com/apache/jena/pull/1375#issuecomment-1153881019
> ServiceExecutorRegistry
> I suppose the ARQ method to copy the function registries should eventually also copy the service executor registries (one is already [available](https://github.com/apache/jena-site/blob/main/source/documentation/query/custom_service_executors.md) and another for bulk requests is [in the works](https://github.com/apache/jena/issues/1314)) - 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] sszuev commented on pull request #1375: GH-1374: add copying Context helper method and new QueryExcecution factory method
Posted by GitBox <gi...@apache.org>.
sszuev commented on PR #1375:
URL: https://github.com/apache/jena/pull/1375#issuecomment-1154119286
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org