From ffb09f1359bd628f817a4f9dcedf4e5285bc5996 Mon Sep 17 00:00:00 2001 From: Linary Date: Mon, 22 Jul 2019 11:37:42 +0800 Subject: [PATCH] Fix query by eq with range index in page only return first page data (#614) Fix #613 Change-Id: I3cfd3d7ff4ce1ca13b9b6f695ba5ba88569f9266 --- .../baidu/hugegraph/backend/query/Query.java | 18 ++- .../backend/serializer/BinarySerializer.java | 26 +++- .../backend/store/mysql/MysqlTable.java | 9 +- .../baidu/hugegraph/core/EdgeCoreTest.java | 18 +-- .../baidu/hugegraph/core/VertexCoreTest.java | 146 ++++++++++++++++-- .../src/main/resources/hugegraph.properties | 2 +- 6 files changed, 174 insertions(+), 45 deletions(-) diff --git a/hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/Query.java b/hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/Query.java index d85a4f856..01d2be295 100644 --- a/hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/Query.java +++ b/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; } diff --git a/hugegraph-core/src/main/java/com/baidu/hugegraph/backend/serializer/BinarySerializer.java b/hugegraph-core/src/main/java/com/baidu/hugegraph/backend/serializer/BinarySerializer.java index 559cb984a..89ee06b72 100644 --- a/hugegraph-core/src/main/java/com/baidu/hugegraph/backend/serializer/BinarySerializer.java +++ b/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; diff --git a/hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTable.java b/hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTable.java index cd25f763e..90af4dc06 100644 --- a/hugegraph-mysql/src/main/java/com/baidu/hugegraph/backend/store/mysql/MysqlTable.java +++ b/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(";"); } diff --git a/hugegraph-test/src/main/java/com/baidu/hugegraph/core/EdgeCoreTest.java b/hugegraph-test/src/main/java/com/baidu/hugegraph/core/EdgeCoreTest.java index 79b8fd248..d02535646 100644 --- a/hugegraph-test/src/main/java/com/baidu/hugegraph/core/EdgeCoreTest.java +++ b/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(); }); } diff --git a/hugegraph-test/src/main/java/com/baidu/hugegraph/core/VertexCoreTest.java b/hugegraph-test/src/main/java/com/baidu/hugegraph/core/VertexCoreTest.java index 59058c0a9..a9af823a8 100644 --- a/hugegraph-test/src/main/java/com/baidu/hugegraph/core/VertexCoreTest.java +++ b/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 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 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 iter = g.V().hasLabel("software") + .has("price", P.eq(100)) + .has("~page", "") + .limit(3); + + List vertices1 = IteratorUtils.list(iter); + String page = TraversalUtil.page(iter); + Assert.assertEquals(3, vertices1.size()); + List 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() diff --git a/hugegraph-test/src/main/resources/hugegraph.properties b/hugegraph-test/src/main/resources/hugegraph.properties index 7bf03f16f..9b4f0f13b 100644 --- a/hugegraph-test/src/main/resources/hugegraph.properties +++ b/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