fix handling of empty routing values

crate · Feb 2, 2015 · a931e9c · a931e9c
1 parent f8ffa7d
commit a931e9c
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 6 deletions.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -5,6 +5,9 @@ Changes for Crate
 Unreleased
 ==========
 
+ - Fix: Filtering on a routing column sometimes didn't work if the value was an
+   empty string
+
  - Fix: Bulk inserts with mixed but compatible types (e.g. int and long) failed
 
  - Fix: Force UTF8 encoding in file reading collector to avoid JVM's default encoding settings

diff --git a/sql/src/main/java/io/crate/analyze/PrimaryKeyVisitor.java b/sql/src/main/java/io/crate/analyze/PrimaryKeyVisitor.java
@@ -26,12 +26,16 @@
 import com.google.common.collect.Sets;
 import io.crate.metadata.ColumnIdent;
 import io.crate.metadata.table.TableInfo;
-import io.crate.operation.operator.*;
+import io.crate.operation.operator.AndOperator;
+import io.crate.operation.operator.EqOperator;
+import io.crate.operation.operator.InOperator;
+import io.crate.operation.operator.OrOperator;
 import io.crate.planner.symbol.*;
 import io.crate.types.DataType;
 import io.crate.types.DataTypes;
 import io.crate.types.LongType;
 import io.crate.types.SetType;
+import org.apache.lucene.util.BytesRef;
 
 import javax.annotation.Nullable;
 import java.util.*;
@@ -62,6 +66,7 @@ public class PrimaryKeyVisitor extends SymbolVisitor<PrimaryKeyVisitor.Context,
     private static final ImmutableSet<String> PK_COMPARISONS = ImmutableSet.of(
             EqOperator.NAME, InOperator.NAME
     );
+    private static final BytesRef EMPTY_BYTES_REF = new BytesRef("");
 
 
     public static class Context {
@@ -89,7 +94,6 @@ public Context(TableInfo tableInfo) {
          * because primary key literals can differ in type.
          * We must return a nested list of primitive literals in such cases.
          *
-         * @return
          */
         @Nullable
         public List keyLiterals() {
@@ -313,7 +317,17 @@ private Set generateIntersection(Literal setLiteral, Literal right) {
     }
 
     private void setClusterBy(Context context, Literal right) {
-        if (context.currentBucket.clusteredBy == null) {
+        Object value = right.value();
+
+        /**
+         * Don't set the clusteredBy value for empty strings:
+         *      IndexRequest sets the routingValue to null if the value is an empty string;
+         *      If the clusteredBy value would be set here the routing value would be calculated using the hash("")
+         *      which is different from the value used in the index operation.
+         */
+        if (value != null && value instanceof BytesRef && EMPTY_BYTES_REF.equals(value)) {
+            invalidate(context);
+        } else if (context.currentBucket.clusteredBy == null) {
             context.currentBucket.clusteredBy = right;
         } else if (!context.currentBucket.clusteredBy.equals(right)) {
             invalidate(context);

diff --git a/sql/src/test/java/io/crate/analyze/where/WhereClauseAnalyzerTest.java b/sql/src/test/java/io/crate/analyze/where/WhereClauseAnalyzerTest.java
@@ -233,9 +233,8 @@ public void testClusteredByValueContainsComma() throws Exception {
     @Test
     public void testEmptyClusteredByValue() throws Exception {
         WhereClauseContext ctx = analyzeSelectWhere("select * from bystring where name = ''");
-        assertThat(ctx.whereClause().clusteredBy().get(), is(""));
-        assertThat(ctx.ids().size(), is(1));
-        assertThat(ctx.ids().get(0), is(""));
+        assertThat(ctx.whereClause().clusteredBy().isPresent(), is(false));
+        assertThat(ctx.ids().size(), is(0));
     }
 
     @Test

diff --git a/sql/src/test/java/io/crate/integrationtests/WherePKIntegrationTest.java b/sql/src/test/java/io/crate/integrationtests/WherePKIntegrationTest.java
@@ -168,6 +168,38 @@ public void testEmptyClusteredByExplicit() throws Exception {
         assertThat(TestingHelpers.printedTable(response.rows()), is("[36.567, 52.998]| ,\n"));
     }
 
+    @Test
+    public void testQueryOnEmptyClusteredByColumn() throws Exception {
+        execute("create table expl_routing (id int primary key, name string primary key) " +
+                "clustered by (name) with (number_of_replicas = 0)");
+        ensureGreen();
+
+        if (randomInt(1) == 0) {
+            execute("insert into expl_routing (id, name) values (?, ?)", new Object[][]{
+                    new Object[]{1, ""},
+                    new Object[]{2, ""},
+                    new Object[]{1, "1"}
+            });
+        } else {
+            execute("insert into expl_routing (id, name) values (?, ?)", new Object[]{1, ""});
+            execute("insert into expl_routing (id, name) values (?, ?)", new Object[]{2, ""});
+            execute("insert into expl_routing (id, name) values (?, ?)", new Object[]{1, "1"});
+        }
+        execute("refresh table expl_routing");
+
+        execute("select count(*) from expl_routing where name = ''");
+        assertThat((Long)response.rows()[0][0], is(2L));
+
+        execute("select * from expl_routing where name = '' order by id");
+        assertThat(response.rowCount(), is(2L));
+        assertThat((Integer) response.rows()[0][0], is(1));
+        assertThat((Integer)response.rows()[1][0], is(2));
+
+        execute("delete from expl_routing where name = ''");
+        execute("select count(*) from expl_routing");
+        assertThat((Long) response.rows()[0][0], is(1L));
+    }
+
     @Test
     public void testDeleteByQueryCommaRouting() throws Exception {
         execute("create table explicit_routing (" +