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/17 10:27:28 UTC
[GitHub] [jena] afs commented on a diff in pull request #1620: Allow toggling extended path flattening on
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