Merge pull request #666 from bserdar/assembler-inner-join

Fix #665: If there are query clauses for non-root nodes, we have to f…

Author: paterczm

crud/src/main/java/com/redhat/lightblue/assoc/CompositeFindImpl.java

@@ -123,7 +123,8 @@ private void initialize(OperationContext ctx,
             selectQueryPlan(req.getQuery(), minimalTree);
             LOGGER.debug("Search query plan:{}, retrieval query plan:{}", searchQPlan, retrievalQPlan);
 
-            executionPlan = new ExecutionPlan(req.getProjection(),
+            executionPlan = new ExecutionPlan(req.getQuery(),
+                                              req.getProjection(),
                                               req.getSort(),
                                               req.getFrom(),
                                               req.getTo(),

crud/src/main/java/com/redhat/lightblue/assoc/ep/ExecutionPlan.java

@@ -83,6 +83,7 @@ public class ExecutionPlan {
     /**
      * Creates an execution plan
      *
+     * @param requestQuery query requested by the client
      * @param requestProjection projection requested by the client
      * @param requestSort sort requested by the client.
      * @param from request.from
@@ -95,7 +96,8 @@ public class ExecutionPlan {
      * documents found by that search. If searchQueryPlan is null, this plan
      * performs the search and retrieval.
      */
-    public ExecutionPlan(Projection requestProjection,
+    public ExecutionPlan(QueryExpression requestQuery,
+                         Projection requestProjection,
                          Sort requestSort,
                          Long from,
                          Long to,
@@ -229,6 +231,14 @@ public ExecutionPlan(Projection requestProjection,
         // Done with the search plan. Now we build the execution plan for retrieval
         LOGGER.debug("Building execution plan from retrieval query plan:{}", retrievalQueryPlan);
         List<Conjunct> unassigned = retrievalQueryPlan.getUnassignedClauses();
+        boolean queries_in_non_root_nodes=false;
+        for(QueryPlanNode node:retrievalQueryPlan.getAllNodes()) {
+            if(node.getSources().length!=0&& // non-root
+               !node.getData().getConjuncts().isEmpty()) {
+                queries_in_non_root_nodes=true;
+                break;
+            }
+        }
         List<QueryFieldInfo> qfi = getAllQueryFieldInfo(retrievalQueryPlan);
         for (QueryPlanNode node : retrievalQueryPlan.getAllNodes()) {
             ExecutionBlock block = qp2BlockMap.get(node);
@@ -242,6 +252,7 @@ public ExecutionPlan(Projection requestProjection,
                 // If there is a search plan, then we already have the root documents, so simply stream
                 // those docs from the search plan. If there is not a search plan, we search the documents
                 // here
+                boolean needsFilter=false;
                 Source<ResultDocument> last;
                 AbstractSearchStep search;
                 if (queryRoot != null) {
@@ -252,22 +263,15 @@ public ExecutionPlan(Projection requestProjection,
                     //There is not a search plan. We'll search documents here
                     search = new Search(block);
                     last = new Source<>(search);
-                    if (unassigned.isEmpty()) {
-                        // There are no unassigned clauses
+                    if (unassigned.isEmpty()&&!queries_in_non_root_nodes) {
+                        // There are no unassigned clauses or queries in non-root nodes
                         // We can sort/limit here
                         search.setLimit(from, to);
                         search.setSort(requestSort);
                     } else {
                         // There are unassigned clauses. We can sort during search, but we have to filter and limit
                         search.setSort(requestSort);
-                        Filter f = new Filter(block, new Source<>(search), getFilterQuery(unassigned));
-                        last = new Source<>(f);
-                        if (from != null) {
-                            last = new Source<>(new Skip(block, from.intValue(), last));
-                        }
-                        if (to != null) {
-                            last = new Source<>(new Limit(block, to.intValue() - from.intValue() + 1, last));
-                        }
+                        needsFilter=true;
                     }
                 }
                 search.recordResultSetSize(true);
@@ -277,6 +281,15 @@ public ExecutionPlan(Projection requestProjection,
                 search.setProjection(writeProjection(fields));
                 search.setQueries(node.getData().getConjuncts());
                 resultStep = new Assemble(block, last, destinationBlocks);
+                if(needsFilter) {
+                    resultStep = new Filter(block, new Source<>(resultStep), requestQuery);
+                    if (from != null) {
+                        resultStep = new Skip(block, from.intValue(), new Source<>(resultStep));
+                    }
+                    if (to != null) {
+                        resultStep = new Limit(block, to.intValue() - from.intValue() + 1, new Source<>(resultStep));
+                    }
+                }
                 resultStep = new Project(block, new Source<>(resultStep), requestProjection);
                 block.setResultStep(resultStep);
             } else {
@@ -447,11 +460,6 @@ public static Set<Path> getIncludedFieldsOfEntityForSort(ExecutionBlock block) {
         return ret;
     }
 
-    private QueryExpression getFilterQuery(List<Conjunct> list) {
-        List<QueryExpression> l = list.stream().map(c -> c.getClause()).collect(Collectors.toList());
-        return Searches.and(l);
-    }
-
     public JsonNode toJson() {
         return resultStep.toJson();
     }

crud/src/test/java/com/redhat/lightblue/assoc/ep/ExecutionPlanTest.java

@@ -94,7 +94,7 @@ public void retrieveAandBonly() throws Exception {
                 new IndexedFieldScorer(),
                 q,
                 minimalTree).choose();
-        ExecutionPlan ep = new ExecutionPlan(p, null, null, null, md, null, searchQP);
+        ExecutionPlan ep = new ExecutionPlan(q,p, null, null, null, md, null, searchQP);
         ObjectNode j = (ObjectNode) ep.toJson();
 
         System.out.println("retrieveAandBonly");
@@ -129,7 +129,7 @@ public void retrieveABC() throws Exception {
                 q,
                 minimalTree).choose();
         QueryPlan retrievalQP = new QueryPlanChooser(md, new First(), new SimpleScorer(), null, minimalTree).choose();
-        ExecutionPlan ep = new ExecutionPlan(p, null, null, null, md, searchQP, retrievalQP);
+        ExecutionPlan ep = new ExecutionPlan(q,p, null, null, null, md, searchQP, retrievalQP);
         ObjectNode j = (ObjectNode) ep.toJson();
 
         System.out.println("retrieveABC");
@@ -170,7 +170,7 @@ public void retrieveAandConly_CFirst() throws Exception {
                 q,
                 minimalTree).choose();
 
-        ExecutionPlan ep = new ExecutionPlan(p, null, null, null, md, searchQP, retrievalQP);
+        ExecutionPlan ep = new ExecutionPlan(q,p, null, null, null, md, searchQP, retrievalQP);
         ObjectNode j = (ObjectNode) ep.toJson();
 
         System.out.println("retrieveAandConly_CFirst");
@@ -224,7 +224,7 @@ public void rev_search_with_arraycond() throws Exception {
                 q,
                 minimalTree).choose();
 
-        ExecutionPlan ep = new ExecutionPlan(p, null, null, null, md, searchQP, retrievalQP);
+        ExecutionPlan ep = new ExecutionPlan(q,p, null, null, null, md, searchQP, retrievalQP);
         ObjectNode j = (ObjectNode) ep.toJson();
 
         System.out.println("rev_search_with_arraycond");

crud/src/test/java/com/redhat/lightblue/mediator/CompositeFinderTest.java

@@ -659,4 +659,16 @@ public void assocQWithNull2() throws Exception {
         Assert.assertEquals(1, response.getEntityData().size());
     }
 
+    @Test
+    public void dont_do_outer_joins() throws Exception {
+        // We need to use a_with_index in this test, so the execution plan becomes A->B instead of B-> A
+        FindRequest fr = new FindRequest();
+        fr.setQuery(query("{'$and': [ {'field':'_id','op':'$in','values':['A99','ADEEP']}, {'field':'level1.arr1.*.ref.*.field1','op':'=','rvalue':'bdeep1'} ] }"));
+        fr.setProjection(projection("[{'field':'*','recursive':1},{'field':'level1.arr1.*.ref','recursive':1}]"));
+        fr.setEntityVersion(new EntityVersion("A_with_index", "1.0.0"));
+        Response response = mediator.find(fr);
+        System.out.println(response.getEntityData());
+        Assert.assertEquals(1, response.getEntityData().size());
+    }
+
 }

crud/src/test/resources/composite/A_with_index.json

@@ -0,0 +1,83 @@
+{
+  "entityInfo" : {
+    "name": "A_with_index",
+    "datastore": {
+        "backend":"mongo",
+        "datasource": "mongodata",
+        "collection": "user"
+    },
+      "indexes":[
+          {
+              "fields":[ {"field":"_id","dir":"$asc"} ]
+          }
+          ]
+  },
+  "schema" : {
+    "name" : "A_with_index",
+    "version": {
+        "value": "1.0.0",
+        "changelog": "Test"
+    },
+    "status": {
+        "value": "active"
+    },
+    "access" : {
+        "insert": ["anyone"],
+        "find":["anyone"],
+        "update":["anyone"],
+        "delete":["anyone"]
+    },
+    "fields": {
+        "_id": {"type": "string", "constraints":{ "identity":1 } },
+        "objectType": {"type": "string"},
+        "field1": { "type": "string" },
+        "obj1": {
+            "type":"object",
+            "fields": {
+                 "field1": { "type":"string" },
+                 "c_ref":{"type":"string"},
+                 "c": {
+                    "type":"reference",
+                    "entity":"C",
+                    "versionValue":"1.0.0",
+                    "query":{"field":"_id","op":"$eq","rfield":"$parent.c_ref"}
+                 }
+            }
+        },
+        "b_ref": { "type": "string" },
+        "b" : {
+            "type":"reference",
+            "entity":"B",
+            "versionValue":"1.0.0",
+            "query":{ "field":"_id","op":"$eq","rfield":"$parent.b_ref"}
+        },
+        "nonid_b_ref":{"type":"string"},
+        "nonid_b" : {
+             "type":"reference",
+             "entity":"B",
+             "versionValue":"1.0.0",
+             "query":{"field":"field1","op":"$eq","rfield":"$parent.nonid_b_ref"}
+        },
+        "level1": {
+            "type":"object",
+            "fields": {
+                "arr1": {
+                   "type":"array",
+                   "items": {
+                      "type":"object",
+                      "fields": {
+                          "b_ref":{"type":"string"},
+                          "ref": {
+                             "type":"reference",
+                             "entity":"B",
+                             "versionValue":"1.0.0",
+                             "query":{"field":"_id","op":"=","rfield":"$parent.b_ref"}
+                          }
+                      }
+                    }
+                }
+            }
+        }
+    }
+  }
+}

crud/src/test/resources/composite/A_with_index_data.json

@@ -0,0 +1,32 @@
+[{
+  "_id":"A99",
+  "objectType":"A",
+  "field1":"dwzedQr4hw5HdHwWhvE7SkvZB22",
+  "obj1":{
+    "field1":"d6gdfa8wh7F6l6Iq.KV8",
+    "c_ref":"C99"
+    },
+  "b_ref":"B99"
+  },{
+  "_id":"ADEEP",
+  "objectType":"A",
+  "level1": {
+    "arr1": [
+       { 
+         "b_ref":"BDEEP1"
+       },
+       { 
+         "b_ref":"BDEEP2"
+       }
+    ]
+  }
+  },{
+  "_id":"MANYB1",
+  "objectType":"A",
+  "nonid_b_ref":"MANYB1"
+  },{
+  "_id":"MANYB2",
+  "objectType":"A",
+  "nonid_b_ref":"MANYB2"
+  }
+]

crud/src/test/resources/composite/U_data.json

@@ -1,5 +1,5 @@
 [ { "_id":1,"status":"active","authentications":[ { "uid":"1","principal":"a","providerName":"p" } ],"legalEntities":[ {"legalEntityId":1,"title":"s","emails":[{"address":"[email protected]"}] } ],"objectType":"U" },
   { "_id":2,"status":"active","authentications":[ { "uid":"1","principal":"a" } ],"legalEntities":[ {"legalEntityId":1,"title":"s" } ],"objectType":"U" },
-  { "_id":3,"status":"active","authentications":[ { "uid":"1","principal":"b" } ],"legalEntities":[ {"legalEntityId":2,"title":"s" } ],"objectType":"U" }  
+  { "_id":3,"status":"active","authentications":[ { "uid":"1","principal":"b" } ],"legalEntities":[ {"legalEntityId":2,"title":"s" } ],"objectType":"U" } 
   ]