Thank you for the in depth explanation. I see that the specific case I couldn't find in the tests is in fact covered. Thank you.
On Mon, Nov 25, 2013 at 10:08 AM, Andy Seaborne <[email protected]> wrote: > 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))) )") ; >>> >>> >>> >>> >> >> > -- I like: Like Like - The likeliest place on the web<http://like-like.xenei.com> LinkedIn: http://www.linkedin.com/in/claudewarren
