diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index d0f7859b3c1..6026ecf9625 100644 --- a/lucene/CHANGES.txt +++ b/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 diff --git a/lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNode.java b/lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNode.java index 42ec586b3fa..4bcd1589cb4 100644 --- a/lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNode.java +++ b/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); } diff --git a/lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNodeImpl.java b/lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNodeImpl.java index 6e3de271dfd..6ff642d24d2 100644 --- a/lucene/queryparser/src/java/org/apache/lucene/queryparser/flexible/core/nodes/QueryNodeImpl.java +++ b/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 getTagMap() { return (Map) this.tags.clone(); } - + + @Override + public void removeChildren(QueryNode childNode){ + Iterator it = this.clauses.iterator(); + while(it.hasNext()){ + if(it.next() == childNode){ + it.remove(); + } + } + childNode.removeFromParent(); + } + @Override public void removeFromParent() { if (this.parent != null) { - List parentChildren = this.parent.getChildren(); - Iterator it = parentChildren.iterator(); - - while (it.hasNext()) { - if (it.next() == this) { - it.remove(); - } - } - + QueryNode parent = this.parent; this.parent = null; + parent.removeChildren(this); } } diff --git a/lucene/queryparser/src/test/org/apache/lucene/queryparser/flexible/core/nodes/TestQueryNode.java b/lucene/queryparser/src/test/org/apache/lucene/queryparser/flexible/core/nodes/TestQueryNode.java index 5b60b6930c3..2b9ba7904d6 100644 --- a/lucene/queryparser/src/test/org/apache/lucene/queryparser/flexible/core/nodes/TestQueryNode.java +++ b/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.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.emptyList()); assertNull(fieldNode.getParent()); } + + public void testRemoveChildren() throws Exception{ + BooleanQueryNode booleanNode = new BooleanQueryNode(Collections.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()); + } }