Skip to content

Commit 3860fa1

Browse files
porunovclaude
andcommitted
Leave out removed vertices before sorting a query in memory
Fixes #5099 A graph query ordered by a key which its index doesn't sort it by, such as one which a composite index answers, or a mixed index which doesn't hold the order key, failed with IllegalStateException: Vertex with id N was removed, in a transaction which had removed one of the vertices it matched. QueryProcessor's PreSortingIterator sorted what the index returned, reading the order key of each element, before getFilterIterator left out the elements which the transaction had deleted, and the order key of a removed vertex can't be read. An unordered query, which reads no order key, and one which the index sorts, which JanusGraph doesn't sort, weren't affected, nor were the edges and properties which the transaction removed, whose ids turn into no element. The pre-sorting iterator now leaves out the elements which the transaction removed (isRemoved) before it sorts the others. getFilterIterator still leaves out the other elements which the transaction deleted, such as those which it changed so that they no longer match, as the sorted elements are reached: only the elements which are read are checked, and a change made while they are read is seen, as for an unsorted query. The check of how many elements a pre-sorting read fetched counts every element which the backend returned, as before. Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> Co-authored-by: Oleksandr Porunov <alexandr.porunov@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 26a78cc commit 3860fa1

5 files changed

Lines changed: 179 additions & 5 deletions

File tree

‎docs/changelog.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,6 +1275,17 @@ Elasticsearch and Solr do: a query which streamed 500,000 ids from Elasticsearch
12751275
after every change, so that a hundred index queries of one result each in a transaction take 0.71 ms instead of 0.89 on
12761276
one thread, and 1.1 ms instead of 3.1 with eight threads running such transactions at once, on the inmemory backend.
12771277

1278+
##### Graph queries which JanusGraph sorts leave out the vertices which the transaction removed
1279+
1280+
A graph query ordered by a key which its index doesn't sort it by, such as one which a composite index answers, or a
1281+
mixed index which doesn't hold the order key, failed in a transaction which had removed one of the vertices it matched,
1282+
with an `IllegalStateException` that the vertex was removed: JanusGraph sorted what the index returned before it left
1283+
out the elements which the transaction had removed, and read the order key of the removed vertex. It now leaves out the
1284+
removed vertices before it sorts the others, as the query without an order leaves them out. So
1285+
`g.V().has('kind', 'p').order().by('name')`, with `kind` in a composite index, returns the vertices of kind `p` which
1286+
are left in a transaction which dropped one of them, as does
1287+
`graph.query().has('kind', 'p').orderBy('name', asc).vertices()`.
1288+
12781289
##### Graph queries read on past the edges and properties which the transaction removed
12791290

12801291
An index, mixed or composite, still holds the edges and vertex properties which the transaction removed, until it

‎janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphIndexTest.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5401,6 +5401,46 @@ public void testLimitedQueriesOfPropertiesReadPastThePropertiesWhichTheTransacti
54015401
}
54025402
}
54035403

5404+
/**
5405+
* An ordered query which a mixed index answers without sorting it, as the index doesn't hold the order key, leaves
5406+
* out the vertices which the transaction removed, which the index still returns, before JanusGraph sorts them, as
5407+
* the order values of a removed vertex can't be read; the edges which it removed, whose ids turn into no element,
5408+
* are left out as well
5409+
*/
5410+
@Test
5411+
public void testQueriesSortedInMemorySkipTheElementsWhichTheTransactionRemoved() {
5412+
final PropertyKey kind = makeKey("kind", String.class);
5413+
makeKey("name", String.class);
5414+
mgmt.makeEdgeLabel("road").multiplicity(Multiplicity.MULTI).make();
5415+
mgmt.buildIndex("mixed", Vertex.class).addKey(kind, getStringMapping()).buildMixedIndex(INDEX);
5416+
mgmt.buildIndex("edges", Edge.class).addKey(kind, getStringMapping()).buildMixedIndex(INDEX);
5417+
finishSchema();
5418+
final Vertex from = tx.addVertex("kind", "q");
5419+
for (final String name : Arrays.asList("a", "b", "c", "d")) {
5420+
from.addEdge("road", tx.addVertex("kind", "p", "name", name), "kind", "p", "name", name);
5421+
}
5422+
newTx();
5423+
final GraphTraversalSource g = tx.traversal();
5424+
//The vertex named b, and with it its edge, and the edge named c
5425+
g.V().has("kind", "p").has("name", "b").drop().iterate();
5426+
g.E().has("kind", "p").has("name", "c").drop().iterate();
5427+
assertEquals(Arrays.asList("a", "c", "d"), g.V().has("kind", "p").order().by("name").values("name").toList());
5428+
assertEquals(Arrays.asList("d", "c"),
5429+
g.V().has("kind", "p").order().by("name", desc).limit(2).values("name").toList());
5430+
final List<Object> vertexNames = new ArrayList<>();
5431+
tx.query().has("kind", "p").orderBy("name", asc).vertices().forEach(v -> vertexNames.add(v.value("name")));
5432+
assertEquals(Arrays.asList("a", "c", "d"), vertexNames);
5433+
assertEquals(Arrays.asList("a", "d"), g.E().has("kind", "p").order().by("name").values("name").toList());
5434+
assertEquals(Collections.singletonList("d"),
5435+
g.E().has("kind", "p").order().by("name", desc).limit(1).values("name").toList());
5436+
final List<Object> edgeNames = new ArrayList<>();
5437+
tx.query().has("kind", "p").orderBy("name", asc).edges().forEach(e -> edgeNames.add(e.value("name")));
5438+
assertEquals(Arrays.asList("a", "d"), edgeNames);
5439+
assertEquals(Arrays.asList("a", "d"),
5440+
g.V(from).local(__.outE("road").order().by("name")).values("name").toList());
5441+
tx.rollback();
5442+
}
5443+
54045444
//A query which removes the edges it reads, as it reads them, gets each of its edges once: a read under a larger
54055445
//limit skips what the index returned before, rather than as many edges as were returned, which are removed by then
54065446
@Test

‎janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,102 @@ public void testUpdateEdgePropertyThenRemoveEdge() {
476476
assertTrue(graph.traversal().E().has("_e", 2).hasNext());
477477
}
478478

479+
/**
480+
* An ordered query which JanusGraph sorts itself, as its index doesn't sort it, leaves out the vertices which the
481+
* transaction removed before it sorts them, as the order values of a removed vertex can't be read; the edges which
482+
* it removed, whose ids turn into no element, are left out as well
483+
*/
484+
@Test
485+
public void testQueriesSortedInMemorySkipTheElementsWhichTheTransactionRemoved() {
486+
final PropertyKey kind = makeKey("kind", String.class);
487+
makeKey("name", String.class);
488+
mgmt.makeEdgeLabel("road").multiplicity(Multiplicity.MULTI).make();
489+
mgmt.buildIndex("verticesByKind", Vertex.class).addKey(kind).buildCompositeIndex();
490+
mgmt.buildIndex("edgesByKind", Edge.class).addKey(kind).buildCompositeIndex();
491+
finishSchema();
492+
final Vertex from = tx.addVertex("kind", "q");
493+
for (final String name : Arrays.asList("a", "b", "c", "d")) {
494+
from.addEdge("road", tx.addVertex("kind", "p", "name", name), "kind", "p", "name", name);
495+
}
496+
newTx();
497+
final GraphTraversalSource g = tx.traversal();
498+
//The vertex named b, and with it its edge, and the edge named c
499+
g.V().has("kind", "p").has("name", "b").drop().iterate();
500+
g.E().has("kind", "p").has("name", "c").drop().iterate();
501+
assertEquals(Arrays.asList("a", "c", "d"), g.V().has("kind", "p").order().by("name").values("name").toList());
502+
assertEquals(Arrays.asList("d", "c"),
503+
g.V().has("kind", "p").order().by("name", desc).limit(2).values("name").toList());
504+
final List<Object> vertexNames = new ArrayList<>();
505+
tx.query().has("kind", "p").orderBy("name", asc).vertices().forEach(v -> vertexNames.add(v.value("name")));
506+
assertEquals(Arrays.asList("a", "c", "d"), vertexNames);
507+
assertEquals(Arrays.asList("a", "d"), g.E().has("kind", "p").order().by("name").values("name").toList());
508+
assertEquals(Collections.singletonList("d"),
509+
g.E().has("kind", "p").order().by("name", desc).limit(1).values("name").toList());
510+
final List<Object> edgeNames = new ArrayList<>();
511+
tx.query().has("kind", "p").orderBy("name", asc).edges().forEach(e -> edgeNames.add(e.value("name")));
512+
assertEquals(Arrays.asList("a", "d"), edgeNames);
513+
assertEquals(Arrays.asList("a", "d"),
514+
g.V(from).local(__.outE("road").order().by("name")).values("name").toList());
515+
tx.rollback();
516+
}
517+
518+
/**
519+
* The elements which the transaction removes, or changes so that they no longer match, while it reads the sorted
520+
* results of a query are left out as they are reached, as for an unsorted query. They are checked where the
521+
* transaction had a change when a graph query began, or a change of the vertex's relations when a vertex query
522+
* began.
523+
*/
524+
@Test
525+
public void testQueriesSortedInMemoryLeaveOutTheElementsRemovedWhileTheyAreRead() {
526+
final PropertyKey kind = makeKey("kind", String.class);
527+
makeKey("name", String.class);
528+
mgmt.makeEdgeLabel("road").multiplicity(Multiplicity.MULTI).make();
529+
mgmt.buildIndex("verticesByKind", Vertex.class).addKey(kind).buildCompositeIndex();
530+
finishSchema();
531+
final Vertex from = tx.addVertex("kind", "q");
532+
for (final String name : Arrays.asList("a", "b", "c", "d", "e", "f", "g", "h")) {
533+
from.addEdge("road", tx.addVertex("kind", "p", "name", name), "name", name);
534+
}
535+
newTx();
536+
tx.addVertex("name", "unrelated");
537+
final List<Object> names = new ArrayList<>();
538+
for (final JanusGraphVertex vertex : tx.query().has("kind", "p").orderBy("name", asc).vertices()) {
539+
names.add(vertex.value("name"));
540+
if (names.size() == 1) {
541+
tx.traversal().V().has("kind", "p").has("name", "g").next().remove();
542+
}
543+
}
544+
assertEquals(Arrays.asList("a", "b", "c", "d", "e", "f", "h"), names);
545+
tx.rollback();
546+
newTx();
547+
548+
//and so are those which it changes so that they no longer match
549+
tx.addVertex("name", "unrelated");
550+
names.clear();
551+
for (final JanusGraphVertex vertex : tx.query().has("kind", "p").orderBy("name", asc).vertices()) {
552+
names.add(vertex.value("name"));
553+
if (names.size() == 1) {
554+
tx.traversal().V().has("kind", "p").has("name", "g").property("kind", "z").iterate();
555+
}
556+
}
557+
assertEquals(Arrays.asList("a", "b", "c", "d", "e", "f", "h"), names);
558+
tx.rollback();
559+
newTx();
560+
561+
//and so are the edges of a vertex whose relations the transaction changed
562+
final JanusGraphVertex source = (JanusGraphVertex) tx.traversal().V(from).next();
563+
source.property("name", "source");
564+
final List<Object> edgeNames = new ArrayList<>();
565+
for (final JanusGraphEdge edge : source.query().labels("road").orderBy("name", asc).edges()) {
566+
edgeNames.add(edge.value("name"));
567+
if (edgeNames.size() == 1) {
568+
tx.traversal().V(source).outE("road").has("name", "g").next().remove();
569+
}
570+
}
571+
assertEquals(Arrays.asList("a", "b", "c", "d", "e", "f", "h"), edgeNames);
572+
tx.rollback();
573+
}
574+
479575
@Test
480576
public void testUpdateForkEdgePropertyThenRemoveEdge() {
481577
initializeGraphWithVerticesAndEdges();

‎janusgraph-core/src/main/java/org/janusgraph/graphdb/query/QueryProcessor.java‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -248,13 +248,26 @@ private final class PreSortingIterator implements CloseableIterator<R> {
248248

249249
private final Iterator<R> iterator;
250250

251+
/*
252+
* Sorts what the backend returns, once it has left out the elements which the transaction removed, as the
253+
* order values of a removed vertex can't be read. getFilterIterator leaves out the other elements which the
254+
* transaction deleted, such as those which it changed so that they no longer match, as they are reached, as
255+
* for an unsorted query, so that a change which the caller makes while it reads the results is seen.
256+
*/
251257
private PreSortingIterator(BackendQueryHolder<B> backendQueryHolder) {
252-
List<R> all = new ArrayList<>();
253-
executor.execute(query,
258+
final List<R> all = new ArrayList<>();
259+
final Iterator<R> results = executor.execute(query,
254260
backendQueryHolder.getBackendQuery().updateLimit(MAX_SORT_ITERATION),
255-
backendQueryHolder.getExecutionInfo(),backendQueryHolder.getProfiler())
256-
.forEachRemaining(all::add);
257-
if (all.size() >= MAX_SORT_ITERATION)
261+
backendQueryHolder.getExecutionInfo(),backendQueryHolder.getProfiler());
262+
int fetched = 0;
263+
while (results.hasNext()) {
264+
final R result = results.next();
265+
fetched++;
266+
if (!result.isRemoved()) {
267+
all.add(result);
268+
}
269+
}
270+
if (fetched >= MAX_SORT_ITERATION)
258271
throw new QueryException("Could not execute query since pre-sorting requires fetching more than " +
259272
MAX_SORT_ITERATION + " elements. Consider rewriting the query to exploit sort orders");
260273
all.sort(query.getSortOrder());

‎janusgraph-core/src/test/java/org/janusgraph/graphdb/query/QueryProcessorTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import static org.junit.jupiter.api.Assertions.assertTrue;
3434
import static org.mockito.ArgumentMatchers.any;
3535
import static org.mockito.Mockito.doReturn;
36+
import static org.mockito.Mockito.doThrow;
3637
import static org.mockito.Mockito.mock;
3738
import static org.mockito.Mockito.when;
3839

@@ -105,6 +106,19 @@ public void asksNothingOfABackendWhichDoesntSort() {
105106
assertNull(executor.lastValue);
106107
}
107108

109+
//An element which the transaction removed, whose order value can't be read, is left out before the others are
110+
//sorted
111+
@Test
112+
public void leavesOutTheRemovedElementsBeforeSortingWhatTheBackendReturns() {
113+
final List<JanusGraphElement> stored = elements(8L, 6L, 4L);
114+
final JanusGraphElement removed = mock(JanusGraphElement.class);
115+
doReturn(true).when(removed).isRemoved();
116+
doThrow(new IllegalStateException("The vertex was removed")).when(removed).valueOrNull(key);
117+
stored.add(1, removed);
118+
final Executor executor = new Executor(elements(), stored);
119+
assertEquals(Arrays.asList(stored.get(0), stored.get(2), stored.get(3)), run(query(true, 10, false), executor));
120+
}
121+
108122
private List<JanusGraphElement> run(ElementQuery<JanusGraphElement, FakeBackendQuery> query, Executor executor) {
109123
final List<JanusGraphElement> result = new ArrayList<>();
110124
new QueryProcessor<>(query, executor).iterator().forEachRemaining(result::add);

0 commit comments

Comments
 (0)