Fix query by eq with range index in page only return first page data (#614)

Fix #613

Change-Id: I3cfd3d7ff4ce1ca13b9b6f695ba5ba88569f9266

hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/Query.java

@@ -141,6 +141,11 @@ public class Query implements Cloneable {
     }
 
     public long limit() {
+        if (this.capacity != NO_CAPACITY) {
+            E.checkArgument(this.limit == Query.NO_LIMIT ||
+                            this.limit <= this.capacity,
+                            "Invalid limit %s, must be <= capacity", this.limit);
+        }
         return this.limit;
     }
 
@@ -150,10 +155,11 @@ public class Query implements Cloneable {
     }
 
     public boolean reachLimit(long count) {
-        if (this.limit == NO_LIMIT) {
+        long limit = this.limit();
+        if (limit == NO_LIMIT) {
             return false;
         }
-        return count >= (this.offset + this.limit);
+        return count >= (limit + this.offset());
     }
 
     /**
@@ -186,13 +192,11 @@ public class Query implements Cloneable {
 
     public String page() {
         if (this.page != null) {
-            E.checkState(this.limit != Query.NO_LIMIT,
-                         "Must set limit when using paging");
-            E.checkState(this.limit != 0L,
+            E.checkState(this.limit() != 0L,
                          "Can't set limit=0 when using paging");
-            E.checkState(this.offset == 0L,
+            E.checkState(this.offset() == 0L,
                          "Can't set offset when using paging, but got '%s'",
-                         this.offset);
+                         this.offset());
         }
         return this.page;
     }

hugegraph-core/src/main/java/com/baidu/hugegraph/backend/serializer/BinarySerializer.java

@@ -639,7 +639,7 @@ public class BinarySerializer extends AbstractSerializer {
                         "INDEX_LABEL_ID and FIELD_VALUES" +
                         "in secondary index query");
 
-        Id index = (Id) query.condition(HugeKeys.INDEX_LABEL_ID);
+        Id index = query.condition(HugeKeys.INDEX_LABEL_ID);
         Object key = query.condition(HugeKeys.FIELD_VALUES);
 
         E.checkArgument(index != null, "Please specify the index label");
@@ -665,7 +665,7 @@ public class BinarySerializer extends AbstractSerializer {
     }
 
     private Query writeRangeIndexQuery(ConditionQuery query) {
-        Id index = (Id) query.condition(HugeKeys.INDEX_LABEL_ID);
+        Id index = query.condition(HugeKeys.INDEX_LABEL_ID);
         E.checkArgument(index != null,
                         "Please specify the index label");
 
@@ -702,9 +702,20 @@ public class BinarySerializer extends AbstractSerializer {
         }
 
         HugeType type = query.resultType();
+        Id start = null;
+        if (query.paging() && !query.page().isEmpty()) {
+            byte[] position = PageState.fromString(query.page()).position();
+            start = new BinaryId(position, null);
+        }
+
         if (keyEq != null) {
             Id id = formatIndexId(type, index, keyEq);
-            return new IdPrefixQuery(query, id);
+            if (start == null) {
+                return new IdPrefixQuery(query, id);
+            }
+            E.checkArgument(Bytes.compare(start.asBytes(), id.asBytes()) >= 0,
+                            "Invalid page out of lower bound");
+            return new IdPrefixQuery(query, start, id);
         }
 
         if (keyMin == null) {
@@ -725,12 +736,11 @@ public class BinarySerializer extends AbstractSerializer {
             keyMinEq = true;
         }
 
-        Id start = min;
-        if (query.paging() && !query.page().isEmpty()) {
-            byte[] position = PageState.fromString(query.page()).position();
-            E.checkArgument(Bytes.compare(position, start.asBytes()) >= 0,
+        if (start == null) {
+            start = min;
+        } else {
+            E.checkArgument(Bytes.compare(start.asBytes(), min.asBytes()) >= 0,
                             "Invalid page out of lower bound");
-            start = new BinaryId(position, null);
         }
 
         Query newQuery;

hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTable.java

@@ -535,10 +535,11 @@ public abstract class MysqlTable
 
         select.append(this.orderByKeys());
 
-        assert query.limit() != Query.NO_LIMIT;
-        // Fetch `limit + 1` records for judging whether reached the last page
-        select.append(" limit ");
-        select.append(query.limit() + 1);
+        if (query.limit() != Query.NO_LIMIT) {
+            // Fetch `limit + 1` rows for judging whether reached the last page
+            select.append(" limit ");
+            select.append(query.limit() + 1);
+        }
         select.append(";");
     }
 

hugegraph-test/src/main/java/com/baidu/hugegraph/core/EdgeCoreTest.java

@@ -2875,23 +2875,15 @@ public class EdgeCoreTest extends BaseCoreTest {
 
         HugeGraph graph = graph();
         init100LookEdges();
+        GraphTraversalSource g = graph.traversal();
+        long bigLimit = Query.defaultCapacity() + 1L;
 
         Assert.assertThrows(IllegalStateException.class, () -> {
-            graph.traversal().E()
-                 .has("~page", "").limit(0)
-                 .toList();
+            g.E().has("~page", "").limit(0).toList();
         });
 
-        Assert.assertThrows(IllegalStateException.class, () -> {
-            graph.traversal().E()
-                 .has("~page", "").limit(-1)
-                 .toList();
-        });
-
-        Assert.assertThrows(IllegalStateException.class, () -> {
-            graph.traversal().E()
-                 .has("~page", "")
-                 .toList();
+        Assert.assertThrows(IllegalArgumentException.class, () -> {
+            g.E().has("~page", "").limit(bigLimit).toList();
         });
     }
 

hugegraph-test/src/main/java/com/baidu/hugegraph/core/VertexCoreTest.java

@@ -53,6 +53,7 @@ import com.baidu.hugegraph.backend.query.Query;
 import com.baidu.hugegraph.backend.store.BackendFeatures;
 import com.baidu.hugegraph.backend.store.Shard;
 import com.baidu.hugegraph.backend.tx.GraphTransaction;
+import com.baidu.hugegraph.exception.LimitExceedException;
 import com.baidu.hugegraph.exception.NoIndexException;
 import com.baidu.hugegraph.schema.PropertyKey;
 import com.baidu.hugegraph.schema.SchemaManager;
@@ -65,6 +66,7 @@ import com.baidu.hugegraph.traversal.optimize.Text;
 import com.baidu.hugegraph.traversal.optimize.TraversalUtil;
 import com.baidu.hugegraph.type.HugeType;
 import com.baidu.hugegraph.type.define.HugeKeys;
+import com.baidu.hugegraph.util.CollectionUtil;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 
@@ -3821,27 +3823,83 @@ public class VertexCoreTest extends BaseCoreTest {
                           storeFeatures().supportsQueryByPage());
 
         HugeGraph graph = graph();
-        init100Books();
+        initPageTestData();
+        GraphTraversalSource g = graph.traversal();
+        long limit = Query.defaultCapacity() + 1;
 
         Assert.assertThrows(IllegalStateException.class, () -> {
-            graph.traversal().V()
-                 .has("~page", "").limit(0)
-                 .toList();
+            g.V().has("~page", "").limit(0).toList();
         });
 
-        Assert.assertThrows(IllegalStateException.class, () -> {
-            graph.traversal().V()
-                 .has("~page", "").limit(-1)
-                 .toList();
+        Assert.assertThrows(IllegalArgumentException.class, () -> {
+            g.V().has("~page", "").limit(limit).toList();
         });
 
-        Assert.assertThrows(IllegalStateException.class, () -> {
-            graph.traversal().V()
-                 .has("~page", "")
-                 .toList();
+        Assert.assertThrows(IllegalArgumentException.class, () -> {
+            g.V().has("name", "marko").has("~page", "").limit(limit).toList();
         });
     }
 
+    @Test
+    public void testQueryByPageWithCapacityAndNoLimit() {
+        Assume.assumeTrue("Not support paging",
+                          storeFeatures().supportsQueryByPage());
+
+        HugeGraph graph = graph();
+        initPageTestData();
+        GraphTraversalSource g = graph.traversal();
+
+        long capacity = 10;
+        long old = Query.defaultCapacity(capacity);
+        try {
+            GraphTraversal<Vertex, Vertex> iter;
+            iter = g.V().has("~page", "").limit(capacity);
+            Assert.assertEquals(10, IteratorUtils.count(iter));
+
+            Assert.assertThrows(IllegalArgumentException.class, () -> {
+                /*
+                 * When query vertices/edge in page, the limit will be regard
+                 * as page size, it shoudn't exceed capacity
+                 */
+                g.V().has("~page", "").limit(capacity + 1).toList();
+            });
+
+            Assert.assertThrows(LimitExceedException.class, () -> {
+                g.V().has("~page", "").limit(-1).toList();
+            });
+        } finally {
+            Query.defaultCapacity(old);
+        }
+    }
+
+    @Test
+    public void testQueryInPageWithoutCapacity() {
+        Assume.assumeTrue("Not support paging",
+                          storeFeatures().supportsQueryByPage());
+
+        HugeGraph graph = graph();
+        GraphTraversalSource g = graph.traversal();
+        initPageTestData();
+
+        long old = Query.defaultCapacity(Query.NO_CAPACITY);
+        try {
+            GraphTraversal<Vertex, Vertex> iter;
+            iter = g.V().has("~page", "").limit(-1);
+            Assert.assertEquals(34, IteratorUtils.count(iter));
+
+            iter = g.V().has("~page", "").limit(20);
+            Assert.assertEquals(20, IteratorUtils.count(iter));
+
+            iter = g.V().has("age", 30).has("~page", "").limit(-1);
+            Assert.assertEquals(18, IteratorUtils.count(iter));
+
+            iter = g.V().has("age", 30).has("~page", "").limit(10);
+            Assert.assertEquals(10, IteratorUtils.count(iter));
+        } finally {
+            Query.defaultCapacity(old);
+        }
+    }
+
     @Test
     public void testQueryByPageWithOffset() {
         Assume.assumeTrue("Not support paging",
@@ -4296,6 +4354,63 @@ public class VertexCoreTest extends BaseCoreTest {
         });
     }
 
+    @Test
+    public void testQueryByRangeIndexInPage() {
+        Assume.assumeTrue("Not support paging",
+                          storeFeatures().supportsQueryByPage());
+
+        HugeGraph graph = graph();
+        GraphTraversalSource g = graph.traversal();
+        initPageTestData();
+
+        // There are 4 vertices matched
+        GraphTraversal<Vertex, Vertex> iter = g.V().hasLabel("software")
+                                               .has("price", P.eq(100))
+                                               .has("~page", "")
+                                               .limit(3);
+
+        List<Vertex> vertices1 = IteratorUtils.list(iter);
+        String page = TraversalUtil.page(iter);
+        Assert.assertEquals(3, vertices1.size());
+        List<Vertex> vertices2 = g.V().hasLabel("software")
+                                  .has("price", P.eq(100))
+                                  .has("~page", page).limit(3)
+                                  .toList();
+        Assert.assertEquals(1, vertices2.size());
+        Assert.assertTrue(CollectionUtil.intersect(vertices1, vertices2)
+                                        .isEmpty());
+
+        // There are 8 vertices matched
+        iter = g.V().hasLabel("software")
+                .has("price", P.gt(200))
+                .has("~page", "")
+                .limit(5);
+
+        vertices1 = IteratorUtils.list(iter);
+        Assert.assertEquals(5, vertices1.size());
+        page = TraversalUtil.page(iter);
+        vertices2 = g.V().hasLabel("software").has("price", P.gt(200))
+                     .has("~page", page).limit(5).toList();
+        Assert.assertEquals(3, vertices2.size());
+        Assert.assertTrue(CollectionUtil.intersect(vertices1, vertices2)
+                                        .isEmpty());
+
+        // There are 8 vertices matched
+        iter = g.V().hasLabel("software")
+                .has("price", P.lt(300))
+                .has("~page", "")
+                .limit(5);
+
+        vertices1 = IteratorUtils.list(iter);
+        Assert.assertEquals(5, vertices1.size());
+        page = TraversalUtil.page(iter);
+        vertices2 = g.V().hasLabel("software").has("price", P.lt(300))
+                     .has("~page", page).limit(5).toList();
+        Assert.assertEquals(3, vertices2.size());
+        Assert.assertTrue(CollectionUtil.intersect(vertices1, vertices2)
+                                        .isEmpty());
+    }
+
     @Test
     public void testQueryByUnionIndexInPageWithSomeIndexNoData() {
         Assume.assumeTrue("Not support paging",
@@ -4695,6 +4810,13 @@ public class VertexCoreTest extends BaseCoreTest {
               .ifNotExist()
               .create();
 
+        schema.indexLabel("programmerByAge")
+              .onV("programmer")
+              .range()
+              .by("age")
+              .ifNotExist()
+              .create();
+
         schema.indexLabel("programmerByCity")
               .onV("programmer")
               .search()

hugegraph-test/src/main/resources/hugegraph.properties

@@ -11,7 +11,7 @@ edge.tx_capacity=10000
 vertex.cache_expire=300
 edge.cache_expire=300
 
-query.page_size=10
+query.page_size=2
 
 # cassandra backend config
 cassandra.host=127.0.0.1