From 37edfe060b983964af88ab490850fffeefe4c69c Mon Sep 17 00:00:00 2001
From: Simon Willnauer <simonw@apache.org>
Date: Mon, 15 Jul 2013 15:13:56 +0200
Subject: [PATCH] Set spare becore comparing comparator bottom value

The actual documents value was never calculated if setSpare wasn't called
before compareBottom was called on a certain document.

Closes #3309
---
 .../StringScriptDataComparator.java           |  7 ++--
 .../search/sort/SimpleSortTests.java          | 33 +++++++++++++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/StringScriptDataComparator.java b/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/StringScriptDataComparator.java
index 8ed32896e9a..07edb8b8cd8 100644
--- a/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/StringScriptDataComparator.java
+++ b/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/StringScriptDataComparator.java
@@ -59,11 +59,11 @@ public class StringScriptDataComparator extends FieldComparator<BytesRef> {
 
     private final SearchScript script;
 
-    private BytesRef[] values;
+    private BytesRef[] values; // TODO maybe we can preallocate or use a sentinel to prevent the conditionals in compare
 
     private BytesRef bottom;
     
-    private BytesRef spare = new BytesRef();
+    private final BytesRef spare = new BytesRef();
     
     private int spareDoc = -1;
 
@@ -102,10 +102,10 @@ public class StringScriptDataComparator extends FieldComparator<BytesRef> {
 
     @Override
     public int compareBottom(int doc) {
-      
         if (bottom == null) {
             return -1;
         }
+        setSpare(doc);
         return bottom.compareTo(spare);
     }
 
@@ -120,7 +120,6 @@ public class StringScriptDataComparator extends FieldComparator<BytesRef> {
         if (spareDoc == doc) {
             return;
         }
-        
         script.setNextDocId(doc);
         spare.copyChars(script.run().toString());
         spareDoc = doc;
diff --git a/src/test/java/org/elasticsearch/test/integration/search/sort/SimpleSortTests.java b/src/test/java/org/elasticsearch/test/integration/search/sort/SimpleSortTests.java
index 099a6e7270c..bc0ecf6388c 100644
--- a/src/test/java/org/elasticsearch/test/integration/search/sort/SimpleSortTests.java
+++ b/src/test/java/org/elasticsearch/test/integration/search/sort/SimpleSortTests.java
@@ -35,6 +35,8 @@ import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 
+import org.elasticsearch.search.sort.ScriptSortBuilder;
+
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -335,6 +337,37 @@ public class SimpleSortTests extends AbstractSharedClusterTest {
         }
 
         assertThat(searchResponse.toString(), not(containsString("error")));
+        
+        
+        // STRING script
+        size = 1 + random.nextInt(10);
+
+        searchResponse = client().prepareSearch()
+                .setQuery(matchAllQuery())
+                .setSize(size)
+                .addSort(new ScriptSortBuilder("doc['str_value'].value", "string"))
+                .execute().actionGet();
+        assertThat(searchResponse.getHits().getTotalHits(), equalTo(10l));
+        assertThat(searchResponse.getHits().hits().length, equalTo(size));
+        for (int i = 0; i < size; i++) {
+            assertThat(searchResponse.getHits().getAt(i).id(), equalTo(Integer.toString(i)));
+            assertThat(searchResponse.getHits().getAt(i).sortValues()[0].toString(), equalTo(new String(new char[]{(char) (97 + i), (char) (97 + i)})));
+        }
+        size = 1 + random.nextInt(10);
+        searchResponse = client().prepareSearch()
+                .setQuery(matchAllQuery())
+                .setSize(size)
+                .addSort("str_value", SortOrder.DESC)
+                .execute().actionGet();
+
+        assertThat(searchResponse.getHits().getTotalHits(), equalTo(10l));
+        assertThat(searchResponse.getHits().hits().length, equalTo(size));
+        for (int i = 0; i < size; i++) {
+            assertThat(searchResponse.getHits().getAt(i).id(), equalTo(Integer.toString(9 - i)));
+            assertThat(searchResponse.getHits().getAt(i).sortValues()[0].toString(), equalTo(new String(new char[]{(char) (97 + (9 - i)), (char) (97 + (9 - i))})));
+        }
+
+        assertThat(searchResponse.toString(), not(containsString("error")));
 
         // BYTE
         size = 1 + random.nextInt(10);