LUCENE-5805: QueryNodeImpl.removeFromParent was doing nothing, in a costly way

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1683839 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -74,6 +74,9 @@ Bug fixes
 * LUCENE-6520: Geo3D GeoPath.done() would throw an NPE if adjacent path
   segments were co-linear. (Karl Wright via David Smiley)
 
+* LUCENE-5805: QueryNodeImpl.removeFromParent was doing nothing in a
+  costly manner (Christoph Kaser, Cao Manh Dat via Mike McCAndless)
+
 Changes in Runtime Behavior
 
 * LUCENE-6501: The subreader structure in ParallelCompositeReader

lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNode.java

@@ -95,4 +95,11 @@ public interface QueryNode {
    * Removes this query node from its parent.
    */
   public void removeFromParent();
+
+
+  /**
+   * Remove a child node
+   * @param childNode Which child to remove
+   */
+  public void removeChildren(QueryNode childNode);
 }

lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNodeImpl.java

@@ -246,20 +246,24 @@ public abstract class QueryNodeImpl implements QueryNode, Cloneable {
   public Map<String, Object> getTagMap() {
     return (Map<String, Object>) this.tags.clone();
   }
-  
+
+  @Override
+  public void removeChildren(QueryNode childNode){
+    Iterator<QueryNode> it = this.clauses.iterator();
+    while(it.hasNext()){
+      if(it.next() == childNode){
+        it.remove();
+      }
+    }
+    childNode.removeFromParent();
+  }
+
   @Override
   public void removeFromParent() {
     if (this.parent != null) {
-      List<QueryNode> parentChildren = this.parent.getChildren();
-      Iterator<QueryNode> it = parentChildren.iterator();
-      
-      while (it.hasNext()) {
-        if (it.next() == this) {
-          it.remove();
-        }
-      }
-      
+      QueryNode parent = this.parent;
       this.parent = null;
+      parent.removeChildren(this);
     }
   }
 

lucene/queryparser/src/test/org/apache/lucene/queryparser/flexible/core/nodes/TestQueryNode.java

@@ -44,24 +44,39 @@ public class TestQueryNode extends LuceneTestCase {
     assertTrue(node.getTag("tAg") != null);
     
   }
-  
+
+
   /* LUCENE-5099 - QueryNodeProcessorImpl should set parent to null before returning on processing */
   public void testRemoveFromParent() throws Exception {
     BooleanQueryNode booleanNode = new BooleanQueryNode(Collections.<QueryNode>emptyList());
     FieldQueryNode fieldNode = new FieldQueryNode("foo", "A", 0, 1);
     assertNull(fieldNode.getParent());
-    
+
     booleanNode.add(fieldNode);
     assertNotNull(fieldNode.getParent());
 
     fieldNode.removeFromParent();
     assertNull(fieldNode.getParent());
+    /* LUCENE-5805 - QueryNodeImpl.removeFromParent does a lot of work without any effect */
+    assertFalse(booleanNode.getChildren().contains(fieldNode));
 
     booleanNode.add(fieldNode);
     assertNotNull(fieldNode.getParent());
-    
+
     booleanNode.set(Collections.<QueryNode>emptyList());
     assertNull(fieldNode.getParent());
   }
+
+  public void testRemoveChildren() throws Exception{
+    BooleanQueryNode booleanNode = new BooleanQueryNode(Collections.<QueryNode>emptyList());
+    FieldQueryNode fieldNode = new FieldQueryNode("foo", "A", 0, 1);
+
+    booleanNode.add(fieldNode);
+    assertTrue(booleanNode.getChildren().size() == 1);
+
+    booleanNode.removeChildren(fieldNode);
+    assertTrue(booleanNode.getChildren().size()==0);
+    assertNull(fieldNode.getParent());
+  }
   
 }