On 25/11/13 07:03, Claude Warren wrote:
Andy,
My test with the bound was removed and I don't see one with the bound
included. I think the bound was what triggered the issue in the case I was
attempting to fix, because the bound creates a new triple block.
Do you think this is not important to test or am I missing a test that
applies in this case?
Claude,
Your fixed worked in some cases but not other - you make have seen it as
a programming bug when in fact it was a design error.
I put in several tests to cover the initial situation of your test case
and also other situations that I could see arising which were related
but not part of the test case. I hope there is now full coverage. I
did simpify tests, taking out long URIs that only add chars to the test
but don't change the thing being tested. Ditto filter expression (if)
which is not relevant here - I wanted to leave behind a focused set of
tests and I feel that reducing them down to the core issue means that
someone looking at them later will find it easier. That's the style of
the test class.
If you want to add back your test, please do so. The essence is covered
by place_extend_05, one of several tests I added or modified.
The core issue was that the expressions that were potential candidates
for pushing down into the expression below (extend). It is not to do
with (sequence), although place_extend_05 covers that, nor the nature of
the FILTER expression (if).
It is wrong to put in the
OpFilter.filter(pushed, ...)
wrapper in either of the two code routes after
Placement p = transform(pushed, opSub) ;
If all potential candidate expressions are not processed, that returns
null, nothing was processed and the correct result is to return null
indicating no processing done.
The other case is when some, but not all expressions are successfully
pushed inwards. This is a new situation outside the test cases.
The unprocessed expressions need to be left outside the (extend) just
like the "unpushed" collection.
At the top of the method we have:
for ( Expr expr : exprs ) {
Set<Var> exprVars = expr.getVarsMentioned() ;
if ( disjoint(vars1, exprVars) )
pushed.add(expr);
else
unpushed.add(expr) ;
}
where if disjoint an expr goes into 'pushed' and into 'unpushed' otherwise.
You added after the recursive step:
pushed = new ExprList() ;
for ( Expr expr : p.unplaced ) {
Set<Var> exprVars = expr.getVarsMentioned() ;
if ( disjoint(vars1, exprVars) )
unpushed.add(expr);
else
pushed.add(expr) ;
}
which has the 'pushed' and 'unpush' swapped.
Note that 'p.unplaced' can only contain elements from the initial
'pushed' - by swapping the arms of the "if", you can see everything from
p.unplaced must be added to "unpushed" and "pushed" is now always empty.
In effect, it copies p.unplaced into unpushed.
OpFilter.filter(pushed, p.op) tests whether the expr list is empty and
does not add a filter is if it oiis, returning p.op.
That can all be reduced to the simpler code.
The if ( ! p.unplaced.isEmpty() ) test is strictly unnecessary. Just
'unpushed.addAll(p.unplaced) ; ' would do.
Andy
Claude
On Sun, Nov 24, 2013 at 4:27 PM, <[email protected]> wrote:
Author: andy
Date: Sun Nov 24 16:27:13 2013
New Revision: 1545008
URL: http://svn.apache.org/r1545008
Log:
More general fix for JENA-595.
Modified:
jena/trunk/jena-arq/src/main/java/com/hp/hpl/jena/sparql/algebra/optimize/TransformFilterPlacement.java
jena/trunk/jena-arq/src/test/java/com/hp/hpl/jena/sparql/algebra/optimize/TestTransformFilterPlacement.java
Modified:
jena/trunk/jena-arq/src/main/java/com/hp/hpl/jena/sparql/algebra/optimize/TransformFilterPlacement.java
URL:
http://svn.apache.org/viewvc/jena/trunk/jena-arq/src/main/java/com/hp/hpl/jena/sparql/algebra/optimize/TransformFilterPlacement.java?rev=1545008&r1=1545007&r2=1545008&view=diff
==============================================================================
---
jena/trunk/jena-arq/src/main/java/com/hp/hpl/jena/sparql/algebra/optimize/TransformFilterPlacement.java
(original)
+++
jena/trunk/jena-arq/src/main/java/com/hp/hpl/jena/sparql/algebra/optimize/TransformFilterPlacement.java
Sun Nov 24 16:27:13 2013
@@ -426,26 +426,19 @@ public class TransformFilterPlacement ex
// And try down the expressions
Placement p = transform(pushed, opSub) ;
-
+
if ( p == null ) {
- Op op1 = OpFilter.filter(pushed, opSub) ;
- Op op2 = input.copy(op1) ; //, input.getVarExprList()) ;
//OpExtend.extend(op1, input.getVarExprList()) ;
- return result(op2, unpushed) ;
- }
- // handle the case where the filter is being to a surrounding
sequence or other bgp container
- // and may apply to the assignation variables.
- // in this case disjoint var sets indicate the expression is
unused.
- pushed = new ExprList() ;
- for ( Expr expr : p.unplaced ) {
- Set<Var> exprVars = expr.getVarsMentioned() ;
- if ( disjoint(vars1, exprVars) )
- unpushed.add(expr);
- else
- pushed.add(expr) ;
+ // Couldn't place an filter expressions. Do nothing.
+ return null ;
}
- Op op1 = OpFilter.filter(pushed, p.op) ;
- Op op2 = input.copy(op1) ;
- return result(op2, unpushed) ;
+
+ if ( ! p.unplaced.isEmpty() )
+ // Some placed, not all.
+ // Pass back out all untouched expressions.
+ unpushed.addAll(p.unplaced) ;
+ Op op1 = input.copy(p.op) ;
+
+ return result(op1, unpushed) ;
}
private Placement placeProject(ExprList exprs, OpProject input) {
Modified:
jena/trunk/jena-arq/src/test/java/com/hp/hpl/jena/sparql/algebra/optimize/TestTransformFilterPlacement.java
URL:
http://svn.apache.org/viewvc/jena/trunk/jena-arq/src/test/java/com/hp/hpl/jena/sparql/algebra/optimize/TestTransformFilterPlacement.java?rev=1545008&r1=1545007&r2=1545008&view=diff
==============================================================================
---
jena/trunk/jena-arq/src/test/java/com/hp/hpl/jena/sparql/algebra/optimize/TestTransformFilterPlacement.java
(original)
+++
jena/trunk/jena-arq/src/test/java/com/hp/hpl/jena/sparql/algebra/optimize/TestTransformFilterPlacement.java
Sun Nov 24 16:27:13 2013
@@ -124,15 +124,6 @@ public class TestTransformFilterPlacemen
null) ;
}
- @Test public void place_sequence_with_bind() {
- test("(filter (= ?foo 1) " +
- "(sequence " +
- "(extend " +
- " ((?bound (if ?v_binder 'Y' 'N')))
" +
- " (bgp (triple ?foob <
http://example.com/binding> ?v_binder)))" +
- "(bgp (triple ?foob <
http://www.w3.org/2000/01/rdf-schema#label> ?foo))))", null );
- }
-
// Join : one sided push.
@Test public void place_join_01() {
test("(filter (= ?x 123) (join (bgp (?s ?p ?x)) (bgp (?s ?p ?z))
))",
@@ -232,27 +223,49 @@ public class TestTransformFilterPlacemen
"(extend ((?z 123)) (filter (= ?x 123) (bgp (?s ?p ?x)) ))")
;
}
- @Test public void place_extend_02() { // Blocked
+ @Test public void place_extend_02() {
+ test("(filter ((= ?x1 123) (= ?x2 456)) (extend (?z 789) (bgp (?s
?p ?x1)) ))",
+ "(filter (= ?x2 456) (extend (?z 789) (filter (= ?x1 123)
(bgp (?s ?p ?x1)) )))") ;
+ }
+
+ @Test public void place_extend_03() { // Blocked
test("(filter (= ?x 123) (extend ((?x 123)) (bgp (?s ?p ?z)) ))",
null) ;
}
- @Test public void place_extend_03() {
+ @Test public void place_extend_04() {
test("(filter (= ?x 123) (extend ((?x1 123)) (filter (< ?x 456)
(bgp (?s ?p ?x) (?s ?p ?z))) ))",
"(extend (?x1 123) (sequence (filter ((= ?x 123) (< ?x 456))
(bgp (?s ?p ?x))) (bgp (?s ?p ?z))) )") ;
}
+ @Test public void place_extend_05() {
+ // Filter further out than one place.
+ test("(filter (= ?z 1) (sequence (extend (?x1 123) (bgp (?s ?p
?x))) (bgp (?s ?p ?z))))",
+ null) ;
+ }
+
+ @Test public void place_extend_06() {
+ // Filter further out than one place.
+ test("(filter (= ?z 1) (join (extend (?x1 123) (bgp (?s ?p ?x)))
(bgp (?s ?p ?z))))" ,
+ "(join (extend (?x1 123) (bgp (?s ?p ?x))) (filter (= ?z 1)
(bgp (?s ?p ?z))) )") ;
+ }
+
@Test public void place_assign_01() {
test("(filter (= ?x 123) (assign ((?z 123)) (bgp (?s ?p ?x)) ))",
"(assign ((?z 123)) (filter (= ?x 123) (bgp (?s ?p ?x)) ))")
;
}
- @Test public void place_assign_02() { // Blocked
+ @Test public void place_assign_02() {
+ test("(filter ((= ?x1 123) (= ?x2 456)) (assign (?z 789) (bgp (?s
?p ?x1)) ))",
+ "(filter (= ?x2 456) (assign (?z 789) (filter (= ?x1 123)
(bgp (?s ?p ?x1)) )))") ;
+ }
+
+ @Test public void place_assign_03() { // Blocked
test("(filter (= ?x 123) (assign ((?x 123)) (bgp (?s ?p ?z)) ))",
null) ;
}
- @Test public void place_assign_03() {
+ @Test public void place_assign_04() {
// Caution - OpFilter equality is sensitive to the order of
expressions
test("(filter (= ?x 123) (assign ((?x1 123)) (filter (< ?x 456)
(bgp (?s ?p ?x) (?s ?p ?z))) ))",
"(assign (?x1 123) (sequence (filter ((= ?x 123) (< ?x 456))
(bgp (?s ?p ?x))) (bgp (?s ?p ?z))) )") ;
