From d88e3f73acf0d90babefd1c05d72b0a4ce424dc4 Mon Sep 17 00:00:00 2001
From: Shay Banon <kimchy@gmail.com>
Date: Sun, 20 Jan 2013 15:01:31 +0100
Subject: [PATCH] add specific makeSafe method to make an unsafe (shared) bytes
 based value to a "safe" one

---
 .../index/fielddata/BytesValues.java          | 19 +++++++++++++++----
 .../index/fielddata/HashedBytesValues.java    | 16 ++++++++++++++++
 .../ConcreteBytesRefAtomicFieldData.java      | 13 +++++++++++++
 .../TermsStatsStringFacetCollector.java       |  8 ++++----
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/src/main/java/org/elasticsearch/index/fielddata/BytesValues.java b/src/main/java/org/elasticsearch/index/fielddata/BytesValues.java
index 9b4ade8ea61..20d1502d632 100644
--- a/src/main/java/org/elasticsearch/index/fielddata/BytesValues.java
+++ b/src/main/java/org/elasticsearch/index/fielddata/BytesValues.java
@@ -28,15 +28,20 @@ import org.elasticsearch.index.fielddata.util.StringArrayRef;
  */
 public interface BytesValues {
 
+    /**
+     * Is one of the documents in this field data values is multi valued?
+     */
+    boolean isMultiValued();
+
     /**
      * Is there a value for this doc?
      */
     boolean hasValue(int docId);
 
     /**
-     * Is one of the documents in this field data values is multi valued?
+     * Converts the provided bytes to "safe" ones from a "non" safe call made (if needed).
      */
-    boolean isMultiValued();
+    BytesRef makeSafe(BytesRef bytes);
 
     /**
      * Returns a bytes value for a docId. Note, the content of it might be shared across invocation.
@@ -147,14 +152,20 @@ public interface BytesValues {
             this.values = values;
         }
 
+        @Override
+        public boolean isMultiValued() {
+            return values.isMultiValued();
+        }
+
         @Override
         public boolean hasValue(int docId) {
             return values.hasValue(docId);
         }
 
         @Override
-        public boolean isMultiValued() {
-            return values.isMultiValued();
+        public BytesRef makeSafe(BytesRef bytes) {
+            // we need to make a copy, since we use scratch to provide it
+            return BytesRef.deepCopyOf(bytes);
         }
 
         @Override
diff --git a/src/main/java/org/elasticsearch/index/fielddata/HashedBytesValues.java b/src/main/java/org/elasticsearch/index/fielddata/HashedBytesValues.java
index 0658a4797a1..2fe6f6da61b 100644
--- a/src/main/java/org/elasticsearch/index/fielddata/HashedBytesValues.java
+++ b/src/main/java/org/elasticsearch/index/fielddata/HashedBytesValues.java
@@ -37,6 +37,11 @@ public interface HashedBytesValues {
      */
     boolean hasValue(int docId);
 
+    /**
+     * Converts the provided bytes to "safe" ones from a "non" safe call made (if needed).
+     */
+    HashedBytesRef makeSafe(HashedBytesRef bytes);
+
     /**
      * Returns a bytes value for a docId. Note, the content of it might be shared across invocation.
      */
@@ -146,6 +151,11 @@ public interface HashedBytesValues {
             return values.hasValue(docId);
         }
 
+        @Override
+        public HashedBytesRef makeSafe(HashedBytesRef bytes) {
+            return new HashedBytesRef(values.makeSafe(bytes.bytes), bytes.hash);
+        }
+
         @Override
         public HashedBytesRef getValue(int docId) {
             BytesRef value = values.getValue(docId);
@@ -289,6 +299,12 @@ public interface HashedBytesValues {
             return values.hasValue(docId);
         }
 
+        @Override
+        public HashedBytesRef makeSafe(HashedBytesRef bytes) {
+            // we use scratch to provide it, so just need to copy it over to a new instance
+            return new HashedBytesRef(bytes.bytes, bytes.hash);
+        }
+
         @Override
         public HashedBytesRef getValue(int docId) {
             String value = values.getValue(docId);
diff --git a/src/main/java/org/elasticsearch/index/fielddata/plain/ConcreteBytesRefAtomicFieldData.java b/src/main/java/org/elasticsearch/index/fielddata/plain/ConcreteBytesRefAtomicFieldData.java
index 55895305cec..ea036083744 100644
--- a/src/main/java/org/elasticsearch/index/fielddata/plain/ConcreteBytesRefAtomicFieldData.java
+++ b/src/main/java/org/elasticsearch/index/fielddata/plain/ConcreteBytesRefAtomicFieldData.java
@@ -146,6 +146,12 @@ public class ConcreteBytesRefAtomicFieldData implements AtomicOrdinalFieldData<S
             return ordinals.getOrd(docId) != 0;
         }
 
+        @Override
+        public BytesRef makeSafe(BytesRef bytes) {
+            // no need to do anything, its already concrete bytes...
+            return bytes;
+        }
+
         @Override
         public BytesRef getValue(int docId) {
             return values[ordinals.getOrd(docId)];
@@ -337,6 +343,13 @@ public class ConcreteBytesRefAtomicFieldData implements AtomicOrdinalFieldData<S
             return ordinals.getOrd(docId) != 0;
         }
 
+        @Override
+        public HashedBytesRef makeSafe(HashedBytesRef bytes) {
+            // we just need to create a copy of the bytes ref, no need
+            // to create a copy of the actual BytesRef, as its concrete
+            return new HashedBytesRef(bytes.bytes, bytes.hash);
+        }
+
         @Override
         public HashedBytesRef getValue(int docId) {
             int ord = ordinals.getOrd(docId);
diff --git a/src/main/java/org/elasticsearch/search/facet/termsstats/strings/TermsStatsStringFacetCollector.java b/src/main/java/org/elasticsearch/search/facet/termsstats/strings/TermsStatsStringFacetCollector.java
index a02b9d41dfa..e75d8c5bb3f 100644
--- a/src/main/java/org/elasticsearch/search/facet/termsstats/strings/TermsStatsStringFacetCollector.java
+++ b/src/main/java/org/elasticsearch/search/facet/termsstats/strings/TermsStatsStringFacetCollector.java
@@ -83,6 +83,7 @@ public class TermsStatsStringFacetCollector extends AbstractFacetCollector {
     @Override
     protected void doSetNextReader(AtomicReaderContext context) throws IOException {
         keyValues = keyIndexFieldData.load(context).getHashedBytesValues();
+        aggregator.keyValues = keyValues;
         if (script != null) {
             script.setNextReader(context);
         } else {
@@ -127,6 +128,7 @@ public class TermsStatsStringFacetCollector extends AbstractFacetCollector {
 
         int missing = 0;
 
+        HashedBytesValues keyValues;
         DoubleValues valueValues;
 
         ValueAggregator valueAggregator = new ValueAggregator();
@@ -135,8 +137,7 @@ public class TermsStatsStringFacetCollector extends AbstractFacetCollector {
         public void onValue(int docId, HashedBytesRef value) {
             InternalTermsStatsStringFacet.StringEntry stringEntry = entries.get(value);
             if (stringEntry == null) {
-                // we use "unsafe" hashedBytes, and only copy over if we "miss" on the map, and need to put it there
-                value = value.deepCopy();
+                value = keyValues.makeSafe(value);
                 stringEntry = new InternalTermsStatsStringFacet.StringEntry(value, 0, 0, 0, Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY);
                 entries.put(value, stringEntry);
             }
@@ -183,8 +184,7 @@ public class TermsStatsStringFacetCollector extends AbstractFacetCollector {
         public void onValue(int docId, HashedBytesRef value) {
             InternalTermsStatsStringFacet.StringEntry stringEntry = entries.get(value);
             if (stringEntry == null) {
-                // we use "unsafe" hashedBytes, and only copy over if we "miss" on the map, and need to put it there
-                value = value.deepCopy();
+                value = keyValues.makeSafe(value);
                 stringEntry = new InternalTermsStatsStringFacet.StringEntry(value, 1, 0, 0, Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY);
                 entries.put(value, stringEntry);
             } else {