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

a convenient way to copy arq-context including registries. #1374

Closed
sszuev opened this issue Jun 12, 2022 · 10 comments
Closed

a convenient way to copy arq-context including registries. #1374

sszuev opened this issue Jun 12, 2022 · 10 comments
Labels
enhancement Incrementally add new feature

Comments

@sszuev
Copy link
Contributor

sszuev commented Jun 12, 2022

Version

4.6.0

Suggestion

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

Are you interested in contributing a solution yourself?

Yes

@sszuev sszuev added the enhancement Incrementally add new feature label Jun 12, 2022
@afs
Copy link
Member

afs commented Jun 12, 2022

Hi @sszuev ,

A copy of the context will share the original registries - the way to modify that is to modify the copy of the context. I'd put that code in the PropertyFunctionRegistry, not "teach" context about a specific setting - moie discussion for the PR - the copy operations look useful.

The best control over a query setup is via the new (4.3.x) builders which allow a change of context: QueryExec for graph level, QueryExecution.create for model level (the associated builder also some legacy compatibility about setting timeouts).

        Graph graph = GraphFactory.createDefaultGraph();

        PropertyFunctionRegistry pfr = new PropertyFunctionRegistry();
        PropertyFunction pf = new version();
        pfr.put("urn:test:version", (x)->pf);

        Context cxt = ARQ.getContext().copy();
        PropertyFunctionRegistry.set(cxt, pfr);

        String qs = """
                SELECT * { ?x <urn:test:version> ?y }
                """;
        RowSet rowSet = QueryExec.graph(graph)
                .context(cxt)
                .query(qs)
                .select();
        RowSetOps.out(rowSet);

Unrelated: there is a problem with some system provided property functions. There is a hardwired translation in the code.

        Graph graph = GraphFactory.createDefaultGraph();
        Context cxt = ARQ.getContext().copy();    
        PropertyFunctionRegistry pfr = new PropertyFunctionRegistry();
        PropertyFunctionRegistry.set(cxt, pfr);
        String qs = """
                PREFIX afn:     <http://jena.apache.org/ARQ/function#>
                PREFIX apf:     <http://jena.apache.org/ARQ/property#>
                SELECT * { ?x apf:version ?y }
                """;
        RowSet rowSet = QueryExec.graph(graph)
                .context(cxt)
                .query(qs)
                .select();
        RowSetOps.out(rowSet);

doesn't work - apf:version is still a property function because afn and apf are hardwired in the registry code to map to "java:" URI which then will always happen. That's a bug.

@sszuev
Copy link
Contributor Author

sszuev commented Jun 12, 2022

There is also a problem (or feature) with the org.apache.jena.query.Query itself: it caches functions in thread-unsafe manner.
This was unexpected behavior for me, and for a while I didn't realize that my code was working incorrectly.

This one is also a reason (although weak) to have obvious way to create ready to use QueryExecution from scratch with specified context, from some well-known place.
But yes, we always can do the same things in our client-projects instead of patching Jena.
Anyway, I think tests provided by PR could be useful.

@afs
Copy link
Member

afs commented Jun 12, 2022

Where does Query cache functions? Do you mean in "E_Function"?

QueryExecution.create(...).context(cxt)... will use that and only that context.

QueryExecutionFactory provides common cases - and it had grown into a bit too large. The new-style builders are better for detailed setup.

@sszuev
Copy link
Contributor Author

sszuev commented Jun 12, 2022

Where does Query cache functions? Do you mean in "E_Function"?

Yes. org.apache.jena.sparql.expr.E_Function

QueryExecution.create(...).context(cxt)... will use that and only that context.

Ok, I see.

But replacing QueryExecutionFactory.create(query, data.getGraph(), context) (from PR) with the expression QueryExecution.create().query(query).model(data).context(context).build() causes test error (where query is String, not Query).
Here https://github.com/sszuev/jena/blob/main/jena-arq/src/test/java/org/apache/jena/query/TestQueryExecutionFactory.java#L89

Just found this, didn't dig yet.

QueryExecutionFactory provides common cases - and it had grown into a bit too large. The new-style builders are better for detailed setup.

Ok.

@afs
Copy link
Member

afs commented Jun 12, 2022

But replacing QueryExecutionFactory.create(query, data.getGraph(), context) (from PR) with the expression QueryExecution.create().query(query).model(data).context(context).build() causes test error (where query is String, not Query).

Visual inspection only but this looks odd. The PR new create(query, graph, context) uses DatasetGraphWrapper to apply the context. But I don't see this going to the QueryExecDatasetBuilder. Dataset contexts are usually added to the context being built for the query (Context.mergeCopy), not a replacement. DatasetGraphWrapper(,Context) hides the wrapped dataset's context.

@sszuev
Copy link
Contributor Author

sszuev commented Jun 13, 2022

But replacing QueryExecutionFactory.create(query, data.getGraph(), context) (from PR) with the expression QueryExecution.create().query(query).model(data).context(context).build() causes test error (where query is String, not Query).

Visual inspection only but this looks odd. The PR new create(query, graph, context) uses DatasetGraphWrapper to apply the context. But I don't see this going to the QueryExecDatasetBuilder. Dataset contexts are usually added to the context being built for the query (Context.mergeCopy), not a replacement. DatasetGraphWrapper(,Context) hides the wrapped dataset's context.

I think, QueryExecutionFactory.create(query, data.getGraph(), context) can be removed from PR, and new bug for QueryExecution.create().query(query).model(data).context(context).build() with tests attached can be started.

@afs ?

@afs
Copy link
Member

afs commented Jun 13, 2022

Seems reasonable.

@sszuev
Copy link
Contributor Author

sszuev commented Jun 13, 2022

A copy of the context will share the original registries - the way to modify that is to modify the copy of the context. I'd put that code in the PropertyFunctionRegistry, not "teach" context about a specific setting - moie discussion for the PR - the copy operations look usefu

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

In the PR, 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
Copy link
Member

afs commented Jun 14, 2022

ContextUtils makes sense.

If it were each registry, I'd put the code there but to have a function that all three at once is better.

@sszuev sszuev changed the title a convenient way to execute a query against graph in a dedicated context. a convenient way to copy arq-context including registries. Jul 8, 2022
afs added a commit that referenced this issue Jul 8, 2022
GH-1374: add copyWithRegisties Context helper method
@afs
Copy link
Member

afs commented Jul 8, 2022

Closed by #1375.

@afs afs closed this as completed Jul 8, 2022
Aklakan pushed a commit to Aklakan/jena that referenced this issue Jul 10, 2022
…xecutionFactory#create(Query, Graph, Context) helpers
Aklakan pushed a commit to Aklakan/jena that referenced this issue Jul 11, 2022
…xecutionFactory#create(Query, Graph, Context) helpers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Incrementally add new feature
Projects
None yet
Development

No branches or pull requests

2 participants