You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jena.apache.org by an...@apache.org on 2015/06/22 20:08:18 UTC

jena git commit: JENA-963: Fix query level processing to work with GROUP

Repository: jena
Updated Branches:
  refs/heads/master 5385f5423 -> 3758cecd3


JENA-963: Fix query level processing to work with GROUP


Project: http://git-wip-us.apache.org/repos/asf/jena/repo
Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/3758cecd
Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/3758cecd
Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/3758cecd

Branch: refs/heads/master
Commit: 3758cecd3d5fa1f5856384c4d5457f980b2bc4d7
Parents: 5385f54
Author: Andy Seaborne <an...@apache.org>
Authored: Mon Jun 22 19:08:12 2015 +0100
Committer: Andy Seaborne <an...@apache.org>
Committed: Mon Jun 22 19:08:12 2015 +0100

----------------------------------------------------------------------
 .../apache/jena/sparql/algebra/OpAsQuery.java   | 1052 ++++++++++--------
 .../jena/sparql/algebra/TestOpAsQuery.java      |  240 ++--
 2 files changed, 743 insertions(+), 549 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/jena/blob/3758cecd/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpAsQuery.java
----------------------------------------------------------------------
diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpAsQuery.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpAsQuery.java
index 2b5be22..0d8cc1d 100644
--- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpAsQuery.java
+++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpAsQuery.java
@@ -16,9 +16,11 @@
  * limitations under the License.
  */
 
-package org.apache.jena.sparql.algebra;
+package org.apache.jena.sparql.algebra ;
 
 import java.util.* ;
+import java.util.function.BiConsumer ;
+import java.util.stream.Collectors ;
 
 import org.apache.jena.atlas.lib.NotImplemented ;
 import org.apache.jena.graph.Node ;
@@ -29,6 +31,8 @@ import org.apache.jena.query.SortCondition ;
 import org.apache.jena.query.Syntax ;
 import org.apache.jena.sparql.ARQInternalErrorException ;
 import org.apache.jena.sparql.ARQNotImplemented ;
+import org.apache.jena.sparql.algebra.Op ;
+import org.apache.jena.sparql.algebra.OpVisitor ;
 import org.apache.jena.sparql.algebra.op.* ;
 import org.apache.jena.sparql.core.BasicPattern ;
 import org.apache.jena.sparql.core.Quad ;
@@ -36,146 +40,397 @@ import org.apache.jena.sparql.core.Var ;
 import org.apache.jena.sparql.core.VarExprList ;
 import org.apache.jena.sparql.engine.QueryIterator ;
 import org.apache.jena.sparql.expr.* ;
+import org.apache.jena.sparql.expr.aggregate.Aggregator ;
 import org.apache.jena.sparql.pfunction.PropFuncArg ;
 import org.apache.jena.sparql.syntax.* ;
 import org.apache.jena.sparql.util.graph.GraphList ;
 import org.apache.jena.vocabulary.RDF ;
 
-/** Convert an Op expression in SPARQL syntax, that is, the reverse of algebra generation */   
-public class OpAsQuery
-{
-    public static Query asQuery(Op op)
-    {        
-        Converter converter = new Converter(op) ;
-        return converter.convert();
+/**
+ * Convert an Op expression in SPARQL syntax, that is, the reverse of algebra
+ * generation
+ */
+public class OpAsQuery {
+    
+    // slice-distinct/reduce-project-order-filter[having]-extend*[AS and aggregate naming]-group-pattern
+    // SELECT { ?s ?p ?o FILTER ( ?o > 5 ) ; }
+    // OpTopN
+
+    public static class /* struct */QueryLevelDetails {
+        OpSlice        opSlice    = null ;
+        OpDistinct     opDistinct = null ;
+        OpReduced      opReduced  = null ;
+        OpProject      opProject  = null ;
+        OpOrder        opOrder    = null ;
+        OpFilter       opHaving   = null ;
+        List<OpExtend> opExtends  = new ArrayList<>() ;
+        OpGroup        opGroup    = null ;
+        // The pattern of the group or query if not grouped.
+        Op             pattern    = null ;
+        
+        private QueryLevelDetails() {}
+        
+        public void info() {
+            if ( opSlice != null )
+                System.out.printf("slice: (%d, %d)\n", opSlice.getStart(), opSlice.getLength()) ;
+            if ( opDistinct != null )
+                System.out.printf("distinct\n") ;
+            if ( opReduced != null )
+                System.out.printf("reduced\n") ;
+            if ( opProject != null )
+                System.out.printf("project: %s\n", opProject.getVars()) ;
+            if ( opOrder != null )
+                System.out.printf("order: %s\n", opOrder.getConditions()) ;
+            if ( opHaving != null )
+                System.out.printf("having: %s\n", opHaving.getExprs()) ;
+            if ( opExtends != null && !opExtends.isEmpty() ) {
+                List<VarExprList> z = opExtends.stream().map(x -> x.getVarExprList()).collect(Collectors.toList()) ;
+                System.out.printf("assigns: %s\n", z) ;
+            }
+            if ( opGroup != null ) {
+                List<ExprAggregator> aggregators = opGroup.getAggregators() ;
+                List<Var> aggVars = aggregators.stream().map(x -> x.getAggVar().asVar()).collect(Collectors.toList()) ;
+                System.out.printf("group: %s |-| %s\n", opGroup.getGroupVars(), opGroup.getAggregators()) ;
+                System.out.printf("group agg vars: %s\n", aggVars) ;
+            }
+        }
+
+        static QueryLevelDetails analyse(Op operation) {
+            QueryLevelDetails details = new QueryLevelDetails() ;
+
+            Op op = operation ;
+            if ( op instanceof OpSlice ) {
+                details.opSlice = (OpSlice)op ;
+                op = details.opSlice.getSubOp() ;
+            }
+            if ( op instanceof OpDistinct ) {
+                details.opDistinct = (OpDistinct)op ;
+                op = details.opDistinct.getSubOp() ;
+            }
+            if ( op instanceof OpReduced ) {
+                details.opReduced = (OpReduced)op ;
+                op = details.opReduced.getSubOp() ;
+            }
+            if ( op instanceof OpProject ) {
+                details.opProject = (OpProject)op ;
+                op = details.opProject.getSubOp() ;
+
+            }
+            if ( op instanceof OpOrder ) {
+                details.opOrder = (OpOrder)op ;
+                op = details.opOrder.getSubOp() ;
+            }
+
+            // Lookahead to see if an opGroup can be found.
+            // Because inner SELECTs must have a project, the case of running
+            // into an inner-SELECT-group does not occur.
+            // And even if it did, either the inner-SELECT is part of a
+            // group-pattern
+            // (the {...} as in { { SELECT ... } ?s ?p ?o }
+            // (so we'd see the othe parts of the pattern as intermediates
+            // or if it's a singleton, collape the inner select into the outer
+            // to give an equivalent query in a different form.
+
+            details.opGroup = getGroup(op) ;
+            if ( details.opGroup == null ) {
+                op = processExtend(op, details.opExtends) ; 
+                details.pattern = op ;
+                return details ;
+            }
+            // (group) found.
+            details.pattern = details.opGroup.getSubOp() ;
+            if ( op instanceof OpFilter ) {
+                details.opHaving = (OpFilter)op ;
+                op = details.opHaving.getSubOp() ;
+            }
+            // Can't tell is just "aggregation" except by looking at the assignment
+            // variables.
+            
+            // AS and aggregate renames.
+            op = processExtend(op, details.opExtends) ;
+            
+            if ( !(op instanceof OpGroup) ) {
+                System.out.println("Expected (group), got " + op.getName()) ;
+            }
+
+            return details ;
+        }
     }
     
-    private static Set<Var> allocProjectVars()
-    {
-        return new LinkedHashSet<>() ;
+    public static Op processExtend(Op op, List<OpExtend> assignments) {
+        while ( op instanceof OpExtend ) {
+            OpExtend opExtend = (OpExtend)op ;
+            assignments.add(opExtend) ;
+            op = opExtend.getSubOp() ;
+        }
+        return op ;
+    }
+    /**
+     * Allows multiple filters and any number of extend - good or bad?
+     */
+    private static OpGroup getGroup(Op op) {
+        // Unwind tail recursion to protected against extreme queries.
+        for ( ; ; ) {
+            if ( op instanceof OpGroup )
+                return (OpGroup)op ;
+            if ( op instanceof OpFilter ) {
+                OpFilter opFilter = (OpFilter)op ;
+                op = opFilter.getSubOp() ;
+                continue ;
+            }
+            if ( op instanceof OpExtend ) { // AS or Aggregate naming
+                OpExtend opExtend = (OpExtend)op ;
+                op = opExtend.getSubOp() ;
+                continue ;
+            }
+            return null ;
+        }
     }
 
-    private static void addProjectVar(Collection<Var> vars, Var var)
-    {
-        // Must add uniquely.  Couple to allocProjectVars
-        //if (!vars.contains(var)) 
-        vars.add(var) ;
+    public static Query asQuery(Op op) {
+        Converter converter = new Converter(op) ;
+        return converter.convert() ;
     }
-    
-    public static class Converter implements OpVisitor
-    {
-        private Query query ;
-        private Op op ;
-        private Element element = null ;
-        private ElementGroup currentGroup = null ;
-        private Deque<ElementGroup> stack = new ArrayDeque<>() ;
-        private Collection<Var> projectVars = allocProjectVars();
-        private Map<Var, Expr> varExpression = new HashMap<>() ;
-        private int groupDepth = 0;
-        private boolean inProject = false;
-        private boolean hasRun = false;
-        
-        public Converter(Op op)
-        {
-            this.query = QueryFactory.create() ;
-            this.op = op;
+
+    public static class Converter implements OpVisitor {
+        private Query               query ;
+        private Op                  queryOp ;
+        private Element             element      = null ;
+        private ElementGroup        currentGroup = null ;
+        private Deque<ElementGroup> stack        = new ArrayDeque<>() ;
+        private int                 groupDepth   = 0 ;
+        private boolean             inProject    = false ;
+        private boolean             hasRun       = false ;
+
+        public Converter(Op op) {
+            this.query = null ;
+            this.queryOp = op ;
             currentGroup = new ElementGroup() ;
         }
-        
+
         Query convert() {
-            if (hasRun) {
-                return this.query;
-            } else {
+            if ( !hasRun )
                 try {
-                    op.visit(this) ;
-                    
-                    Collection<Var> vars = this.projectVars;
-                    query.setQueryResultStar(vars.isEmpty());   // SELECT * unless we are projecting
-        
-                    Iterator<Var> iter = vars.iterator();
-                    for (; iter.hasNext();) {
-                        Var var = iter.next();           
-                        if (this.varExpression.containsKey(var))
-                            query.addResultVar(var, this.varExpression.get(var));
-                        else
-                            query.addResultVar(var);
-                    }
-                    
-                    // Simplify currentGroup if possible, primarily look for the case of a single sub-query
-                    // which will mean we have an ElementGroup with a single item which is
-                    ElementGroup eg = this.currentGroup ;
-                    if (eg.getElements().size() == 1) {
-                        Element e = eg.getElements().get(0);
-                        if (e instanceof ElementSubQuery) {
-                            query.setQueryPattern(e);
-                        } else {
-                            query.setQueryPattern(eg);
-                        }
-                    } else {
-                        query.setQueryPattern(eg) ;
-                    }
-                    query.setQuerySelectType() ;
-                    query.setResultVars() ; // Variables from the group.
-                    return query ; 
-                } finally {
-                    this.hasRun = true;
+                    // Which may be broken, or null, if something went wrong.
+                    query = convertInner() ;
                 }
+                finally {
+                    this.hasRun = true ;
+                }
+            return query ;
+        }
+
+        Query convertInner() {
+            this.query = QueryFactory.create() ;
+            // Special case. SELECT * { ... BIND (  AS ...) }
+            if ( queryOp instanceof OpExtend ) {
+                List<OpExtend> assignments = new ArrayList<>() ;
+                Op op = processExtend(queryOp, assignments) ;
+                processQueryPattern(op, assignments) ;
+                query.setQueryResultStar(true) ;
+                query.setResultVars(); 
+                return query ;
             }
+            
+            // There is a projection.
+            QueryLevelDetails level = QueryLevelDetails.analyse(queryOp) ;
+            processQueryPattern(level) ;
+
+            // Modifiers.
+            // slice-distinct/reduce-project-order-filter[having]-extend[AS]-extend[agg]-group-pattern
+            // Do in reverse order because e.g. exts have effects on project.
+
+            // Substitution mapping
+            Map<ExprVar, Expr> aggVarExprMap = new HashMap<>() ;
+
+            if ( level.opGroup != null ) {
+                query.getGroupBy().addAll(level.opGroup.getGroupVars()) ;
+                level.opGroup.getAggregators().forEach(eAgg -> {
+                    ExprVar v = eAgg.getAggVar() ;
+                    Aggregator agg = eAgg.getAggregator() ;
+                    aggVarExprMap.put(v, eAgg) ;
+                }) ;
+                query.getAggregators().addAll(level.opGroup.getAggregators()) ;
+            }
+            
+            ExprTransform varToExpr = new ExprTransformCopy() {
+                @Override
+                public Expr transform(ExprVar nv) {
+                    if ( aggVarExprMap.containsKey(nv) )
+                        return aggVarExprMap.get(nv) ;
+                    return nv ;
+                }
+            } ;
+
+            // The assignments will become part of the project. 
+            Map<Var, Expr> assignments = new HashMap<>() ;
+            if ( level.opExtends != null ) {
+                processExtends(level.opExtends, (var,expr)->{
+                    // Internal rename.
+                    expr = rewrite(expr, varToExpr) ;
+                    assignments.put(var, expr) ;
+                }) ;
+            }
+            
+            
+            if ( level.opHaving != null ) {
+                level.opHaving.getExprs().getList().forEach(expr -> {
+                    expr = rewrite(expr, varToExpr) ;
+                    query.getHavingExprs().add(expr) ;
+                }) ;
+            }
+
+            if ( level.opOrder != null ) {
+                level.opOrder.getConditions().forEach(sc -> {
+                    Expr expr = sc.getExpression() ;
+                    expr = rewrite(expr, varToExpr) ;
+                    if ( expr == sc.getExpression() )
+                        query.addOrderBy(sc) ;
+                    else
+                        query.addOrderBy(new SortCondition(expr, sc.getDirection())) ;
+                }) ;
+                // level.opOrder.getConditions().forEach(sc->query.addOrderBy(sc))
+                // ;
+            }
+            if ( level.opProject != null ) {
+                level.opProject.getVars().forEach(v -> {
+                    if ( assignments.containsKey(v) ) {
+                        query.addResultVar(v, assignments.get(v)) ;
+                    } else
+                        query.getProjectVars().add(v) ;
+                    
+                }) ;
+            } else {
+                // Insert BIND for any (extends) and no project happsn in processQueryPattern.
+                query.setQueryResultStar(true) ;
+            }
+
+            if ( level.opDistinct != null )
+                query.setDistinct(true) ;
+            if ( level.opReduced != null )
+                query.setReduced(true) ;
+
+            if ( level.opSlice != null ) {
+                query.setOffset(level.opSlice.getStart()) ;
+                query.setLimit(level.opSlice.getLength()) ;
+            }
+            query.setResultVars() ;
+            
+//            // Fixup -- fixupSubQueryOfOne in processQueryPattern
+//            // Simplify currentGroup if possible, primarily look for the case of a single sub-query
+//            // which will mean we have an ElementGroup with a single item which is
+//            ElementGroup eg = this.currentGroup ;
+//            if (eg.getElements().size() == 1) {
+//                Element e = eg.getElements().get(0);
+//                if (e instanceof ElementSubQuery) {
+//                    query.setQueryPattern(e);
+//                } else {
+//                    query.setQueryPattern(eg);
+//                }
+//            } else {
+//                query.setQueryPattern(eg) ;
+//            }
+            return query ;
         }
 
-        Element asElement(Op op)
-        {
+        private static void processExtends(List<OpExtend> ext, BiConsumer<Var, Expr> action) {
+            ext.forEach(extend->{
+                extend.getVarExprList().getExprs().forEach(action) ;
+            });
+        }
+        
+        private static void processAssigns(List<OpAssign> assigns, BiConsumer<Var, Expr> action) {
+            assigns.forEach(assign->{
+                assign.getVarExprList().getExprs().forEach(action) ;
+            });
+        }
+
+        private static Expr rewrite(Expr expr, ExprTransform transform) {
+            return ExprTransformer.transform(transform, expr) ;
+        }
+
+        private void processQueryPattern(QueryLevelDetails level) {
+            Op op = level.pattern ;
+            op.visit(this) ;
+            ElementGroup eg = this.currentGroup ;
+            Element e = fixupSubQueryOfOne(eg) ;
+            query.setQueryPattern(e) ;
+            query.setQuerySelectType() ;
+        }
+        
+        // Without level: This is SLEECT * { ... BIND (..) }
+        // which is (extend ... 
+        private void processQueryPattern(Op op, List<OpExtend> assignments) {
+            op.visit(this) ;
+
+            ElementGroup eg = this.currentGroup ;
+            processExtends(assignments,(v,e)->eg.addElement(new ElementBind(v, e)) ) ;
+            
+            Element e = fixupSubQueryOfOne(eg) ;
+            query.setQueryPattern(e) ;
+            query.setQuerySelectType() ;
+        }
+
+        private Element fixupSubQueryOfOne(ElementGroup eg) {
+            // Simplify currentGroup if possible, primarily look for the case of
+            // a single sub-query which will mean we have an ElementGroup with a single
+            // item which is ElementSubQuery.
+            if ( eg.getElements().size() != 1 )
+                return eg ;
+            Element e = eg.getElements().get(0) ;
+            if ( e instanceof ElementSubQuery )
+                return e ;
+            return eg ;
+        }
+        
+        Element asElement(Op op) {
             ElementGroup g = asElementGroup(op) ;
             if ( g.getElements().size() == 1 )
                 return g.getElements().get(0) ;
             return g ;
         }
-        
-        ElementGroup asElementGroup(Op op)
-        {
+
+        ElementGroup asElementGroup(Op op) {
             startSubGroup() ;
             op.visit(this) ;
             return endSubGroup() ;
         }
 
         @Override
-        public void visit(OpBGP opBGP)
-        {
+        public void visit(OpBGP opBGP) {
             currentGroup().addElement(process(opBGP.getPattern())) ;
         }
 
-//        public void visit(OpPropFunc opPropFunc)
-//        {
-//            OpBGP opBGP = opPropFunc.getBGP() ;
-//            currentGroup().addElement(process(opBGP.getPattern())) ;
-//        }
-        
-        @Override
-        public void visit(OpTriple opTriple)
-        { currentGroup().addElement(process(opTriple.getTriple())) ; }
+        // public void visit(OpPropFunc opPropFunc)
+        // {
+        // OpBGP opBGP = opPropFunc.getBGP() ;
+        // currentGroup().addElement(process(opBGP.getPattern())) ;
+        // }
 
         @Override
-        public void visit(OpQuad opQuad)
-        { throw new ARQNotImplemented("OpQuad") ; }
+        public void visit(OpTriple opTriple) {
+            currentGroup().addElement(process(opTriple.getTriple())) ;
+        }
 
+        @Override
+        public void visit(OpQuad opQuad) {
+            throw new ARQNotImplemented("OpQuad") ;
+        }
 
         @Override
-        public void visit(OpProcedure opProcedure)
-        {
+        public void visit(OpProcedure opProcedure) {
             throw new ARQNotImplemented("OpProcedure") ;
         }
-        
+
         @Override
-        public void visit(OpPropFunc opPropFunc)
-        {
+        public void visit(OpPropFunc opPropFunc) {
             Node s = processPropFuncArg(opPropFunc.getSubjectArgs()) ;
             Node o = processPropFuncArg(opPropFunc.getObjectArgs()) ;
             Triple t = new Triple(s, opPropFunc.getProperty(), o) ;
             currentGroup().addElement(process(t)) ;
         }
-        
-        private Node processPropFuncArg(PropFuncArg args)
-        {
+
+        private Node processPropFuncArg(PropFuncArg args) {
             if ( args.isNode() )
                 return args.getArg() ;
 
@@ -188,109 +443,100 @@ public class OpAsQuery
             currentGroup().addElement(process(bgp)) ;
             return head ;
         }
-        
+
         // There is one special case to consider:
-        // A path expression was expaned into a OpSequence during Algenra generation.
-        // The simple path expressions become an OpSequence that could be recombined
-        // into on ElementPathBlock 
-        
+        // A path expression was expaned into a OpSequence during Algenra
+        // generation.
+        // The simple path expressions become an OpSequence that could be
+        // recombined
+        // into on ElementPathBlock
+
         @Override
-        public void visit(OpSequence opSequence)
-        {
+        public void visit(OpSequence opSequence) {
             ElementGroup g = currentGroup() ;
-            boolean nestGroup = ! g.isEmpty() ;
-            if ( nestGroup )
-            {
+            boolean nestGroup = !g.isEmpty() ;
+            if ( nestGroup ) {
                 startSubGroup() ;
                 g = currentGroup() ;
             }
-            
-            for ( Op op : opSequence.getElements() )
-            {
+
+            for ( Op op : opSequence.getElements() ) {
                 Element e = asElement(op) ;
                 g.addElement(e) ;
             }
-            
+
             if ( nestGroup )
                 endSubGroup() ;
             return ;
         }
-        
+
         @Override
-        public void visit(OpDisjunction opDisjunction)
-        {
+        public void visit(OpDisjunction opDisjunction) {
             throw new ARQNotImplemented("OpDisjunction") ;
         }
 
-        private Element process(BasicPattern pattern)
-        {
-            // The different SPARQL versions use different internal structures for BGPs.
-            if ( query.getSyntax() == Syntax.syntaxSPARQL_10 )
-            {
+        private Element process(BasicPattern pattern) {
+            // The different SPARQL versions use different internal structures
+            // for BGPs.
+            if ( query.getSyntax() == Syntax.syntaxSPARQL_10 ) {
                 ElementTriplesBlock e = new ElementTriplesBlock() ;
-                for (Triple t : pattern)
+                for ( Triple t : pattern )
                     // Leave bNode variables as they are
-                    // Query serialization will deal with them. 
+                    // Query serialization will deal with them.
                     e.addTriple(t) ;
                 return e ;
             }
-            
-            if ( query.getSyntax() == Syntax.syntaxSPARQL_11 ||
-                 query.getSyntax() == Syntax.syntaxARQ )
-            {
+
+            if ( query.getSyntax() == Syntax.syntaxSPARQL_11 || query.getSyntax() == Syntax.syntaxARQ ) {
                 ElementPathBlock e = new ElementPathBlock() ;
-                for (Triple t : pattern)
+                for ( Triple t : pattern )
                     // Leave bNode variables as they are
-                    // Query serialization will deal with them. 
+                    // Query serialization will deal with them.
                     e.addTriple(t) ;
                 return e ;
             }
-            
-            throw new ARQInternalErrorException("Unrecognized syntax: "+query.getSyntax()) ;
-            
+
+            throw new ARQInternalErrorException("Unrecognized syntax: " + query.getSyntax()) ;
+
         }
-        
-        private ElementTriplesBlock process(Triple triple)
-        {
+
+        private ElementTriplesBlock process(Triple triple) {
             // Unsubtle
             ElementTriplesBlock e = new ElementTriplesBlock() ;
             e.addTriple(triple) ;
             return e ;
         }
-        
+
         @Override
-        public void visit(OpQuadPattern quadPattern)
-        {
-            Node graphNode = quadPattern.getGraphNode();
-            if (graphNode.equals(Quad.defaultGraphNodeGenerated)) {
+        public void visit(OpQuadPattern quadPattern) {
+            Node graphNode = quadPattern.getGraphNode() ;
+            if ( graphNode.equals(Quad.defaultGraphNodeGenerated) ) {
                 currentGroup().addElement(process(quadPattern.getBasicPattern())) ;
             } else {
-                startSubGroup();
+                startSubGroup() ;
                 Element e = asElement(new OpBGP(quadPattern.getBasicPattern())) ;
-                endSubGroup();
-                
-                //If not element group make it one
-                if (!(e instanceof ElementGroup)) {
-                    ElementGroup g = new ElementGroup();
-                    g.addElement(e);
-                    e = g;
+                endSubGroup() ;
+
+                // If not element group make it one
+                if ( !(e instanceof ElementGroup) ) {
+                    ElementGroup g = new ElementGroup() ;
+                    g.addElement(e) ;
+                    e = g ;
                 }
-                
+
                 Element graphElt = new ElementNamedGraph(graphNode, e) ;
                 currentGroup().addElement(graphElt) ;
             }
         }
 
         @Override
-        public void visit(OpQuadBlock quadBlock)
-        {
+        public void visit(OpQuadBlock quadBlock) {
             // Gather into OpQuadPatterns.
             throw new NotImplemented("OpQuadBlock") ;
         }
 
         @Override
-        public void visit(OpPath opPath)
-        {
+        public void visit(OpPath opPath) {
             ElementPathBlock epb = new ElementPathBlock() ;
             epb.addTriplePath(opPath.getTriplePath()) ;
             ElementGroup g = currentGroup() ;
@@ -298,50 +544,53 @@ public class OpAsQuery
         }
 
         @Override
-        public void visit(OpJoin opJoin)
-        {
-            // Keep things clearly separated.
+        public void visit(OpJoin opJoin) {
             Element eLeft = asElement(opJoin.getLeft()) ;
-            Element eRight = asElementGroup(opJoin.getRight()) ;
-            
+            ElementGroup eRightGroup = asElementGroup(opJoin.getRight()) ;
+            Element eRight = eRightGroup ;
+            // Very special case. If the RHS is not something that risks
+            // reparsing into a copmbined element of a group, strip the group-of-one. 
+            if ( eRightGroup.getElements().size() == 1 ) {
+                // This always was a {} around it but it's unnecessary in a group of one. 
+                if ( eRightGroup.getElements().get(0) instanceof ElementSubQuery )
+                    eRight = eRightGroup.getElements().get(0) ;
+            }
+
             ElementGroup g = currentGroup() ;
             g.addElement(eLeft) ;
             g.addElement(eRight) ;
             return ;
         }
 
-        private static boolean emptyGroup(Element element)
-        {
-            if ( ! ( element instanceof ElementGroup ) )
+        private static boolean emptyGroup(Element element) {
+            if ( !(element instanceof ElementGroup) )
                 return false ;
             ElementGroup eg = (ElementGroup)element ;
             return eg.isEmpty() ;
         }
-        
+
         @Override
-        public void visit(OpLeftJoin opLeftJoin)
-        {
+        public void visit(OpLeftJoin opLeftJoin) {
             Element eLeft = asElement(opLeftJoin.getLeft()) ;
             ElementGroup eRight = asElementGroup(opLeftJoin.getRight()) ;
-            
-            if ( opLeftJoin.getExprs() != null )
-            {
-                for ( Expr expr : opLeftJoin.getExprs() )
-                {
+
+            if ( opLeftJoin.getExprs() != null ) {
+                for ( Expr expr : opLeftJoin.getExprs() ) {
                     ElementFilter f = new ElementFilter(expr) ;
                     eRight.addElement(f) ;
                 }
             }
             ElementGroup g = currentGroup() ;
-            if ( ! emptyGroup(eLeft) )
+            if ( !emptyGroup(eLeft) )
                 g.addElement(eLeft) ;
             ElementOptional opt = new ElementOptional(eRight) ;
             g.addElement(opt) ;
         }
 
         @Override
-        public void visit(OpDiff opDiff)
-        { throw new ARQNotImplemented("OpDiff") ; }
+        public void visit(OpDiff opDiff) {
+            throw new ARQNotImplemented("OpDiff") ;
+        }
 
         @Override
         public void visit(OpMinus opMinus) {
@@ -349,30 +598,28 @@ public class OpAsQuery
             Element eRight = asElementGroup(opMinus.getRight()) ;
             ElementMinus elMinus = new ElementMinus(eRight) ;
             ElementGroup g = currentGroup() ;
-            if ( ! emptyGroup(eLeft) )
+            if ( !emptyGroup(eLeft) )
                 g.addElement(eLeft) ;
             g.addElement(elMinus) ;
         }
 
         @Override
-        public void visit(OpUnion opUnion)
-        {
+        public void visit(OpUnion opUnion) {
             Element eLeft = asElementGroup(opUnion.getLeft()) ;
             Element eRight = asElementGroup(opUnion.getRight()) ;
-            if ( eLeft instanceof ElementUnion )
-            {
+            if ( eLeft instanceof ElementUnion ) {
                 ElementUnion elUnion = (ElementUnion)eLeft ;
                 elUnion.addElement(eRight) ;
                 return ;
             }
-            
-//            if ( eRight instanceof ElementUnion )
-//            {
-//                ElementUnion elUnion = (ElementUnion)eRight ;
-//                elUnion.getElements().add(0, eLeft) ;
-//                return ;
-//            }
-            
+
+            // if ( eRight instanceof ElementUnion )
+            // {
+            // ElementUnion elUnion = (ElementUnion)eRight ;
+            // elUnion.getElements().add(0, eLeft) ;
+            // return ;
+            // }
+
             ElementUnion elUnion = new ElementUnion() ;
             elUnion.addElement(eLeft) ;
             elUnion.addElement(eRight) ;
@@ -380,374 +627,277 @@ public class OpAsQuery
         }
 
         @Override
-        public void visit(OpConditional opCondition)
-        { throw new ARQNotImplemented("OpCondition") ; }
+        public void visit(OpConditional opCondition) {
+            throw new ARQNotImplemented("OpCondition") ;
+        }
 
         @Override
-        public void visit(OpFilter opFilter)
-        {
-            // (filter .. (filter ( ... ))   (non-canonicalizing OpFilters)
-            // Inner gets Grouped unnecessarily. 
+        public void visit(OpFilter opFilter) {
+            // (filter .. (filter ( ... )) (non-canonicalizing OpFilters)
+            // Inner gets Grouped unnecessarily.
             Element e = asElement(opFilter.getSubOp()) ;
             if ( currentGroup() != e )
                 currentGroup().addElement(e) ;
-            element = currentGroup() ;      // Was cleared by asElement. 
-            
+            element = currentGroup() ; // Was cleared by asElement.
+
             ExprList exprs = opFilter.getExprs() ;
-            for ( Expr expr : exprs )
-            {
+            for ( Expr expr : exprs ) {
                 ElementFilter f = new ElementFilter(expr) ;
                 currentGroup().addElement(f) ;
             }
         }
 
         @Override
-        public void visit(OpGraph opGraph)
-        {
+        public void visit(OpGraph opGraph) {
             startSubGroup() ;
             Element e = asElement(opGraph.getSubOp()) ;
             endSubGroup() ;
-            
-            //If not element group make it one
-            if (!(e instanceof ElementGroup)) {
-                ElementGroup g = new ElementGroup();
-                g.addElement(e);
-                e = g;
+
+            // If not element group make it one
+            if ( !(e instanceof ElementGroup) ) {
+                ElementGroup g = new ElementGroup() ;
+                g.addElement(e) ;
+                e = g ;
             }
-            
+
             Element graphElt = new ElementNamedGraph(opGraph.getNode(), e) ;
             currentGroup().addElement(graphElt) ;
         }
 
         @Override
-        public void visit(OpService opService)
-        { 
+        public void visit(OpService opService) {
             // Hmm - if the subnode has been optimized, we may fail.
             Op op = opService.getSubOp() ;
-            Element x = asElement(opService.getSubOp()) ; 
+            Element x = asElement(opService.getSubOp()) ;
             Element elt = new ElementService(opService.getService(), x, opService.getSilent()) ;
             currentGroup().addElement(elt) ;
         }
-        
+
         @Override
-        public void visit(OpDatasetNames dsNames)
-        { throw new ARQNotImplemented("OpDatasetNames") ; }
+        public void visit(OpDatasetNames dsNames) {
+            throw new ARQNotImplemented("OpDatasetNames") ;
+        }
 
         @Override
-        public void visit(OpTable opTable)
-        { 
-            // This will go in a group so simply forget it. 
-            if ( opTable.isJoinIdentity() ) return ;
-            
+        public void visit(OpTable opTable) {
+            // This will go in a group so simply forget it.
+            if ( opTable.isJoinIdentity() )
+                return ;
+
             // Put in a VALUES
             // This may be related to the grpup of the overall query.
-            
-            ElementData el = new ElementData() ; 
+
+            ElementData el = new ElementData() ;
             el.getVars().addAll(opTable.getTable().getVars()) ;
             QueryIterator qIter = opTable.getTable().iterator(null) ;
-            while(qIter.hasNext())
+            while (qIter.hasNext())
                 el.getRows().add(qIter.next()) ;
             qIter.close() ;
             currentGroup().addElement(el) ;
         }
 
         @Override
-        public void visit(OpExt opExt)
-        {
-//            Op op = opExt.effectiveOp() ;
-//            // This does not work in all cases.
-//            op.visit(this) ;
+        public void visit(OpExt opExt) {
+            // Op op = opExt.effectiveOp() ;
+            // // This does not work in all cases.
+            // op.visit(this) ;
             throw new ARQNotImplemented("OpExt") ;
         }
 
         @Override
-        public void visit(OpNull opNull)
-        { throw new ARQNotImplemented("OpNull") ; }
+        public void visit(OpNull opNull) {
+            throw new ARQNotImplemented("OpNull") ;
+        }
 
         @Override
-        public void visit(OpLabel opLabel)
-        {
+        public void visit(OpLabel opLabel) {
             if ( opLabel.hasSubOp() )
                 opLabel.getSubOp().visit(this) ;
         }
 
+        private void newLevel(Op op) {
+            convertAsSubQuery(op) ;
+        }
+
         @Override
-        public void visit(OpAssign opAssign)
-        {
-            opAssign.getSubOp().visit(this) ;
-            
-            // Go through each var and get the assigned expression
-            for ( Var v : opAssign.getVarExprList().getVars() )
-            {
-                Expr e = opAssign.getVarExprList().getExpr(v);
-                
-                // Substitute group aggregate expressions in for generated vars
-                SubExprForVar sefr = new SubExprForVar(varExpression);
-                Expr tr = ExprTransformer.transform(sefr, e);
-                
-                // If in top level we defer assignment to SELECT section
-                // This also covers the GROUP recombine
-                if (inTopLevel()) {
-                    if (!inGroupRecombine(opAssign)) {
-                        // If not wrapped over a Group then we need to ensure we add the variable
-                        // to the list or otherwise the BIND will not round trip
-                        // Note - This does mean top level BIND will manifest as a project expression
-                        //        rather than a BIND but this is semantically equivalent so is not an issue
-                        addProjectVar(projectVars, v) ;
-                    }
-                    varExpression.put(v, tr);
-                } else {
-                    Element elt = new ElementAssign(v, e) ;
-                    ElementGroup g = currentGroup() ;
-                    g.addElement(elt) ;
-                }
-            }
+        public void visit(OpAssign opAssign) {
+            Element e = asElement(opAssign.getSubOp()) ;
+            if ( currentGroup() != e )
+                currentGroup().addElement(e) ;
+            processAssigns(Arrays.asList(opAssign), (var,expr)->{
+                currentGroup().addElement(new ElementAssign(var,expr)) ;
+            }) ;
         }
 
         @Override
-        public void visit(OpExtend opExtend)
-        { 
-            opExtend.getSubOp().visit(this) ;
-            
-            // Go through each var and get the assigned expression
-            for ( Var v : opExtend.getVarExprList().getVars() )
-            {
-                Expr e = opExtend.getVarExprList().getExpr(v);
-                
-                // Substitute group aggregate expressions in for generated vars
-                Expr tr = ExprTransformer.transform(new SubExprForVar(varExpression), e);
-                
-                // If in top level we defer assignment to SELECT section
-                // This also covers the GROUP recombine
-                if (inTopLevel()) {
-                    if (groupDepth == 0 || inGroupRecombine(opExtend)) {
-                        if (!inGroupRecombine(opExtend)) {
-                            // If not wrapped over a Group then we need to ensure we add the variable
-                            // to the list or otherwise the BIND will not round trip
-                            // Note - This does mean top level BIND will manifest as a project expression
-                            //        rather than a BIND but this is semantically equivalent so is not an issue
-                            addProjectVar(projectVars, v) ;
-                        }
-                        varExpression.put(v, tr);
-                    } else {
-                        Element elt = new ElementBind(v, tr) ;
-                        ElementGroup g = currentGroup() ;
-                        g.addElement(elt);
-                    }
-                } else {
-                    Element elt = new ElementBind(v, tr) ;
-                    ElementGroup g = currentGroup() ;
-                    g.addElement(elt) ;
-                }
-            }
+        public void visit(OpExtend opExtend) {
+            Element e = asElement(opExtend.getSubOp()) ;
+            if ( currentGroup() != e )
+                currentGroup().addElement(e) ;
+            processExtends(Arrays.asList(opExtend), (var,expr)->{
+                currentGroup().addElement(new ElementBind(var,expr)) ;
+            }) ;
         }
 
-        
         @Override
-        public void visit(OpList opList)
-        { /* No action */ }
+        public void visit(OpList opList) {
+            opList.getSubOp().visit(this) ;
+        }
 
         @Override
-        public void visit(OpOrder opOrder)
-        {
-            List<SortCondition> x = opOrder.getConditions() ;
-            for ( SortCondition sc : x )
-                query.addOrderBy(sc);
-            opOrder.getSubOp().visit(this) ;
+        public void visit(OpOrder opOrder) {
+            newLevel(opOrder) ;
+            // List<SortCondition> x = opOrder.getConditions() ;
+            // for ( SortCondition sc : x )
+            // query.addOrderBy(sc);
+            // opOrder.getSubOp().visit(this) ;
         }
 
         @Override
-        public void visit(OpProject opProject)
-        {
-            if (inProject) {
-                // If we've already inside a project then we are reconstructing a sub-query
-                // Create a new converter and call on the sub-op to get the sub-query
-                convertAsSubQuery(opProject);
-            } else {
-                // Defer adding result vars until the end.
-                // OpGroup generates dupes otherwise
-                this.projectVars = allocProjectVars() ;
-                this.projectVars.addAll(opProject.getVars());
-                inProject = true;
-                opProject.getSubOp().visit(this) ;
-                inProject = false;
-            }
+        public void visit(OpProject opProject) {
+            newLevel(opProject) ;
+            // if (inProject) {
+            // // If we've already inside a project then we are reconstructing a
+            // sub-query
+            // // Create a new converter and call on the sub-op to get the
+            // sub-query
+            // convertAsSubQuery(opProject);
+            // } else {
+            // // Defer adding result vars until the end.
+            // // OpGroup generates dupes otherwise
+            // this.projectVars = allocProjectVars() ;
+            // this.projectVars.addAll(opProject.getVars());
+            // inProject = true;
+            // opProject.getSubOp().visit(this) ;
+            // inProject = false;
+            // }
         }
 
         private void convertAsSubQuery(Op op) {
-            Converter subConverter = new Converter(op);
-            ElementSubQuery subQuery = new ElementSubQuery(subConverter.convert());
-            ElementGroup g = currentGroup();
-            g.addElement(subQuery);
+            Converter subConverter = new Converter(op) ;
+            ElementSubQuery subQuery = new ElementSubQuery(subConverter.convert()) ;
+            ElementGroup g = currentGroup() ;
+            g.addElement(subQuery) ;
         }
 
         @Override
-        public void visit(OpReduced opReduced)
-        { 
-            if (inProject) {
-                convertAsSubQuery(opReduced);
-            } else {
-                query.setReduced(true) ;
-                opReduced.getSubOp().visit(this) ;
-            }
+        public void visit(OpReduced opReduced) {
+            newLevel(opReduced) ;
+            // if (inProject) {
+            // convertAsSubQuery(opReduced);
+            // } else {
+            // query.setReduced(true) ;
+            // opReduced.getSubOp().visit(this) ;
+            // }
         }
 
         @Override
-        public void visit(OpDistinct opDistinct)
-        { 
-            if (inProject) {
-                convertAsSubQuery(opDistinct);
-            } else {
-                query.setDistinct(true) ;
-                opDistinct.getSubOp().visit(this) ;
-            }
+        public void visit(OpDistinct opDistinct) {
+            newLevel(opDistinct) ;
+            // if (inProject) {
+            // convertAsSubQuery(opDistinct);
+            // } else {
+            // query.setDistinct(true) ;
+            // opDistinct.getSubOp().visit(this) ;
+            // }
         }
 
         @Override
-        public void visit(OpSlice opSlice)
-        {
-            if (inProject) {
-                convertAsSubQuery(opSlice);
-            } else {
-                if ( opSlice.getStart() != Query.NOLIMIT )
-                    query.setOffset(opSlice.getStart()) ;
-                if ( opSlice.getLength() != Query.NOLIMIT )
-                    query.setLimit(opSlice.getLength()) ;
-                opSlice.getSubOp().visit(this) ;
-            }
+        public void visit(OpSlice opSlice) {
+            newLevel(opSlice) ;
+            // if (inProject) {
+            // convertAsSubQuery(opSlice);
+            // } else {
+            // if ( opSlice.getStart() != Query.NOLIMIT )
+            // query.setOffset(opSlice.getStart()) ;
+            // if ( opSlice.getLength() != Query.NOLIMIT )
+            // query.setLimit(opSlice.getLength()) ;
+            // opSlice.getSubOp().visit(this) ;
+            // }
         }
 
         @Override
         public void visit(OpGroup opGroup) {
-            List<ExprAggregator> a = opGroup.getAggregators();
-            
-            // Aggregators are broken up in the algebra, split between a
-            // group and an assignment (extend or assign) using a generated var.
-            // We record them here and insert later.
-            for (ExprAggregator ea : a) {
-                // Substitute generated var for actual
-                Var givenVar = ea.getAggVar().asVar();
-                // Copy aggregator across (?)
-                Expr myAggr = query.allocAggregate(ea.getAggregator());
-                varExpression.put(givenVar, myAggr);
-            }
-
-            VarExprList b = opGroup.getGroupVars();
-            for (Var v : b.getVars()) {
-                Expr e = b.getExpr(v);
-
-                if (e != null) {
-                    query.addGroupBy(v, e);
-
-                } else {
-                    query.addGroupBy(v);
-
-                }
-            }
-            groupDepth++;
-            opGroup.getSubOp().visit(this);
-            groupDepth--;
-        }
-
-        @Override
-        public void visit(OpTopN opTop)
-        { throw new ARQNotImplemented("OpTopN") ; }
-        
-        private Element lastElement()
-        {
+            newLevel(opGroup) ;
+            // List<ExprAggregator> a = opGroup.getAggregators();
+            //
+            // // Aggregators are broken up in the algebra, split between a
+            // // group and an assignment (extend or assign) using a generated
+            // var.
+            // // We record them here and insert later.
+            // for (ExprAggregator ea : a) {
+            // // Substitute generated var for actual
+            // Var givenVar = ea.getAggVar().asVar();
+            // // Copy aggregator across (?)
+            // Expr myAggr = query.allocAggregate(ea.getAggregator());
+            // varExpression.put(givenVar, myAggr);
+            // }
+            //
+            // VarExprList b = opGroup.getGroupVars();
+            // for (Var v : b.getVars()) {
+            // Expr e = b.getExpr(v);
+            //
+            // if (e != null) {
+            // query.addGroupBy(v, e);
+            //
+            // } else {
+            // query.addGroupBy(v);
+            //
+            // }
+            // }
+            // groupDepth++;
+            // opGroup.getSubOp().visit(this);
+            // groupDepth--;
+        }
+
+        @Override
+        public void visit(OpTopN opTop) {
+            throw new ARQNotImplemented("OpTopN") ;
+        }
+
+        private Element lastElement() {
             ElementGroup g = currentGroup ;
             if ( g == null || g.getElements().size() == 0 )
                 return null ;
             int len = g.getElements().size() ;
-            return g.getElements().get(len-1) ;
+            return g.getElements().get(len - 1) ;
         }
 
-        private void startSubGroup()
-        {
+        private void startSubGroup() {
             push(currentGroup) ;
             ElementGroup g = new ElementGroup() ;
             currentGroup = g ;
         }
-        
-        private ElementGroup endSubGroup()
-        {
+
+        private ElementGroup endSubGroup() {
             ElementGroup g = pop() ;
             ElementGroup r = currentGroup ;
             currentGroup = g ;
             return r ;
         }
-        
-//        private void endCurrentGroup()
-//        {
-//            currentGroup = null ;
-//            element = null ; //??
-//        }
-        
-        private ElementGroup currentGroup()
-        {
-//            if ( currentGroup == null )
-//                startSubGroup() ;
+
+        private ElementGroup currentGroup() {
+            // if ( currentGroup == null )
+            // startSubGroup() ;
             return currentGroup ;
         }
-        
-        private ElementGroup peek()
-        {
+
+        private ElementGroup peek() {
             if ( stack.size() == 0 )
                 return null ;
-            return stack.peek();
-        }
-        private ElementGroup pop() { return stack.pop(); }
-        private void push(ElementGroup el) { stack.push(el); }
-        private boolean inTopLevel() { return stack.size() == 0; }
-        
-        private boolean inGroupRecombine(OpExtend op) {
-            Op subOp = op.getSubOp();
-            if (subOp instanceof OpExtend) {
-                return inGroupRecombine((OpExtend)subOp);
-            } else if (subOp instanceof OpAssign) {
-                return inGroupRecombine((OpAssign)subOp);
-            } else if (subOp instanceof OpGroup) {
-                return true;
-            } else {
-                return false;
-            }
+            return stack.peek() ;
         }
-        
-        private boolean inGroupRecombine(OpAssign op) {
-            Op subOp = op.getSubOp();
-            if (subOp instanceof OpExtend) {
-                return inGroupRecombine((OpExtend)subOp);
-            } else if (subOp instanceof OpAssign) {
-                return inGroupRecombine((OpAssign)subOp);
-            } else if (subOp instanceof OpGroup) {
-                return true;
-            } else {
-                return false;
-            }
+
+        private ElementGroup pop() {
+            return stack.pop() ;
         }
-    }
-    
-    /**
-     * This class is used to take substitute an expressions for variables
-     * in another expression. It is used to stick grouping expressions back
-     * together.
-     */
-    public static class SubExprForVar extends ExprTransformCopy {
-        private final Map<Var, Expr> varExpr;
-        private boolean subOccurred = false;
-        public SubExprForVar(Map<Var, Expr> varExpr) {
-            this.varExpr = varExpr;
+
+        private void push(ElementGroup el) {
+            stack.push(el) ;
         }
-        
-        public boolean didChange() { return subOccurred; }
-        
-        @Override
-        public Expr transform(ExprVar var) {
-            if (varExpr.containsKey(var.asVar())) {
-                subOccurred = true;
-                return varExpr.get(var.asVar()).deepCopy();
-            }
-            else return var.deepCopy();
+
+        private boolean inTopLevel() {
+            return stack.size() == 0 ;
         }
     }
 }

http://git-wip-us.apache.org/repos/asf/jena/blob/3758cecd/jena-arq/src/test/java/org/apache/jena/sparql/algebra/TestOpAsQuery.java
----------------------------------------------------------------------
diff --git a/jena-arq/src/test/java/org/apache/jena/sparql/algebra/TestOpAsQuery.java b/jena-arq/src/test/java/org/apache/jena/sparql/algebra/TestOpAsQuery.java
index 2cc2d82..671b2ee 100644
--- a/jena-arq/src/test/java/org/apache/jena/sparql/algebra/TestOpAsQuery.java
+++ b/jena-arq/src/test/java/org/apache/jena/sparql/algebra/TestOpAsQuery.java
@@ -20,7 +20,6 @@ package org.apache.jena.sparql.algebra;
 
 import static org.junit.Assert.assertEquals ;
 import static org.junit.Assert.assertFalse ;
-import static org.junit.Assert.assertNotEquals ;
 import static org.junit.Assert.assertTrue ;
 
 import java.util.Map ;
@@ -29,6 +28,8 @@ import org.apache.jena.atlas.lib.StrUtils ;
 import org.apache.jena.query.Query ;
 import org.apache.jena.query.QueryFactory ;
 import org.apache.jena.query.Syntax ;
+import org.apache.jena.sparql.algebra.Algebra ;
+import org.apache.jena.sparql.algebra.Op ;
 import org.junit.Assert ;
 import org.junit.Test ;
 
@@ -37,30 +38,34 @@ import org.junit.Test ;
  */
 public class TestOpAsQuery {
 
+    // basic stuff
+    @Test public void testBasic01() { test$RoundTripQuery("SELECT * { }") ; }
+    @Test public void testBasic02() { test$RoundTripQuery("SELECT * { ?s ?p ?o }") ; }
+    @Test public void testBasic03() { test$RoundTripQuery("SELECT * { ?s ?p ?o FILTER(?o > 5) }") ; }
+    @Test public void testBasic04() { test$RoundTripQuery("SELECT ?s { ?s ?p ?o FILTER(?o > 5) }") ; }
+    @Test public void testBasic05() { test$RoundTripQuery("SELECT ?s (?o + 5 AS ?B) { ?s ?p ?o }") ; }
+    
     /**
      * Test of asQuery method, of class OpAsQuery.
      */
     @Test
     public void testCountStar() {
-        Object[] result = checkQuery("select (count(*) as ?cs) { ?s ?p ?o }");
-        assertEquals(result[0], result[1]);
+        test$RoundTripQuery("select (count(*) as ?cs) { ?s ?p ?o }");
     }
     
     @Test
     public void testCountGroup() {
-        Object[] result = checkQuery("select (count(?p) as ?cp) { ?s ?p ?o } group by ?s");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("select (count(?p) as ?cp) { ?s ?p ?o } group by ?s");
     }
     
     @Test
     public void testCountGroupAs() {
-        Object[] result = checkQuery("select (count(?p) as ?cp) { ?s ?p ?o }");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("select (count(?p) as ?cp) { ?s ?p ?o }");
     }
     
     @Test
     public void testDoubleCount() {
-        Query[] result = checkQuery("select (count(?s) as ?sc) (count(?p) as ?pc) { ?s ?p ?o }");
+        Query[] result = test$RoundTripQuery("select (count(?s) as ?sc) (count(?p) as ?pc) { ?s ?p ?o }") ;
         assertEquals(2, result[1].getResultVars().size());
         assertTrue(result[1].getResultVars().contains("sc"));
         assertTrue(result[1].getResultVars().contains("pc"));
@@ -69,100 +74,139 @@ public class TestOpAsQuery {
     /* JENA-166 */
     @Test
     public void testGroupWithExpression() {
-        Object[] result = checkQuery("SELECT (sample(?a) + 1 AS ?c) {} GROUP BY ?x");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT (sample(?a) + 1 AS ?c) {} GROUP BY ?x");
     }
     
+    /* Coverage developed for JENA-963 : GROUP BY*/
+    @Test public void testGroupBy_01()
+    { test$RoundTripQuery("SELECT ?s { ?s ?p ?o } GROUP BY ?s"); }
+    
+    @Test public void testGroupBy_02()
+    { test$RoundTripQuery("SELECT (count(?p) as ?cp) { ?s ?p ?o } GROUP BY ?s"); }
+    
+    @Test public void testGroupBy_03()
+    { test$RoundTripQuery("SELECT ?s { ?s ?p ?o } GROUP BY ?s HAVING (count(*) > 1 )"); }
+    
+    @Test public void testGroupBy_04()
+    { test$RoundTripQuery("SELECT ?s { ?s ?p ?o } GROUP BY ?s HAVING (?s > 1 )"); }
+    
+    @Test public void testGroupBy_05()
+    { test$RoundTripQuery("SELECT (count(?p) as ?cp) { ?s ?p ?o } GROUP BY ?s HAVING (?cp > 1 )"); }
+    
+    @Test public void testGroupBy_06()
+    { test$RoundTripQuery("SELECT (count(?p) as ?cp) { ?s ?p ?o } GROUP BY (abs(?o)) HAVING (?cp > 1 )"); }
+    
+    @Test public void testGroupBy_07()
+    { test$RoundTripQuery("SELECT (?X+2 AS ?Y) (count(?p) as ?cp) ?Z (1/?X AS ?X1) { ?s ?p ?o } GROUP BY ?Z (abs(?o) AS ?X) HAVING (?cp > 1 )"); }
+    
+    @Test public void testGroupBy_08()
+    { test$RoundTripQuery("SELECT (count(?p) as ?cp) { ?s ?p ?o } GROUP BY (abs(?o)) HAVING (?cp > 1 )"); }
+    
+    @Test public void testGroupBy_09()
+    { test$RoundTripQuery("SELECT (count(?p) as ?cp) { ?s ?p ?o } GROUP BY (abs(?o)) ORDER BY (COUNT(*))"); }
+    
+    @Test public void testGroupBy_10()
+    { test$RoundTripQuery("SELECT (7+count(?p) as ?cp) { ?s ?p ?o } GROUP BY (abs(?o)) HAVING (?cp > 1 && SUM(?o) > 99 ) ORDER BY (6+COUNT(*))"); }
+    
+    @Test public void testGroupBy_11()
+    { test$RoundTripQuery("SELECT ?X { ?s ?p ?o } GROUP BY (abs(?o) AS ?X) HAVING (?cp > 1 )"); }
+    
+    @Test public void testGroupBy_12()
+    { test$RoundTripQuery("SELECT * { ?s ?q ?z {SELECT DISTINCT * { ?s ?p ?o }} }"); } 
+    
+    @Test public void testSubQuery_01()
+    { test$RoundTripQuery("SELECT ?s { SELECT (count(*) as ?cp) { ?s ?p ?o } }") ; }
+    
+    @Test public void testSubQuery_02()
+    { test$RoundTripQuery("SELECT ?s { ?s ?p ?o { SELECT (count(*) as ?cp) { ?s ?p ?o } }}") ; }
+    
+    @Test public void testSubQuery_03()
+    //{ testRoundTripQuery("SELECT ?s { { SELECT (count(*) as ?cp) { ?s ?p ?o } } ?s ?p ?o }") ; }
+    // The trailing ?s ?p ?o gets a {} round it.
+    { test$RoundTripAlegbra("SELECT ?s { { SELECT (count(*) as ?cp) { ?s ?p ?o } } ?s ?p ?o }") ; }
+    
+    @Test public void testSubQuery_04()
+    { test$RoundTripQuery("SELECT * WHERE { ?s ?p ?o . BIND(?o AS ?x) }") ; }
+    
+    @Test public void testSubQuery_05()
+    { test$RoundTripQuery("SELECT (?o AS ?x) WHERE { ?s ?p ?o .}") ; }
+    
     @Test
     public void testProject1() {
-        Object[] result = checkQuery("SELECT (?x + 1 AS ?c) {}");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT (?x + 1 AS ?c) {}");
     }
     
     @Test
     public void testProject2() {
-        Query[] result = checkQuery("SELECT (?x + 1 AS ?c) ?d {}");
+        Query[] result = test$RoundTripQuery("SELECT (?x + 1 AS ?c) ?d {}");
         assertEquals(2, result[1].getResultVars().size());
         assertTrue(result[1].getResultVars().contains("c"));
         assertTrue(result[1].getResultVars().contains("d"));
     }
     
-    // This BIND is distinguisable, however
     @Test
     public void testNestedBind() {
-        Object[] result = checkQuery("SELECT ?c { { } UNION { BIND(?x + 1 AS ?c) } }");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT ?c { { } UNION { BIND(?x + 1 AS ?c) } }");
     }
     
     @Test
     public void testNestedProject() {
-        Object[] result = checkQuery("SELECT (?x + 1 AS ?c) { { } UNION { } }");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT (?x + 1 AS ?c) { { } UNION { } }");
     }
     
     @Test
     public void testGroupExpression() {
-        Object[] result = checkQuery("SELECT ?z { } GROUP BY (?x + ?y AS ?z)");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT ?z { } GROUP BY (?x + ?y AS ?z)");
     }
     
     @Test
     public void testNestedProjectWithGroup() {
-        Object[] result = checkQuery("SELECT (SAMPLE(?c) as ?s) { {} UNION {BIND(?x + 1 AS ?c)} } GROUP BY ?x");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT (SAMPLE(?c) as ?s) { {} UNION {BIND(?x + 1 AS ?c)} } GROUP BY ?x");
     }
     
     @Test
     public void testQuadPatternInDefaultGraph() {
-        Object[] result = checkQuadQuery("SELECT * WHERE { ?s a ?type }");
-        assertEquals(result[0], result[1]);
+        test$RoundTripQueryQuads("SELECT * WHERE { ?s a ?type }");
     }
     
     @Test
     public void testGraphClauseUri() {
-        Object[] result = checkQuery("SELECT * WHERE { GRAPH <http://example> { ?s a ?type } }");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT * WHERE { GRAPH <http://example> { ?s a ?type } }");
     }
     
     @Test
     public void testGraphClauseComplex() {
-        Object[] result = checkQuery("SELECT * WHERE { GRAPH <http://example> { ?s a ?type . OPTIONAL { ?s <http://label> ?label } } }");
-        assertEquals(result[0], result[1]);
+		test$RoundTripQuery("SELECT * WHERE { GRAPH <http://example> { ?s a ?type . OPTIONAL { ?s <http://label> ?label } } }");
     }
     
     @Test
     public void testQuadPatternInGraph() {
-        Object[] result = checkQuadQuery("SELECT * WHERE { GRAPH <http://example> { ?s a ?type } }");
-        assertEquals(result[0], result[1]);
+        test$RoundTripQueryQuads("SELECT * WHERE { GRAPH <http://example> { ?s a ?type } }");
     }
     
     @Test
     public void testQuadPatternInGraphComplex01() {
         //This fails because OpQuadPattern's are converted back to individual GRAPH clauses
-        Object[] result = checkQuadQuery("SELECT * WHERE { GRAPH <http://example> { ?s a ?type . OPTIONAL { ?s <http://label> ?label } } }");
+        Object[] result = roundTripQueryQuad("SELECT * WHERE { GRAPH <http://example> { ?s a ?type . OPTIONAL { ?s <http://label> ?label } } }");
         assertFalse(result[0].equals(result[1]));
     }
     
     @Test
     public void testQuadPatternInGraphComplex02() {
         //This succeeds since each OpQuadPattern is from a single simple GRAPH clause
-        Object[] result = checkQuadQuery("SELECT * WHERE { GRAPH <http://example> { ?s a ?type } OPTIONAL { GRAPH <http://example> { ?s <http://label> ?label } } }");
-        assertEquals(result[0], result[1]);
+        test$RoundTripQueryQuads("SELECT * WHERE { GRAPH <http://example> { ?s a ?type } OPTIONAL { GRAPH <http://example> { ?s <http://label> ?label } } }");
     }
     
     @Test
     public void testExtend1() {
-        //Top Level BIND should now be round trippable
-        Query[] result = checkQuery("SELECT * WHERE { ?s ?p ?o . BIND(?o AS ?x) }");
-        assertNotEquals(result[0], result[1]);
-        assertTrue(result[1].getResultVars().contains("x"));
+        // Top Level BIND should now be round trippable
+		test$RoundTripQuery("SELECT * WHERE { ?s ?p ?o . BIND(?o AS ?x) }");
     }
     
     @Test
     public void testExtend2() {
-        //Nested BIND should always have been round trippable
-        Query[] result = checkQuery("SELECT * WHERE { GRAPH ?g { ?s ?p ?o . BIND(?o AS ?x) } }");
-        assertEquals(result[0], result[1]);
+        // Nested BIND should always have been round trippable
+		test$RoundTripQuery("SELECT * WHERE { GRAPH ?g { ?s ?p ?o . BIND(?o AS ?x) } }");
     }
     
     @Test
@@ -180,58 +224,52 @@ public class TestOpAsQuery {
                  "   :documentType \"exam results\" ." ,
                  "    BIND( mylib:DateFormat( xsd:string(?date), \"yyyy-MM\" ) as ?yearmonth )",
                 "} group by ?yearmonth") ;
-        checkQueryParseable(query, true);
+        test$RoundTripQuery(query);
     }
     
     @Test
     public void testExtend4() {
         //Simplified repo of JENA-429
-        String query  = "SELECT ?key (COUNT(?member) AS ?total) WHERE { ?s ?p ?o . BIND(LCASE(?o) AS ?key) } GROUP BY ?key";
-        checkQueryParseable(query, true);
+        test$RoundTripQuery("SELECT ?key (COUNT(?member) AS ?total) WHERE { ?s ?p ?o . BIND(LCASE(?o) AS ?key) } GROUP BY ?key");
     }
     
     @Test
     public void testExtendInService() {
         //Original test case from JENA-422
-        Query[] result = checkQuery("SELECT * WHERE { SERVICE <http://example/endpoint> { ?s ?p ?o . BIND(?o AS ?x) } }");
-        assertEquals(result[0], result[1]);
+        Query[] result = test$RoundTripQuery("SELECT * WHERE { SERVICE <http://example/endpoint> { ?s ?p ?o . BIND(?o AS ?x) } }");
         assertTrue(result[1].toString().contains("BIND"));
     }
     
     @Test
     public void testSubQuery1() {
-        String query = "SELECT ?s WHERE { SELECT ?s ?p WHERE { ?s ?p ?o } }";
-        checkQueryParseable(query, true);
+        test$RoundTripQuery("SELECT ?s WHERE { SELECT ?s ?p WHERE { ?s ?p ?o } }");
     }
     
     @Test
     public void testSubQuery2() {
         String query = "SELECT ?s ?x WHERE { { SELECT ?s ?p WHERE { ?s ?p ?o } } { SELECT ?x WHERE { ?x ?p ?o } } }";
-        //These end up being non-equal queries because the nesting in the final query is a little funky
-        //but the results should still be semantically equivalent
-        checkQueryParseable(query, false);
+        // The second inner sub-query is specially fixed up  in OpJoin processing.
+        // Not all cases of sub-query have unnecessary {} removed.
+		test$RoundTripQuery(query) ;
     }
     
     @Test
     public void testSubQuery3() {
         String query = "SELECT * WHERE { { SELECT ?s ?p WHERE { ?s ?p ?o } } { SELECT ?x WHERE { ?x ?p ?o } } }";
-        //In this case there is insufficient information to correctly reverse translate the algebra so this query
-        //will not round trip
-        checkQueryNonRecoverable(query);
+		test$RoundTripQuery(query) ;
     }
     
     @Test
     public void testAggregatesInSubQuery1() {
         //Simplified form of a test case provided via the mailing list (JENA-445)
         String query = "SELECT ?key ?agg WHERE { SELECT ?key (COUNT(*) AS ?agg) { ?key ?p ?o } GROUP BY ?key }";
-        checkQueryParseable(query, true);
+        test$RoundTripQuery(query);
     }
     
     @Test
     public void testAggregatesInSubQuery2() {
         //Simplified form of a test case provided via the mailing list (JENA-445)
-        String query = "SELECT * WHERE { { SELECT ?key (COUNT(*) AS ?agg) { ?key ?p ?o } GROUP BY ?key } }";
-        checkQueryParseable(query, false);
+        test$RoundTripAlegbra("SELECT * WHERE { { SELECT ?key (COUNT(*) AS ?agg) { ?key ?p ?o } GROUP BY ?key } }");
     }
     
     @Test
@@ -258,7 +296,7 @@ public class TestOpAsQuery {
                 "}GROUP by ?country_cat \n" + 
                 "} \n" + 
                 "}\n"; 
-        checkQuadQuery(queryString);
+        test$RoundTripQuery(queryString);
     }
     
     @Test
@@ -272,7 +310,7 @@ public class TestOpAsQuery {
                                           "    } LIMIT 1",
                                           "}");
         
-        checkQueryParseable(query, true);
+        test$RoundTripQuery(query) ;
     }
     
     @Test
@@ -286,7 +324,7 @@ public class TestOpAsQuery {
                                           "    } LIMIT 1",
                                           "}");
         
-        checkQueryParseable(query, true);
+        test$RoundTripQuery(query);
     }
     
     @Test
@@ -300,7 +338,7 @@ public class TestOpAsQuery {
                                           "    } LIMIT 1",
                                           "}");
         
-        checkQueryParseable(query, true);
+        test$RoundTripQuery(query);
     }
     
     @Test
@@ -314,37 +352,69 @@ public class TestOpAsQuery {
                                           "    } OFFSET 1",
                                           "}");
         
-        checkQueryParseable(query, true);
+        test$RoundTripQuery(query);
     }
     
     @Test
     public void testPathExpressions1() {
         // test that the query after serialization is legal (as much a test of the serializer as way OpAsQuery works)
         String query = "PREFIX : <http://example/> SELECT * { ?s :p* ?o . ?x :r 123 . }" ;
-        Query r[] = checkQueryParseable(query, false);
+        test$RoundTripAlegbra(query);
     }
         
     @Test
     public void testPathExpressions2() {
         // test that the query  
         String query = "PREFIX : <http://example/> SELECT * { ?s :p*/:q ?o . ?x :r 123 . }" ;
-        Query r[] = checkQueryParseable(query, false);
+        test$RoundTripAlegbra(query);
     }
 
     @Test
     public void testMinus1() {
-        String query = "PREFIX : <http://example/> SELECT * { ?s :p ?o MINUS { ?s :q ?v .FILTER(?v<5) } }" ; 
-        Query r[] = checkQueryParseable(query, true);
+        test$RoundTripQuery("PREFIX : <http://example/> SELECT * { ?s :p ?o MINUS { ?s :q ?v .FILTER(?v<5) } }") ; 
     }
     
     @Test
     public void testMinus2() {
         // query gains a level of {} but the meaning is the same. 
         String query = "PREFIX : <http://example/> SELECT * { ?s :p ?o OPTIONAL { ?s :x ?2 } MINUS { ?s :q ?v .FILTER(?v<5) } }" ; 
-        Query r[] = checkQueryParseable(query, false);
+       test$RoundTripAlegbra(query);
+    }
+    
+    // Test for queries that do query->algebra->OpAsQuery->query
+    // to produce an output that is .equals the input.
+    public static Query[] test$RoundTripQuery(String query) {
+        Query[] r = roundTripQuery(query) ;
+        stripNamespacesAndBase(r[0]) ; 
+        stripNamespacesAndBase(r[1]) ;
+        assertEquals(r[0], r[1]) ;
+        return r ;
     }
     
-    public Query[] checkQuery(String query) {
+    // Test via quads  
+    public static Query[] test$RoundTripQueryQuads(String query) {
+        Query[] r = roundTripQueryQuad(query) ;
+        assertEquals(r[0], r[1]) ;
+        return r ;
+    }
+
+
+    // Compare A1 and A2 where 
+    //  query[Q1]->algebra[A1]->OpAsQuery->query[Q2]->algebra[A2]
+    // Sometimes Q1 and Q2 are equivalent but not .equals.  
+    public void test$RoundTripAlegbra(String query) {
+        Query[] r = roundTripQuery(query);
+        
+        // Even if the strings come out as non-equal because of the translation from algebra to query
+        // the algebras should be equal
+        // i.e. the queries should remain semantically equivalent
+        Op a1 = Algebra.compile(r[0]);
+        Op a2 = Algebra.compile(r[1]);
+        Assert.assertEquals(a1, a2);
+    }
+
+    // query->algebra->OpAsQuery->query
+    private static Query[] roundTripQuery(String query) {
         Query orig = QueryFactory.create(query, Syntax.syntaxSPARQL_11);
         Op toReconstruct = Algebra.compile(orig);
         Query got = OpAsQuery.asQuery(toReconstruct);
@@ -352,7 +422,8 @@ public class TestOpAsQuery {
         return r;
     }
     
-    public Query[] checkQuadQuery(String query) {
+ // query->algebra/quads->OpAsQuery->query
+    private static Query[] roundTripQueryQuad(String query) {
         Query orig = QueryFactory.create(query, Syntax.syntaxSPARQL_11);
         Op toReconstruct = Algebra.compile(orig);
         toReconstruct = Algebra.toQuadForm(toReconstruct);
@@ -361,33 +432,8 @@ public class TestOpAsQuery {
         return r;
     }
     
-    public Query[] checkQueryParseable(String query, boolean expectEquals) {
-        Query[] r = checkQuery(query);
-        
-        // Strip namespaces and Base URI from each so comparison is not affected by those
-        stripNamespacesAndBase(r[0]);
-        stripNamespacesAndBase(r[1]);
-        
-        if (expectEquals) {
-            // Expecting the string forms of the queries to be equal
-            // If the strings forms are equal their algebras will be
-            Assert.assertEquals(r[0], r[1]);
-        } else {
-            // Even if the strings come out as non-equal because of the translation from algebra to query
-            // the algebras should be equal
-            // i.e. the queries should remain semantically equivalent
-            Assert.assertNotEquals(r[0], r[1]);
-            Op a1 = Algebra.compile(r[0]);
-            Op a2 = Algebra.compile(r[1]);
-            Assert.assertEquals(a1, a2);
-        }
-        String query2 = r[1].toString();
-        Query q = QueryFactory.create(query2);
-        return r;
-    }
-    
-    public Query[] checkQueryNonRecoverable(String query) {
-        Query[] r = checkQuery(query);
+    public static void checkQueryNonRecoverable(String query) {
+        Query[] r = roundTripQuery(query);
         
         // Strip namespaces and Base URI from each so comparison is not affected by those
         stripNamespacesAndBase(r[0]);
@@ -400,11 +446,9 @@ public class TestOpAsQuery {
         Op a1 = Algebra.compile(r[0]);
         Op a2 = Algebra.compile(r[1]);
         Assert.assertNotEquals(a1, a2);
-        
-        return r;
     }
     
-    protected void stripNamespacesAndBase(Query q) {
+    protected static void stripNamespacesAndBase(Query q) {
         Map<String, String> prefixes = q.getPrefixMapping().getNsPrefixMap();
         for (String prefix : prefixes.keySet()) {
             q.getPrefixMapping().removeNsPrefix(prefix);