Merge pull request #21744 from vespa-engine/jonmv/multi-range-item

Some minor improvements

15 files changed, 68 insertions, 77 deletions

diff --git a/container-search/abi-spec.json b/container-search/abi-spec.json
index 0e45aa59421..9b560e86d86 100644
--- a/container-search/abi-spec.json
+++ b/container-search/abi-spec.json
@@ -441,7 +441,7 @@
       "protected void encodeThis(java.nio.ByteBuffer)",
       "protected int encodingArity()",
       "protected void appendBodyString(java.lang.StringBuilder)",
-      "protected boolean shouldParenthize()",
+      "protected boolean shouldParenthesize()",
       "public com.yahoo.prelude.query.CompositeItem clone()",
       "public int hashCode()",
       "public boolean equals(java.lang.Object)",
@@ -741,6 +741,7 @@
       "public static final enum com.yahoo.prelude.query.Item$ItemType GEO_LOCATION_TERM",
       "public static final enum com.yahoo.prelude.query.Item$ItemType TRUE",
       "public static final enum com.yahoo.prelude.query.Item$ItemType FALSE",
+      "public static final enum com.yahoo.prelude.query.Item$ItemType MULTI_TERM",
       "public final int code"
     ]
   },
@@ -774,12 +775,12 @@
       "public com.yahoo.prelude.query.CompositeItem getParent()",
       "public abstract int encode(java.nio.ByteBuffer)",
       "protected void encodeThis(java.nio.ByteBuffer)",
-      "protected static final byte[] getBytes(java.lang.String)",
+      "protected static byte[] getBytes(java.lang.String)",
       "public static void putString(java.lang.String, java.nio.ByteBuffer)",
       "public static void putBytes(byte[], java.nio.ByteBuffer)",
       "public abstract int getTermCount()",
       "public java.lang.String toString()",
-      "protected boolean shouldParenthize()",
+      "protected boolean shouldParenthesize()",
       "protected void appendHeadingString(java.lang.StringBuilder)",
       "protected abstract void appendBodyString(java.lang.StringBuilder)",
       "public com.yahoo.prelude.query.Item clone()",
@@ -1027,7 +1028,7 @@
       "public com.yahoo.prelude.query.BlockItem getBlockItem(int)",
       "protected void encodeThis(java.nio.ByteBuffer)",
       "public int encode(java.nio.ByteBuffer)",
-      "protected boolean shouldParenthize()",
+      "protected boolean shouldParenthesize()",
       "protected void appendHeadingString(java.lang.StringBuilder)",
       "protected void appendBodyString(java.lang.StringBuilder)",
       "public java.lang.String getIndexedString()",
@@ -1060,7 +1061,7 @@
       "protected void encodeThis(java.nio.ByteBuffer)",
       "public int encode(java.nio.ByteBuffer)",
       "public int encodeContent(java.nio.ByteBuffer)",
-      "protected boolean shouldParenthize()",
+      "protected boolean shouldParenthesize()",
       "protected void appendHeadingString(java.lang.StringBuilder)",
       "protected void appendBodyString(java.lang.StringBuilder)",
       "public java.lang.String getIndexedString()",
diff --git a/container-search/src/main/antlr4/com/yahoo/search/yql/yqlplus.g4 b/container-search/src/main/antlr4/com/yahoo/search/yql/yqlplus.g4
index 9af8d83c6d7..422c3c8681e 100644
--- a/container-search/src/main/antlr4/com/yahoo/search/yql/yqlplus.g4
+++ b/container-search/src/main/antlr4/com/yahoo/search/yql/yqlplus.g4
@@ -127,7 +127,7 @@ HEX_DIGIT : ('0'..'9'|'a'..'f'|'A'..'F') ;
 
 fragment
 ESC_SEQ
-    :   '\\' ('b'|'t'|'n'|'f'|'r'|'\"'|'\''|'\\'|'/')
+    :   '\\' ('b'|'t'|'n'|'f'|'r'|'"'|'\''|'\\'|'/')
     |   UNICODE_ESC
     ;
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java b/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java
index d3fbeb020f8..f48dc9a8630 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java
@@ -42,13 +42,9 @@ public abstract class CompositeItem extends Item {
     }
 
     public void ensureNotInSubtree(CompositeItem item) {
-        for (Iterator<Item> i = item.getItemIterator(); i.hasNext();) {
-            Item possibleCycle = i.next();
-
-            if (this == possibleCycle) {
+        for (Item i = this; i != null; i = i.getParent()) {
+            if (i == item) {
                 throw new IllegalArgumentException("Cannot add " + item + " to " + this + " as it would create a cycle");
-            } else if (possibleCycle instanceof CompositeItem) {
-                ensureNotInSubtree((CompositeItem) possibleCycle);
             }
         }
     }
@@ -205,7 +201,7 @@ public abstract class CompositeItem extends Item {
     }
 
     /** Composite items should be parenthized when not on the top level */
-    protected boolean shouldParenthize() {
+    protected boolean shouldParenthesize() {
         return getParent()!= null && ! (getParent() instanceof QueryTree);
     }
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/EquivItem.java b/container-search/src/main/java/com/yahoo/prelude/query/EquivItem.java
index 500c68cf4bd..43258db8963 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/EquivItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/EquivItem.java
@@ -20,7 +20,7 @@ public class EquivItem extends CompositeTaggableItem {
     public EquivItem() {}
 
     /**
-     * Creates an EQUIV with the given item as child.  The new EQUIV will take connectivity,
+     * Creates an EQUIV with the given item as child. The new EQUIV will take connectivity,
      * significance and weight from the given item.
      *
      * @param item will be modified and added as a child
@@ -48,7 +48,7 @@ public class EquivItem extends CompositeTaggableItem {
         // steal other item's weight:
         setWeight(item.getWeight());
 
-        // we have now stolen all of the other item's unique id needs:
+        // we have now stolen all the other item's unique id needs:
         item.setHasUniqueID(false);
     }
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/Item.java b/container-search/src/main/java/com/yahoo/prelude/query/Item.java
index 92b321adb70..06fe07d3895 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/Item.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/Item.java
@@ -11,7 +11,6 @@ import com.yahoo.text.Utf8;
 
 import java.nio.ByteBuffer;
 import java.util.Objects;
-import java.util.Optional;
 
 
 /**
@@ -20,7 +19,7 @@ import java.util.Optional;
  * Items are in general mutable and not thread safe.
  * They can be deeply cloned by calling clone().
  * Their identity is defined by their content
- * (i.e the field value of two items decide if they are equal).
+ * (i.e. the field value of two items decide if they are equal).
  *
  * @author bratseth
  * @author havardpe
@@ -60,7 +59,8 @@ public abstract class Item implements Cloneable {
         NEAREST_NEIGHBOR(26),
         GEO_LOCATION_TERM(27),
         TRUE(28),
-        FALSE(29);
+        FALSE(29),
+        MULTI_TERM(30);
 
         public final int code;
 
@@ -233,36 +233,34 @@ public abstract class Item implements Cloneable {
     public abstract int encode(ByteBuffer buffer);
 
     protected void encodeThis(ByteBuffer buffer) {
-        int FEAT_SHIFT = 5;
-        int CODE_MASK = 0x1f;
-        int FEAT_MASK = 0xe0;
-        int FEAT_WEIGHT = 0x01;
-        int FEAT_UNIQUEID = 0x02;
-        int FEAT_FLAGS = 0x04;
+        byte CODE_MASK =     0b00011111;
+        byte FEAT_WEIGHT =   0b00100000;
+        byte FEAT_UNIQUEID = 0b01000000;
+        byte FEAT_FLAGS =   -0b10000000;
 
-        int features = 0;
+        byte type = (byte) (getCode() & CODE_MASK);
+        if (type != getCode())
+            throw new IllegalStateException("must increase number of bytes in serialization format for queries");
 
         if (weight != DEFAULT_WEIGHT) {
-            features |= FEAT_WEIGHT;
+            type |= FEAT_WEIGHT;
         }
         if (hasUniqueID()) {
-            features |= FEAT_UNIQUEID;
+            type |= FEAT_UNIQUEID;
         }
         byte flags = getFlagsFeature();
         if (flags != 0) {
-            features |= FEAT_FLAGS;
+            type |= FEAT_FLAGS;
         }
-        byte type = (byte)(((getCode() & CODE_MASK)
-                       | ((features << FEAT_SHIFT) & FEAT_MASK)) & 0xff);
 
         buffer.put(type);
-        if ((features & FEAT_WEIGHT) != 0) {
+        if ((type & FEAT_WEIGHT) != 0) {
             IntegerCompressor.putCompressedNumber(weight, buffer);
         }
-        if ((features & FEAT_UNIQUEID) != 0) {
+        if ((type & FEAT_UNIQUEID) != 0) {
             IntegerCompressor.putCompressedPositiveNumber(uniqueID, buffer);
         }
-        if (flags != 0) {
+        if ((type & FEAT_FLAGS) != 0) {
             buffer.put(flags);
         }
     }
@@ -297,7 +295,7 @@ public abstract class Item implements Cloneable {
 
 
     /** Utility method for turning a string into utf-8 bytes */
-    protected static final byte[] getBytes(String string) {
+    protected static byte[] getBytes(String string) {
         return Utf8.toBytes(string);
     }
     public static void putString(String s, ByteBuffer buffer) {
@@ -322,7 +320,7 @@ public abstract class Item implements Cloneable {
     public String toString() {
         StringBuilder buffer = new StringBuilder();
 
-        if (shouldParenthize()) {
+        if (shouldParenthesize()) {
             buffer.append("(");
         }
         if (isFilter()) {
@@ -330,7 +328,7 @@ public abstract class Item implements Cloneable {
         }
         appendHeadingString(buffer);
         appendBodyString(buffer);
-        if (shouldParenthize()) {
+        if (shouldParenthesize()) {
             buffer.append(")");
         }
 
@@ -343,10 +341,10 @@ public abstract class Item implements Cloneable {
     }
 
     /**
-     * Returns whether this item should be parethized when printed.
+     * Returns whether this item should be parenthesized when printed.
      * Default is false - no parentheses
      */
-    protected boolean shouldParenthize() {
+    protected boolean shouldParenthesize() {
         return false;
     }
 
@@ -376,7 +374,7 @@ public abstract class Item implements Cloneable {
             // note: connectedItem and connectedBacklink references are corrected in CompositeItem.clone()
             return clone;
         } catch (CloneNotSupportedException e) {
-            throw new RuntimeException("Someone made Item unclonable");
+            throw new RuntimeException("Someone made Item uncloneable");
         }
     }
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/PhraseItem.java b/container-search/src/main/java/com/yahoo/prelude/query/PhraseItem.java
index fae282868f8..379dfd6bb30 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/PhraseItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/PhraseItem.java
@@ -202,7 +202,7 @@ public class PhraseItem extends CompositeIndexedItem {
 
     /** Returns false, no parenthezes for phrases */
     @Override
-    protected boolean shouldParenthize() {
+    protected boolean shouldParenthesize() {
         return false;
     }
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java b/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java
index 16e22f6d482..2a97970ac4a 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java
@@ -148,7 +148,7 @@ public class PhraseSegmentItem extends IndexedSegmentItem {
 
     /** Returns false, no parenthezes for phrases */
     @Override
-    protected boolean shouldParenthize() {
+    protected boolean shouldParenthesize() {
         return false;
     }
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/TaggableItem.java b/container-search/src/main/java/com/yahoo/prelude/query/TaggableItem.java
index f23f2088f1d..eb23570ec90 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/TaggableItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/TaggableItem.java
@@ -18,10 +18,10 @@ public interface TaggableItem {
      * This is used to influence ranking features taking proximity into account: nativeRank and a subset of the
      * fieldMatch features.
      * <p>
-     * By default consecutive query terms are 'somewhat' connected, meaning ranking features will be better in documents
+     * By default consecutive query terms are 'somewhat' connected, meaning ranking features will score higher in documents
      * where the terms are found close to each other. This effect can be increased or decreased by manipulating the
      * connectivity value. Typical use is to increase the connectivity between terms in the query that we believe are
-     * semantically connected. E.g in the query 'new york hotel', it is a good idea to increase the connectivity between
+     * semantically connected. E.g., in the query 'new york hotel', it is a good idea to increase the connectivity between
      * "new" and "york" to ensure that a document containing "List of hotels in New York" is ranked above one containing
      * "List of new hotels in York".
      *
@@ -36,8 +36,8 @@ public interface TaggableItem {
     /**
      * Used for setting explicit term significance (in the tf/idf sense) to a single term or phrase,
      * relative to the rest of the query.
-     * This influences ranking features which take term significance into account and overrides the default
-     * partial corpus based term significance computation happening in the backend.
+     * This influences ranking features which take term significance into account, and overrides the default
+     * partial corpus based term significance computation in the backend.
      */
     void setSignificance(double significance);
     boolean hasExplicitSignificance();
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/WeightedSetItem.java b/container-search/src/main/java/com/yahoo/prelude/query/WeightedSetItem.java
index a988753d699..a5903f24cbd 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/WeightedSetItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/WeightedSetItem.java
@@ -10,6 +10,8 @@ import java.util.Iterator;
 import java.util.Map;
 import java.util.Objects;
 
+import static java.util.Objects.requireNonNullElse;
+
 /**
  * A term which contains a weighted set.
  *
@@ -30,20 +32,12 @@ public class WeightedSetItem extends SimpleTaggableItem {
 
     /** Creates an empty weighted set; note you must provide an index name up front */
     public WeightedSetItem(String indexName) {
-        if (indexName == null) {
-            this.indexName = "";
-        } else {
-            this.indexName = indexName;
-        }
+        this.indexName = requireNonNullElse(indexName, "");
         set = new CopyOnWriteHashMap<>(1000);
     }
 
     public WeightedSetItem(String indexName, Map<Object, Integer> map) {
-        if (indexName == null) {
-            this.indexName = "";
-        } else {
-            this.indexName = indexName;
-        }
+        this.indexName = requireNonNullElse(indexName, "");
         set = new CopyOnWriteHashMap<>(map);
     }
 
@@ -96,11 +90,7 @@ public class WeightedSetItem extends SimpleTaggableItem {
 
     @Override
     public void setIndexName(String index) {
-        if (index == null) {
-            this.indexName = "";
-        } else {
-            this.indexName = index;
-        }
+        this.indexName = requireNonNullElse(index, "");
     }
 
     public String getIndexName() {
diff --git a/container-search/src/test/java/com/yahoo/prelude/query/ItemsCommonStuffTestCase.java b/container-search/src/test/java/com/yahoo/prelude/query/ItemsCommonStuffTestCase.java
index ba225722fd0..418d7121769 100644
--- a/container-search/src/test/java/com/yahoo/prelude/query/ItemsCommonStuffTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/query/ItemsCommonStuffTestCase.java
@@ -212,7 +212,7 @@ public class ItemsCommonStuffTestCase {
         w.setConnectivity(v, 1.0);
         String expected = "puppy";
         String expected2 = "kvalp";
-        EquivItem e = new EquivItem(w, Arrays.asList(new String[] { expected, expected2 }));
+        EquivItem e = new EquivItem(w, Arrays.asList(expected, expected2));
         assertEquals(1.0, e.getConnectivity(), 1e-9);
         assertSame(v, e.getConnectedItem());
         assertEquals(expected, ((WordItem) e.getItem(1)).getWord());
diff --git a/searchlib/src/vespa/searchlib/parsequery/parse.h b/searchlib/src/vespa/searchlib/parsequery/parse.h
index 7db9f0e43ea..c9b7940b887 100644
--- a/searchlib/src/vespa/searchlib/parsequery/parse.h
+++ b/searchlib/src/vespa/searchlib/parsequery/parse.h
@@ -56,12 +56,12 @@ public:
         ITEM_GEO_LOCATION_TERM     =   27,
         ITEM_TRUE                  =   28,
         ITEM_FALSE                 =   29,
-        ITEM_MAX                   =   30,  // Indicates how long tables must be.
+        ITEM_MULTI_TERM            =   30,
         ITEM_UNDEF                 =   31,
     };
 
     /** A tag identifying the origin of this query node.
-     *  Note that descendants may origin from elsewhere.
+     *  Note that descendants may originate from elsewhere.
      *  If changes necessary:
      *  NB! Append at end of list - corresponding type
      *  used in Juniper and updates of these two types must be synchronized.
diff --git a/searchsummary/src/tests/extractkeywords/simplequerystackitem.cpp b/searchsummary/src/tests/extractkeywords/simplequerystackitem.cpp
index c28446cf01f..b31173c74c2 100644
--- a/searchsummary/src/tests/extractkeywords/simplequerystackitem.cpp
+++ b/searchsummary/src/tests/extractkeywords/simplequerystackitem.cpp
@@ -187,7 +187,7 @@ SimpleQueryStackItem::AppendBuffer(RawBuf *buf) const
         buf->appendCompressedPositiveNumber(_arg3); // explore_additional_hits
         break;
     case ITEM_PREDICATE_QUERY: // not handled at all here
-    case ITEM_MAX:
+    case ITEM_MULTI_TERM: // TODO: handle
     case ITEM_UNDEF:
         abort();
         break;
diff --git a/vespajlib/src/main/java/com/yahoo/collections/CopyOnWriteHashMap.java b/vespajlib/src/main/java/com/yahoo/collections/CopyOnWriteHashMap.java
index 424e850426c..218d0c407ec 100644
--- a/vespajlib/src/main/java/com/yahoo/collections/CopyOnWriteHashMap.java
+++ b/vespajlib/src/main/java/com/yahoo/collections/CopyOnWriteHashMap.java
@@ -7,6 +7,7 @@ import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
 
 /**
  * A hashmap wrapper which defers cloning of the enclosed map until it is written to.
@@ -21,8 +22,8 @@ public class CopyOnWriteHashMap<K,V> extends AbstractMap<K,V> implements Cloneab
 
     private Map<K,V> map;
 
-    /** True when this class is allowed to write to the map */
-    private boolean writable = true;
+    /** This class may write to the map if it is the sole owner */
+    private AtomicInteger owners = new AtomicInteger(1);
 
     /** Lazily initialized view */
     private transient Set<Map.Entry<K,V>> entrySet = null;
@@ -40,13 +41,18 @@ public class CopyOnWriteHashMap<K,V> extends AbstractMap<K,V> implements Cloneab
     }
 
     private void makeReadOnly() {
-        writable = false;
+        owners.incrementAndGet();
+    }
+
+    private boolean isWritable() {
+        return owners.get() == 1;
     }
 
     private void makeWritable() {
-        if (writable) return;
+        if (isWritable()) return;
         map = copyMap(map);
-        writable = true;
+        owners.decrementAndGet();
+        owners = new AtomicInteger(1);
         entrySet = null;
     }
 
@@ -62,8 +68,7 @@ public class CopyOnWriteHashMap<K,V> extends AbstractMap<K,V> implements Cloneab
     public CopyOnWriteHashMap<K,V> clone() {
         try {
             CopyOnWriteHashMap<K,V> clone = (CopyOnWriteHashMap<K,V>)super.clone();
-            this.makeReadOnly();
-            clone.makeReadOnly();
+            makeReadOnly(); // owners shared with clone
             return clone;
         }
         catch (CloneNotSupportedException e) {
@@ -94,7 +99,7 @@ public class CopyOnWriteHashMap<K,V> extends AbstractMap<K,V> implements Cloneab
     @Override
     public boolean equals(Object other) {
         if ( ! (other instanceof CopyOnWriteHashMap)) return false;
-        return this.map.equals(((CopyOnWriteHashMap)other).map);
+        return this.map.equals(((CopyOnWriteHashMap<?, ?>)other).map);
     }
 
     @Override
@@ -137,7 +142,7 @@ public class CopyOnWriteHashMap<K,V> extends AbstractMap<K,V> implements Cloneab
     private class EntryIterator implements Iterator<Map.Entry<K,V>> {
 
         /** Wrapped iterator */
-        private Iterator<Map.Entry<K,V>> mapIterator;
+        private final Iterator<Map.Entry<K,V>> mapIterator;
 
         public EntryIterator() {
             mapIterator = map.entrySet().iterator();
@@ -152,7 +157,7 @@ public class CopyOnWriteHashMap<K,V> extends AbstractMap<K,V> implements Cloneab
         }
 
         public void remove() {
-            if ( ! writable)
+            if ( ! isWritable())
                 throw new UnsupportedOperationException("Cannot perform the copy-on-write operation during iteration");
             mapIterator.remove();
         }
diff --git a/vespajlib/src/main/java/com/yahoo/text/Utf8.java b/vespajlib/src/main/java/com/yahoo/text/Utf8.java
index 91bbb86d3fc..b2b6fa13b70 100644
--- a/vespajlib/src/main/java/com/yahoo/text/Utf8.java
+++ b/vespajlib/src/main/java/com/yahoo/text/Utf8.java
@@ -111,7 +111,7 @@ public final class Utf8 {
      * @return Utf8 encoded array
      */
     public static byte[] toBytes(String string) {
-        // This is just wrapper for String::getBytes. Pre-Java 9 this had an more efficient approach for ASCII-only strings.
+        // This is just wrapper for String::getBytes. Pre-Java 9 this had a more efficient approach for ASCII-only strings.
         return string.getBytes(StandardCharsets.UTF_8);
     }
     /**
@@ -121,7 +121,7 @@ public final class Utf8 {
      * @return Utf8 encoded array
      */
     public static String toString(byte [] utf8) {
-        // This is just wrapper for String::new. Pre-Java 9 this had an more efficient approach for ASCII-onlu strings.
+        // This is just wrapper for String::new. Pre-Java 9 this had a more efficient approach for ASCII-onlu strings.
         return new String(utf8, StandardCharsets.UTF_8);
     }
 
diff --git a/vespajlib/src/test/java/com/yahoo/collections/CopyOnWriteHashMapTestCase.java b/vespajlib/src/test/java/com/yahoo/collections/CopyOnWriteHashMapTestCase.java
index 2072247bf96..35401a0cb19 100644
--- a/vespajlib/src/test/java/com/yahoo/collections/CopyOnWriteHashMapTestCase.java
+++ b/vespajlib/src/test/java/com/yahoo/collections/CopyOnWriteHashMapTestCase.java
@@ -2,7 +2,8 @@
 package com.yahoo.collections;
 
 import org.junit.Test;
-import static org.junit.Assert.*;
+
+import static org.junit.Assert.assertEquals;
 
 /**
  * @author bratseth