Simplify more

5 files changed, 43 insertions, 44 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/query/NullItem.java b/container-search/src/main/java/com/yahoo/prelude/query/NullItem.java
index aa3a04d670f..d04a8753f65 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/NullItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/NullItem.java
@@ -8,7 +8,7 @@ import java.nio.ByteBuffer;
 /**
  * A place holder for null queries to make searchers easier to write.
  *
- * @author  <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a>
+ * @author Steinar Knutsen
  */
 public class NullItem extends Item {
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java b/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java
index 8f61807d2dd..e770e6e6b61 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java
@@ -32,9 +32,9 @@ public class QueryCanonicalizer {
      * @return null if the query is valid, an error message if it is invalid
      */
     public static String canonicalize(QueryTree query) {
-        CanonicalizationResult result = treeCanonicalize(query.getRoot(), null);
-        if (result.newRoot().isPresent())
-            query.setRoot(result.newRoot().get());
+        ListIterator<Item> rootItemIterator = query.getItemIterator();
+        CanonicalizationResult result = recursivelyCanonicalize(rootItemIterator.next(), rootItemIterator);
+        if (query.isEmpty() && ! result.isError()) return "No query";
         return result.error().orElse(null);
     }
 
@@ -42,13 +42,21 @@ public class QueryCanonicalizer {
      * Canonicalize this query
      * 
      * @param item the item to canonicalize
-     * @param parentIterator iterator for the parent of this item, or null if there is no parent
+     * @param parentIterator iterator for the parent of this item, never null
      * @return true if the given query is valid, false otherwise
      */
-    public static CanonicalizationResult treeCanonicalize(Item item, ListIterator<Item> parentIterator) {
-        if (parentIterator == null && (item == null || item instanceof NullItem)) 
-            return CanonicalizationResult.error("No query");
-
+    private static CanonicalizationResult recursivelyCanonicalize(Item item, ListIterator<Item> parentIterator) {
+        if (item instanceof CompositeItem) { // children first as they may be removed
+            CompositeItem composite = (CompositeItem)item;
+            for (ListIterator<Item> i = composite.getItemIterator(); i.hasNext(); ) {
+                CanonicalizationResult childResult = recursivelyCanonicalize(i.next(), i);
+                if (childResult.isError()) return childResult;
+            }
+        }
+        return canonicalizeThis(item, parentIterator);
+    }
+    
+    private static CanonicalizationResult canonicalizeThis(Item item, ListIterator<Item> parentIterator) {
         if (item instanceof TermItem) return CanonicalizationResult.success();
 
         if (item instanceof NullItem) parentIterator.remove();
@@ -56,11 +64,12 @@ public class QueryCanonicalizer {
         if ( ! (item instanceof CompositeItem)) return CanonicalizationResult.success();
 
         CompositeItem composite = (CompositeItem)item;
-        for (ListIterator<Item> i = composite.getItemIterator(); i.hasNext();) {
-            CanonicalizationResult childResult = treeCanonicalize(i.next(), i);
-            if (childResult.isError()) return childResult;
+        if (composite.getItemCount() == 0) {
+            System.out.println(item + " is empty, removing it");
+            parentIterator.remove();
+            return CanonicalizationResult.success();
         }
-
+            
         if (composite instanceof EquivItem) {
             removeDuplicates((EquivItem) composite);
         }
@@ -72,22 +81,13 @@ public class QueryCanonicalizer {
                 return CanonicalizationResult.error("Can not search for only negative items");
         }
 
-        if (composite.getItemCount() == 0) {
-            if (parentIterator == null)
-                return CanonicalizationResult.error("No query: Contained an empty " + composite.getName() + " only");
-            else
-                parentIterator.remove();
-        }
+        if (composite.getItemCount() == 0)
+            parentIterator.remove();
 
         if (composite.getItemCount() == 1 && ! (composite instanceof NonReducibleCompositeItem)) {
-            if (composite instanceof PhraseItem || composite instanceof PhraseSegmentItem) {
+            if (composite instanceof PhraseItem || composite instanceof PhraseSegmentItem)
                 composite.getItem(0).setWeight(composite.getWeight());
-            }
-            if (parentIterator == null) {
-                return CanonicalizationResult.successWithRoot(composite.getItem(0));
-            } else {
-                parentIterator.set(composite.getItem(0));
-            }
+            parentIterator.set(composite.getItem(0));
         }
 
         return CanonicalizationResult.success();
diff --git a/container-search/src/main/java/com/yahoo/search/federation/vespa/VespaSearcher.java b/container-search/src/main/java/com/yahoo/search/federation/vespa/VespaSearcher.java
index 65864468b85..bec714e4882 100644
--- a/container-search/src/main/java/com/yahoo/search/federation/vespa/VespaSearcher.java
+++ b/container-search/src/main/java/com/yahoo/search/federation/vespa/VespaSearcher.java
@@ -93,8 +93,7 @@ public class VespaSearcher extends ConfiguredHTTPProviderSearcher {
         segmenterVersion = null;
     }
 
-    void addProperty(Map<String, String> queryMap, Query query,
-            CompoundName property) {
+    void addProperty(Map<String, String> queryMap, Query query, CompoundName property) {
         Object o = query.properties().get(property);
         if (o != null) {
             queryMap.put(property.toString(), o.toString());
@@ -173,10 +172,10 @@ public class VespaSearcher extends ConfiguredHTTPProviderSearcher {
 
     public String marshalQuery(QueryTree root) {
         QueryTree rootClone = root.clone(); // TODO: Why?
-        QueryCanonicalizer.CanonicalizationResult result = QueryCanonicalizer.treeCanonicalize(rootClone, null);
-        if (result.isError()) return null;
+        String error = QueryCanonicalizer.canonicalize(rootClone);
+        if (error != null) return null;
 
-        return marshalRoot(result.newRoot().orElse(rootClone.getRoot()));
+        return marshalRoot(rootClone.getRoot());
     }
 
     private String marshalRoot(Item root) {
diff --git a/container-search/src/main/java/com/yahoo/search/query/QueryTree.java b/container-search/src/main/java/com/yahoo/search/query/QueryTree.java
index 621b972dd2b..ba7f9ed6f59 100644
--- a/container-search/src/main/java/com/yahoo/search/query/QueryTree.java
+++ b/container-search/src/main/java/com/yahoo/search/query/QueryTree.java
@@ -36,8 +36,8 @@ public class QueryTree extends CompositeItem {
     }
 
     public ItemType getItemType() {
-        throw new RuntimeException("Packet type access attempted. " +
-                "A query tree has no packet code. This is probably a misbehaving searcher.");
+        throw new RuntimeException("Packet type access attempted. A query tree has no packet code. " + 
+                                   "This is probably a misbehaving searcher.");
     }
 
     public String getName() { return "ROOT"; }
@@ -53,15 +53,15 @@ public class QueryTree extends CompositeItem {
 
     /** Returns the query root. This is null if this is a null query. */
     public Item getRoot() {
-        if (getItemCount()==0) return null;
+        if (getItemCount() == 0) return null;
         return getItem(0);
     }
 
     public final void setRoot(Item root) {
-        if (root==this) throw new IllegalArgumentException("Cannot make a root point at itself");
+        if (root == this) throw new IllegalArgumentException("Cannot make a root point at itself");
         if (root == null) throw new IllegalArgumentException("Root must not be null, use NullItem instead.");
         if (root instanceof QueryTree) throw new IllegalArgumentException("Do not use a new QueryTree instance as a root.");
-        if (this.getItemCount()==0) // initializing
+        if (this.getItemCount() == 0) // initializing
             super.addItem(root);
         else
             setItem(0,root); // replacing
@@ -103,7 +103,7 @@ public class QueryTree extends CompositeItem {
 
     /** Returns true if this represents the null query */
     public boolean isEmpty() {
-        return getRoot() instanceof NullItem;
+        return getRoot() instanceof NullItem || getItemCount() == 0;
     }
 
     // -------------- Facade
diff --git a/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java b/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java
index 9b2f05d8804..7fc98ac993a 100644
--- a/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java
@@ -69,7 +69,7 @@ public class QueryCanonicalizerTestCase {
 
         root.addItem(and1);
         and1.addItem(and2);
-        assertCanonicalized("NULL", "No query", new Query());
+        assertCanonicalized(null, "No query", new Query());
     }
 
     @Test
@@ -86,7 +86,7 @@ public class QueryCanonicalizerTestCase {
         and1.addItem(and22);
         and22.addItem(and31);
         and22.addItem(and32);
-        assertCanonicalized("NULL", "No query", new Query());
+        assertCanonicalized(null, "No query", new Query());
     }
 
     @Test
@@ -109,7 +109,7 @@ public class QueryCanonicalizerTestCase {
 
     @Test
     public void testNullRoot() {
-        assertCanonicalized("NULL", "No query", new Query());
+        assertCanonicalized(null, "No query", new Query());
     }
 
     @Test
@@ -124,7 +124,7 @@ public class QueryCanonicalizerTestCase {
 
         query.getModel().getQueryTree().setRoot(root);
 
-        assertCanonicalized("NULL", "No query: Contained an empty AND only", root);
+        assertCanonicalized(null, "No query", root);
     }
 
     @Test
@@ -141,7 +141,7 @@ public class QueryCanonicalizerTestCase {
 
         query.getModel().getQueryTree().setRoot(root);
 
-        assertCanonicalized("NULL", "No query: Contained an empty AND only", root);
+        assertCanonicalized(null, "No query", root);
     }
 
     @Test
@@ -205,7 +205,7 @@ public class QueryCanonicalizerTestCase {
         NotItem root = new NotItem();
 
         root.addNegativeItem(new WordItem("negative"));
-        assertCanonicalized("NULL","Can not search for only negative items", root);
+        assertCanonicalized("+(null) -negative","Can not search for only negative items", root);
     }
 
     @Test
@@ -216,7 +216,7 @@ public class QueryCanonicalizerTestCase {
         root.addItem(not);
         root.addItem(new WordItem("word"));
         not.addNegativeItem(new WordItem("negative"));
-        assertCanonicalized("NULL","Can not search for only negative items", root);
+        assertCanonicalized("AND (+(null) -negative) word","Can not search for only negative items", root);
     }
 
     /**