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/11/16 16:07:47 UTC
[GitHub] [jena] rvesse opened a new pull request, #1620: Allow toggling extended path flattening on
rvesse opened a new pull request, #1620:
URL: https://github.com/apache/jena/pull/1620
Intended as a less invasive first step related to #1616
Adds a new ARQ symbol `optPathFlattenExtended` that allows enabling more comprehensive path flattened transform via existing code `TransformPathFlatternStd`. When enabled this is used in preference to the existing simpler path flattening transform.
This remains disabled by default as it can generate algebra structures that are less performant for the general case. However users might have use cases that benefit from this so this now allows opting into it without needing deeper modification of ARQs optimiser.
----
- [x] Tests are included.
- [ ] Documentation change and updates are provided for the [Apache Jena website](https://github.com/apache/jena-site/)
- [x] 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] SimonBin commented on pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
SimonBin commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1318898910
just don't forget that even with this option, ungrounded paths (that were not flattened) still don't respect limit and timeout
--
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] rvesse commented on a diff in pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
rvesse commented on code in PR #1620:
URL: https://github.com/apache/jena/pull/1620#discussion_r1025030591
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java:
##########
@@ -35,8 +35,15 @@
/** The path transformation step exactly as per the SPARQL 1.1 spec.
* i.e. joins triples rather creating BGPs.
+ * <p>
* It does not produce very nice execution structures so ARQ uses
- * a functional equivalent, but different, transformation.
+ * a functional equivalent, but different, transformation, see {@link TransformPathFlattern}
+ * </p>
+ * <p>
+ * However for users who are using property paths in their queries heavily there may be benefits to using this
+ * transform over the simpler one. The {@link org.apache.jena.query.ARQ#optPathFlattenExtended} symbol can be set in
+ * an ARQ context to enable this transform in preference to the simpler transform.
+ * </p>
Review Comment:
Interestingly the `:p{2}` case actually looks to be expanded the wrong way round (at least to my eye) in the current code. It produces the following:
```
(prefix ((: <http://example/>))
(sequence
(path ??P0 (pathN* :p) :T1)
(bgp
(triple ?x :p ??P1)
(triple ??P1 :p ??P0)
)))
```
When I'd expect to see the `bgp` first followed by the `path` operator
--
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] SimonBin commented on pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
SimonBin commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1319027463
yes, see e.g the work Lorenz started here https://github.com/AKSW/jena/commit/061b7c90b36a64a0c700978007fd5340c34650d8
--
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 a diff in pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
afs commented on code in PR #1620:
URL: https://github.com/apache/jena/pull/1620#discussion_r1025068012
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java:
##########
@@ -35,8 +35,15 @@
/** The path transformation step exactly as per the SPARQL 1.1 spec.
* i.e. joins triples rather creating BGPs.
+ * <p>
* It does not produce very nice execution structures so ARQ uses
- * a functional equivalent, but different, transformation.
+ * a functional equivalent, but different, transformation, see {@link TransformPathFlattern}
+ * </p>
+ * <p>
+ * However for users who are using property paths in their queries heavily there may be benefits to using this
+ * transform over the simpler one. The {@link org.apache.jena.query.ARQ#optPathFlattenExtended} symbol can be set in
+ * an ARQ context to enable this transform in preference to the simpler transform.
+ * </p>
Review Comment:
Looks like that the bound object and unbound subject isn't ideal.
It would be better to 2-step with bound object first.
--
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 a diff in pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
afs commented on code in PR #1620:
URL: https://github.com/apache/jena/pull/1620#discussion_r1024931810
##########
jena-arq/src/test/java/org/apache/jena/sparql/algebra/optimize/TestTransformPathFlatten.java:
##########
@@ -112,15 +169,23 @@ private static Op op(String...opStr) {
}
private static void test(Op opInput, Op opExpected) {
- Op op = Transformer.transform(new TransformPathFlattern(), opInput);
+ testPathTransform(opInput, opExpected, new TransformPathFlattern());
+ }
+
+ private static void testPathTransform(Op opInput, Op opExpected, Transform transform) {
+ Op op = Transformer.transform(transform, opInput);
if ( opExpected == null ) {
System.out.print(opInput);
System.out.println(" ==>");
System.out.print(op);
System.out.println();
return;
}
-
+
assertEquals(opExpected, op);
}
+
+ private static void testExtended(Op opInput, Op opExpected) {
Review Comment:
`extended` is a bit confusing as they are both in flux.
How about one of:
* "Alternative" (despite the name clash with `|`)
* "Algebra" because it is more directly algebra related than the other one.
* "test2"
This then impacts the test names.
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java:
##########
@@ -35,8 +35,15 @@
/** The path transformation step exactly as per the SPARQL 1.1 spec.
* i.e. joins triples rather creating BGPs.
+ * <p>
* It does not produce very nice execution structures so ARQ uses
- * a functional equivalent, but different, transformation.
+ * a functional equivalent, but different, transformation, see {@link TransformPathFlattern}
+ * </p>
+ * <p>
+ * However for users who are using property paths in their queries heavily there may be benefits to using this
+ * transform over the simpler one. The {@link org.apache.jena.query.ARQ#optPathFlattenExtended} symbol can be set in
+ * an ARQ context to enable this transform in preference to the simpler transform.
+ * </p>
Review Comment:
simpler or complex is about the current implementations so this comment can easily become out-of-date.
An example of alt transformation didn't make it into the [path transformations section](https://www.w3.org/TR/sparql11-query/#propertypath-syntaxforms) in the SPARQL 1.1 spec. So the "Std" isn't accurate.
With PR #1616 adding alt that isn't really a case of simple/complex anymore. The other transform includes `:p{2,}` to `:p{2}/:p*` with the `:p{2}` becoming a BGP for example.
I suggest just saying this is a different set of transformations based on the algebra for now.
As PR #1616 currently introduces a third way to optimize, there needs to be some code sorting out and the comments can be upgraded then.
The tests will help with that.
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/OptimizerStd.java:
##########
@@ -71,7 +71,11 @@ public Op rewrite(Op op) {
// Convert paths to triple patterns if possible.
if ( context.isTrueOrUndef(ARQ.optPathFlatten) ) {
- op = apply("Path flattening", new TransformPathFlattern(), op) ;
+ if (context.isTrue(ARQ.optPathFlattenExtended)) {
+ op = apply("Path flattening (extended)", new TransformPathFlatternStd(), op);
Review Comment:
See other comments about "extended" -- the difference is more in style than simple/complex.
In the overall work on this area, may be we should rename it as the more appropriate `TransformPathFlattenAlgebra`.
##########
jena-arq/src/test/java/org/apache/jena/sparql/algebra/optimize/TestTransformPathFlatten.java:
##########
@@ -98,6 +99,62 @@ public class TestTransformPathFlatten {
," ))");
test(op1, op2);
}
+
+ @Test public void pathFlatten_alt_01() {
+ Op op1 = path("?x", ":p1|:p2", ":T1");
+ // Basic flatten does not flatten alternative paths
+ test(op1, op1);
+ }
+
+ @Test public void pathFlatten_alt_02() {
+ Op op1 = path("?x", ":p1|:p2", ":T1");
+ // Extended flatten does flatten alternative paths
+ Op expected = op(
+ "(union",
+ " (triple ?x :p1 :T1)",
+ " (triple ?x :p2 :T1)",
+ ")");
+ testExtended(op1, expected);
+ }
+
+ @Test public void pathFlatten_alt_03() {
+ Op op1 = path("?x", ":p1|^:p2", ":T1");
+ // Extended flatten does flatten alternative paths
+ Op expected = op(
+ "(union",
+ " (triple ?x :p1 :T1)",
+ " (triple :T1 :p2 ?x)",
+ ")");
+ testExtended(op1, expected);
+ }
+
+ @Test public void pathFlatten_alt_04() {
+ Op op1 = path("?x", ":p1|:p2|(:p3*)", ":T1");
+ // Extended flatten does flatten alternative paths
+ Op expected = op(
+ "(union",
+ " (union",
+ " (triple ?x :p1 :T1)",
+ " (triple ?x :p2 :T1))",
+ " (path ?x (path* :p3) :T1)",
+ ")");
+ testExtended(op1, expected);
+ }
+
+ @Test public void pathFlatten_alt_05() {
+ Op op1 = path("?x", ":p1|:p2|(:p3{2})", ":T1");
+ // Extended flatten does flatten alternative paths
+ Op expected = op(
+ "(union",
+ " (union",
+ " (triple ?x :p1 :T1)",
+ " (triple ?x :p2 :T1))",
+ " (join",
+ " (triple ?x :p3 ??Q0)",
+ " (triple ??Q0 :p3 :T1))",
+ ")");
+ testExtended(op1, expected);
+ }
Review Comment:
Please add a test for ":p{2,})" - which doesn't get changed in this transform but does in the other and could here. While it's missing, the code looks like could and should can do it.
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java:
##########
@@ -35,8 +35,15 @@
/** The path transformation step exactly as per the SPARQL 1.1 spec.
* i.e. joins triples rather creating BGPs.
Review Comment:
That's no longer the whole story with the change to `TransformMergeBGPs`. Maybe add a mention of `TransformMergeBGPs`.
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformMergeBGPs.java:
##########
@@ -106,6 +107,8 @@ public Op transform(OpSequence opSequence, List<Op> elts) {
private static BasicPattern asBGP(Op op) {
if ( op instanceof OpBGP )
return ((OpBGP)op).getPattern() ;
+ if ( op instanceof OpTriple)
Review Comment:
+1
--
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 #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1318928539
> still don't respect limit and timeout
The description explains:
_Intended as a less invasive first step related to https://github.com/apache/jena/pull/1616_
This PR is about enabling the transformation.
I read "first step" as acknowledging that Limit and timeout come from PathLib which is at execution time.
--
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 #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1318673891
cc @SimonBin @Aklakan @LorenzBuehmann
--
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 #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
afs merged PR #1620:
URL: https://github.com/apache/jena/pull/1620
--
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 #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1319023795
It's not the only way to address the double unbound situation:
In `determineUngroundedStartingSet` add
```
if (path instanceof P_Alt ) {
Iterator<Node> x = subjectsOfPath(graph, ((P_Path2)path).getLeft(), execCxt);
if ( x == null )
x = subjectsOfPath(graph, ((P_Path2)path).getRight(), execCxt);
if ( x != null )
return x;
}
...
```
where `subjectsOfPath` handles `P_Link` and does `graph.find(_, property, _)`
(this is not the only way - it is just an example)
--
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] rvesse commented on a diff in pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
rvesse commented on code in PR #1620:
URL: https://github.com/apache/jena/pull/1620#discussion_r1025071677
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java:
##########
@@ -35,8 +35,15 @@
/** The path transformation step exactly as per the SPARQL 1.1 spec.
* i.e. joins triples rather creating BGPs.
+ * <p>
* It does not produce very nice execution structures so ARQ uses
- * a functional equivalent, but different, transformation.
+ * a functional equivalent, but different, transformation, see {@link TransformPathFlattern}
+ * </p>
+ * <p>
+ * However for users who are using property paths in their queries heavily there may be benefits to using this
+ * transform over the simpler one. The {@link org.apache.jena.query.ARQ#optPathFlattenExtended} symbol can be set in
+ * an ARQ context to enable this transform in preference to the simpler transform.
+ * </p>
Review Comment:
Yeah I'm leaning towards that too.
Generally seems like we need a lot more test cases around this area and some fixing up for these kind of quirks
--
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] rvesse commented on a diff in pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
rvesse commented on code in PR #1620:
URL: https://github.com/apache/jena/pull/1620#discussion_r1025032619
##########
jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java:
##########
@@ -35,8 +35,15 @@
/** The path transformation step exactly as per the SPARQL 1.1 spec.
* i.e. joins triples rather creating BGPs.
+ * <p>
* It does not produce very nice execution structures so ARQ uses
- * a functional equivalent, but different, transformation.
+ * a functional equivalent, but different, transformation, see {@link TransformPathFlattern}
+ * </p>
+ * <p>
+ * However for users who are using property paths in their queries heavily there may be benefits to using this
+ * transform over the simpler one. The {@link org.apache.jena.query.ARQ#optPathFlattenExtended} symbol can be set in
+ * an ARQ context to enable this transform in preference to the simpler transform.
+ * </p>
Review Comment:
Although digging into the code some more looks like it makes the decision based on which end of the path has a fixed value so maybe this is correct
--
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] SimonBin commented on pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
SimonBin commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1318930517
my personal opinion is still that P_Alt should be optimised by default but any improvement is a good thing :-)
--
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 #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
afs commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1318495640
@rvesse - could you add this one-line fix to the PR please: I just ran across the exception being raised when it shouldn't.
The optimize doesn't do anything in the case of `{2,}` but that's another issue.
First, no exception.
```
diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java
index fe847fba79..8a19be4a50 100644
--- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java
+++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformPathFlatternStd.java
@@ -169,7 +169,7 @@ public class TransformPathFlatternStd extends TransformCopy {
@Override
public void visit(P_Mod pathMod) {
- if ( pathMod.getMin() > pathMod.getMax() )
+ if ( pathMod.getMax() > 0 && pathMod.getMin() > pathMod.getMax() )
throw new ARQException("Bad path: " + pathMod);
Op op = null;
```
--
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] rvesse commented on pull request #1620: Allow toggling extended path flattening on
Posted by GitBox <gi...@apache.org>.
rvesse commented on PR #1620:
URL: https://github.com/apache/jena/pull/1620#issuecomment-1318520668
> @rvesse - could you add this one-line fix to the PR please: I just ran across the exception being raised when it shouldn't. The optimize doesn't do anything in the case of `{2,}` but that's another issue. First, no exception.
Yes, both things addressed in my latest commit
--
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