a convenient way to copy arq-context including registries. about jena HOT 10 CLOSED

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.

from jena.

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.

from jena.

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.

from jena.

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.

from jena.

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.

from jena.

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 ?

from jena.

Seems reasonable.

from jena.

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.

from jena.

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.

from jena.

Closed by #1375.

from jena.