Repository navigation
Conversation
Fixes JanusGraph#5107 JanusGraph folds an order().by(key) of a traversal into its query, and the query returned the elements which have no value of the key as well, after the others. TinkerPop leaves such an element out, as a by() modulator which yields no value filters the traverser: g.V().has("kind", "p").order().by("name") returned the vertices without a name, which TinkerGraph doesn't, and a count() after it counted them. A query which JanusGraph builds for a traversal now requires a value of each key it is ordered by. For a graph query that is a condition which doesn't take part in choosing the indexes, as an index which holds the key would otherwise be read for every element which has a value of it: a mixed index which the query reads for its other conditions takes it as an exists clause, and skips these elements itself, and the query checks it in memory otherwise, also against the elements which the transaction added or changed, reading on where it leaves some out. A query of the edges or properties of a vertex takes it as a has(key). A query of JanusGraph's API still keeps these elements, last, and so does an order of vertices by a key of several values, by which only an index orders them, for each of its keys: the query would take the vertices whose value of a key the transaction changed among its new ones, which it can't order by such a key, and TinkerPop fails on an element with several values of it. An edge holds one value of a key at most, so an order of edges leaves out those without the key whatever its cardinality. Several order() steps in a row are folded in together, by the keys of the last one and then by those of the ones before it, as TinkerPop's sorts, which keep the order of ties, order the elements, and the elements without one of those keys are left out; JanusGraph dropped all but the last of them. They are left to TinkerPop where one of them isn't by a key. So are an order by(key, shuffle), which failed with an AssertionError, an order of the edges or properties of a vertex by several keys, which failed with an IllegalArgumentException as such a query takes a single key, a by(key) whose traversal yields a value where the key has none, as the deprecated ProductiveByStrategy makes it, and an ElementValueComparator which orders other elements than the step's. An order by the same key twice, which failed with an IllegalArgumentException, orders by the first. An or() with a branch whose order isn't folded in and may leave the element out, as one by shuffle or by any traversal, is left to TinkerPop, rather than folded without that order, together with the has() steps after it, which TinkerPop then checks against every element which the query of the steps before the or() returns. The relations which a multi-query loaded into the cache of a vertex were returned in the order of the storage backend, also for a query in an order which no vertex-centric index gives them. So local(outE().order().by(key).limit(n)) took the wrong edges where the traversal fetched them in a batch, as query.batch.enabled has it by default, and so did an ordered JanusGraphMultiVertexQuery. Such a query is now sorted on the default path. 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>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
What changed in this PR
Aligns JanusGraph traversal ordering semantics with TinkerPop by excluding elements/relations that lack values for order().by(key) keys (instead of returning them last), while preserving existing behavior for JanusGraph API queries.
Changes:
- Require presence of order keys for folded traversal orders (graph-centric and vertex-centric), leveraging mixed-index “exists” where supported.
- Improve order-folding behavior (multiple
order()steps,shuffle, comparator edge-cases) to match TinkerPop and avoid previous exceptions. - Add/adjust tests and docs to verify and document the new traversal semantics and remaining exceptions (e.g., multi-valued order keys for vertices).
| File | Description |
|---|---|
| janusgraph-core/src/main/java/org/janusgraph/graphdb/query/graph/GraphCentricQueryBuilder.java | Adds requireOrderValues() and injects presence predicates / mixed-index exists narrowing post index selection. |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/query/graph/JointIndexQuery.java | Extracts conjunction-building helper to avoid nested And issues (Solr parser). |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/query/vertex/BasicVertexCentricQueryBuilder.java | Avoids returning cached relations unsorted when a sorted result is required. |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/step/HasStepFolder.java | Updates order fold-in validation and behavior (multi-order folding, shuffle, bypass traversals, or()-branch handling). |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/step/JanusGraphStep.java | Enables order-key presence enforcement for graph-centric folded queries. |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/step/JanusGraphVertexStep.java | Adds has(key) to vertex-centric queries for folded order keys. |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/step/JanusGraphPropertiesStep.java | Adds has(key) to property queries for folded order keys. |
| janusgraph-test/src/test/java/org/janusgraph/graphdb/query/QueryTest.java | Adds non-indexed reference-comparison tests vs TinkerGraph for ordering semantics. |
| janusgraph-test/src/test/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphStepStrategyTest.java | Expands strategy tests for multi-order folding, shuffle, bypass traversal, and comparator cases. |
| janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphIndexTest.java | Adds mixed/composite/no-index coverage and transaction behavior tests vs TinkerGraph. |
| janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java | Adds vertex-centric traversal ordering tests vs TinkerGraph across batch/non-batch. |
| docs/schema/index-management/index-performance.md | Documents traversal behavior change for missing order-key values and how to keep such elements. |
| docs/index-backend/solr.md | Clarifies traversal vs API query behavior for ordering elements without values. |
| docs/changelog.md | Adds changelog entry explaining behavioral change and which orders are now left to TinkerPop. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
3 resolved since last review
🧠 Review effort: Lite



Fixes #5107
JanusGraph folds a traversal's
order().by(key)into its query, and the query returned the elements which have no value of the key as well, after the others. TinkerPop leaves such an element out, as aby()modulator which yields no value filters the traverser, and so does TinkerGraph:g.V().has('kind', 'p').order().by('name')returned the vertices without aname, last, and acount()after it counted them. That held wherever JanusGraph folds the order in: in a query which a mixed index sorts, one which JanusGraph sorts in memory, for the elements which a transaction added or changed, and in an order of the edges or properties of a vertex.The traversal now leaves these elements out, and returns what TinkerGraph returns for the same elements. A query of JanusGraph's API, such as
graph.query().has('kind', 'p').orderBy('name', asc).vertices(), still keeps them, last.Along the way, these orders now give what TinkerPop gives too:
order().by(key, shuffle)failed with anAssertionError.order()steps in a row, JanusGraph dropped all but the last, together with the elements they leave out and the order they give the ties of the last one.IllegalArgumentException.query.batch.enabled, the default, a folded order of a vertex's edges which no vertex-centric index gives them came in the order of the storage backend, and its limit took the wrong edges. So did an orderedJanusGraphMultiVertexQuery.ProductiveByStrategy, JanusGraph placed the elements without the key last, where TinkerPop places them first when ascending.order().by('name').by('name', desc), failed withIllegalArgumentException: orders [[ASC(name)]] already contains key [name].Changes
GraphCentricQueryBuilder.requireOrderValues()(new) makes the query leave out the elements without a value of an order key.constructQueryWithoutProfileadds ahas(key)(Cmp.NOT_EQUALnull) for each order key to the query's condition, unless the condition has it already, after the index selection strategy chose the indexes:ENABLED, and able to tell which elements have a value of it (IndexSerializer.supportsExistsQuery), takes it as an exists clause, joined to its conjuncts as Find the vertices a transaction changed for its graph queries by key and value #5023'snarrowFirstjoins its range. The clause then counts as covered, so that the query is still fitted where it was. A mixed index which sorts the query always holds its order keys, so it skips these elements itself, and a limit reads no more than before.getNewby the query's condition, which now holds the presence of the order keys, so that a vertex which gained a value of the key in the transaction is found where the index skips it, and one which lost it is left out (isDeleted).LISTorSET, which JanusGraph folds in for vertices only where every mixed index backend can sort by it, keeps the vertices without a value of any of its keys, last, as before. A presence condition on any of its keys, of one value or several, would bring the vertices whose value of that key the transaction changed intogetNew, and the query would compare them with the others byOrderList, which can't take a key of several values: Find the vertices a transaction changed for its graph queries by key and value #5023 leaves such vertices where the index places them for that reason. TinkerPop itself fails on an element with several values of an order key. An edge holds one value of a key at most, also of one declaredLIST, which JanusGraph folds an order of edges by on any backend, so an order of edges leaves out those without the key whatever its cardinality.JointIndexQuery.conjunction(new), whichnarrowFirstnow uses too. It joins the conjuncts rather than nesting a conjunction, as Solr writes a nested conjunction without parentheses, which its query parser rejects; Keep the clauses of each condition together in Solr queries #5079 fixes that, after which the nesting would do as well.JanusGraphSteprequires the values of the order keys in every query it builds, that of anor()branch included.JanusGraphVertexStepandJanusGraphPropertiesStepadd ahas(key)for each order key they fold in to the query of a vertex's edges or properties, unless one of the step's conditions is thathas(key)already (HasStepFolder.requiresAValueOf, new). A query of a vertex's relations takes it as a condition which no slice holds, so it chooses the same slices and checks the condition in memory, reading on where it leaves relations out.HasStepFolder.foldInOrderfolds severalorder()steps in a row in together, the keys of the last one first and then those of the ones before it, as TinkerPop's sorts, which keep the order of ties, order the elements; a key which the orders taken so far already have, those of a laterorder()step or of an earlierby()of the same step, is taken once, so an order by the same key twice orders by the first. Each of those keys then needs a value. Where one of the steps can't be folded in, none of them is, and TinkerPop runs them all. It dropped all but the last before. An order of a vertex's edges or properties by several keys isn't folded in, as such a query takes a single key.HasStepFolder.validFoldInHasContainerleaves anor()to TinkerPop where anorder()of one of its branches isn't folded in and may leave the element out, as one byshuffle(dropsElementsUnfolded, new), rather than folding the branch without that order. An order which leaves out no element, one of the element itself, byT.idorT.label, or by a key whichProductiveByStrategymakes yield null where it has none (yieldsForEveryElement, new), is dropped as before, and theor()folded in; an order of edges is checked as one of edges, which may be by a key declaredLIST:g.V().or(__.has('kind', 'p').order().by('name', shuffle), __.has('kind', 'q'))returned the vertices of kindpwithout a name too, as the branch's order was dropped, where TinkerPop leaves them out; on master it failed with theAssertionError. (With Apply the limits, ranges and orders of or() branches to each element #5106, which folds a branch's order in as ahas(key), thator()is folded in, and returns the same.) Such anor(), and thehas()steps after it, now go to TinkerPop, which checks every element which the query of thehas()steps before theor()returns, every vertex where there are none, and so fails underquery.force-index. That holds for a branch ordered by any traversal, as JanusGraph can't tell that one such asby(__.values('age').count())leaves no element out.dropsElementsUnfoldedtells an order of vertices from one of edges by the traversal's start step, not by theJanusGraphStepwhich theor()folds into, so ing.E()...V().or(...)it checks a branch's order as one of edges, which may be by a key declaredLIST; the rebase removes this check (see the Draft note).HasStepFolder.validJanusGraphOrderdoesn't fold in:Order.shuffle, whichOrder.convertfailed on with anAssertionError;by(key)whoseValueTraversalhas a bypass traversal, whichProductiveByStrategygives it so that it yields null where the key has none, andSubgraphStrategywith a filter of vertex properties too, whose orders TinkerPop then sorts, more correctly and more slowly;ElementValueComparatorof other elements than the step's, such asby(out('knows'), new ElementValueComparator('name', asc)), which was folded in as an order of the step's own elements byname.BasicVertexCentricQueryBuilder.loadRelationsFromCachereturns the relations which a multi-query loaded into a vertex's cache as they are only where each subquery is sorted by the query's order, and sorts them on the default path otherwise.or()steps with a branch ordered by a traversal, which go to TinkerPop with thehas()steps after them. The section on code point order said that such an element comes last in a merge ofor()branches; it now refers to the new section.Tests
JanusGraphIndexTest.testOrderLeavesOutTheElementsWithoutAKeyItOrdersBy(every index backend; the mixed indexes are namedmixedandedges, which the Solr test container creates). Vertices of kindspandq, some without anameor anage, and edges of one vertex, some without aweight, are written into the graph and into a TinkerGraph. Fourteen traversals return the same from both:nameand byage, ascending and descending, with limits and ranges, by two keys, twoorder()steps in a row, and acount()after an order, which the mixed index sorts or counts;edges, and of the edges of a vertex.They return the same again in a transaction which adds a vertex with a
nameand one without, gives a vertex anameand takes another one's, takes anage, and gives an edge aweightand takes another one's. A new transaction's profiles show the exists clauses in the queries of both mixed indexes.JanusGraphIndexTest.testOrderByAKeyOfSeveralValuesKeepsTheElementsWithoutIt(index backends which sort by keys of several values, Elasticsearch and Solr): an order by aLISTkey, alone and followed by a key of one value, keeps the vertices without a value of either key, last, and doesn't fail on the vertices whose values of either key the transaction changed.JanusGraphIndexTest.testMixedIndexOrdersElementsWithoutTheKeyLastandtestMixedIndexOrdersElementsWithoutABooleanKeyLast(from Fix the aggregations and orders a mixed index answers #5032) check the place of an element without the key throughgraph.query().orderBy(), which keeps it, and that a traversal leaves it out.JanusGraphIndexTest.testTransactionReadsTheIndexOnlyUpToItsLastNewValueWithinTheLimit(from Find the vertices a transaction changed for its graph queries by key and value #5023): the new vertices without anagedon't count towards a traversal's limit, while a query of the transaction places them after the others.JanusGraphIndexTest.testTransactionQueriesAgreeWithAScanOfTheirElements(from Find the vertices a transaction changed for its graph queries by key and value #5023): the scan leaves out the elements without a value of an order key, as the traversals now do.JanusGraphTest.testVertexCentricOrderLeavesOutTheRelationsWithoutTheKey(every storage backend). A vertex has edges of a label which a vertex-centric index sorts byweight, descending, and of a label which none sorts, some without aweight, andLISTproperties with a meta-propertyat, some without it.local()orders of its edges and properties, ascending and descending, with and without limits, and by two keys, return what a TinkerGraph returns, in a transaction which fetches in batches and in one which doesn't. The vertex's own query and aJanusGraphMultiVertexQueryreturn the edges in order, those without aweightlast.QueryTest.testOrderLeavesOutTheElementsWithoutAKeyItOrdersByAsTinkerGraphDoes(inmemory, no index). Orders by keys, twoorder()steps in a row, and an order by a traversal next to one by a key return what TinkerGraph returns.order().by('name', shuffle)returns the vertices which have a name. WithProductiveByStrategy, the order is TinkerPop's, the vertex without anagefirst. An order by anElementValueComparatorleaves out the vertex without the key. An order by the same key twice orders by the first. Anor()with a branch ordered byshufflereturns what TinkerGraph returns, the vertices of the branch which have a name, and so door()steps with branches ordered by the vertex itself, its label, and a key underProductiveByStrategy. An order of edges by a key declaredLIST, of which each edge holds one value at most, leaves out the edge without it, ascending and descending, as TinkerGraph does.QueryTest.testOrBranchesAreMergedInTheOrderOfTheirQueries(from Order the strings of graph queries by code point, as the indexes do #5067) no longer expects the vertex without a name.JanusGraphStepStrategyTest: twoorder()steps fold into one order by the last one's key and then the first one's; an order by a traversal and an order next to it,shuffle, and anElementValueComparatorof a traversal's elements aren't folded in;or()steps whose branches are ordered by the element itself, its label, or a key underProductiveByStrategyare folded in.On master
3285cbf18, with these tests:testOrderLeavesOutTheElementsWithoutAKeyItOrdersByfails on Lucene at its first traversal:[2, 6, 1, 4, 5, 3], where TinkerGraph gives[2, 6, 1, 4]. The two tests of Fix the aggregations and orders a mixed index answers #5032 and the randomized test of Find the vertices a transaction changed for its graph queries by key and value #5023 fail with the elements without the key returned last.testVertexCentricOrderLeavesOutTheRelationsWithoutTheKeyfails on inmemory in the transaction which fetches in batches, with[13, 15, 12, 10, 11, 14]: every edge, in the storage order. TinkerGraph gives[12, 13, 10, 15].QueryTesttests fail with the vertices without a name returned last, and fourJanusGraphStepStrategyTestcases fail. The order by the same key twice fails with theIllegalArgumentException.Mutations, each reverting one part of the change, are all caught (those of the reviews' changes on
QueryTestandJanusGraphStepStrategyTest):JanusGraphStepnot requiring the values: the index, query and Find the vertices a transaction changed for its graph queries by key and value #5023 tests return the elements without the key.has(key)in the vertex steps: the vertex-centric test returns the edges without aweight.JanusGraphMultiVertexQueryreturns[2, 3, null, 5, null, 1].shufflefolded in: theAssertionError, and the strategy case.ProductiveByStrategycheck returns[5, 6, 2, 3, 1].order()steps folded in, the others dropped, as before: the index test,QueryTestand two strategy cases fail.IllegalArgumentException.ElementValueComparatorof a traversal's elements folded in: the strategy case.testOrderByAKeyOfSeveralValuesKeepsTheElementsWithoutItloses the vertex withoutnames, and Find the vertices a transaction changed for its graph queries by key and value #5023'stestTransactionKeepsTheIndexOrderOfAKeyOfSeveralValuesfails withIllegalStateException: Multiple properties exist for the provided keyfromOrderList.compare, which the query reaches by merging the vertex whosenamesthe transaction changed.LIST, as for vertices:QueryTest's order of edges returns the edge without the key, last.or()folded in without the order of a branch which isn't folded in, as before:QueryTest'sor()with ashufflebranch returns the vertex of the branch without a name too.or()with a branch order which isn't folded in left to TinkerPop, as in a first version: the two strategy cases ofor()steps with branches ordered by the element itself, its label, and a key underProductiveByStrategy.IllegalArgumentException.testOrderByAKeyOfSeveralValuesKeepsTheElementsWithoutItloses the vertex without anageinorder().by('names', desc).by('age'), and Find the vertices a transaction changed for its graph queries by key and value #5023's test fails with the same exception where the transaction changed theageof a vertex with severalnames.Locally, Java 11, with checkstyle (no violations), with fresh builds of this change's code as it stands:
QueryTestandJanusGraphStepStrategyTest(96) and theor(), order and transaction tests ofBerkeleyLuceneTest(22) pass.-Dtest.skip.tp=false,InMemoryJanusGraphProcessTestandInMemoryMultiQueryJanusGraphProcessTest, 1,860 tests, 80 skipped) pass.or(), order and transaction tests ofBerkeleyElasticsearchTest(23) pass against Elasticsearch 9.5.4 in a container.BerkeleyElasticsearchTest(119, 1 skipped). In one run of janusgraph-test,ManagementLoggerGraphCacheEvictionTest, which doesn't touch orders, failed twice on a timeout while two suites ran at once; it passed on its own and in the next run.Notes
This changes what traversals return. A traversal which orders by a key no longer returns the elements without it, and a
count()or other aggregation after such an order no longer counts them. The changelog says how to keep them: order by a traversal which gives them a value, such asorder().by(coalesce(values('name'), constant(''))), which TinkerPop then sorts in memory. A query of JanusGraph's API is unchanged.Cost. A traversal ordered by a key now checks each element for a value of it, unless a mixed index skips the elements without it, and a query of a vertex's edges or properties ordered by a key now checks each of them in memory, instead of taking the path of a query which the storage backend answers whole. An A/B with JMH against master's
janusgraph-core, on an inmemory graph, ofg.V(v).local(outE(label).order().by('w').limit(10))(firstTen) andg.V(v).local(outE(label).order().by('w', desc))(all) in a new transaction each, with a label which a vertex-centric index sorts ascending byw(sorted) and one which none sorts (plain), 100 or 10,000 edges each with aw, andquery.batch.enabledon or off (two passes of reversed order, 3 forks a side each, so 6 per side; each figure the median of the six fork means; the means of the forks give somewhat different figures, such as +3.8% for a cell whose median gives +6.6%; p of an exact Mann-Whitney test over the forks):sorted, no batchsorted, batchplain, no batchplain, batchfirstTenfirstTenallallWhere master is "wrong", it fetched the edges in a batch and returned them in the storage order without sorting them:
firstTenof theplainlabel returnedwof, e.g.,71, 75, 79, …, depending on the ids which the edges got, instead of0, 1, 2, …, andallcame unsorted; this change sorts them, at the cost of master's unbatched path. Where master was right, the change costs about 2 µs a query, 7–10%, on an order which a vertex-centric index sorts, and up to 8% where JanusGraph sorts the edges itself. Leaving out the edges without the key in the slice which a vertex-centric index reads, rather than in memory, could save the former; that needs the edge serializer to bound the slice before the missing values, and is left out here.Several
order()steps in a row now order the ties of the last one by the ones before it, as TinkerPop does, where JanusGraph left them in the order of its query.An order by an
ElementValueComparator, on which TinkerPop fails where an element has no value, leaves such an element out, as an order by the key does.An order of vertices which has a key of several values keeps the vertices without a value of any of its keys, last, as before (see Changes). Order by a key of several values as the indexes do #5102 compares such a key in memory by the least or greatest of its values, as the indexes sort it, so the rebase onto it lifts this exception (see the Draft note).
Folding several
order()steps in a row can cost more: where the index which sorts the query doesn't hold the key of an earlier step, as with onlybindexed inorder().by('a').order().by('b').limit(n), JanusGraph now sorts every match in memory, where it read the firstnbybfrom the index, leaving their ties unordered. The changelog says so.SubgraphStrategywith a filter of vertex properties gives theValueTraversalof aby(key)a bypass traversal asProductiveByStrategydoes, so such orders aren't folded in either: TinkerPop sorts them, more correctly and more slowly.An order which JanusGraph leaves to TinkerPop, such as one by a traversal, behaved as TinkerPop's already.
With Apply the limits, ranges and orders of or() branches to each element #5106, which applies the limits, ranges and orders of
or()branches to each element, this one merges without conflicts, and is to merge after it. Merged, the two agree with TinkerGraph on 400 randomor()traversals on 30 vertices, with orders of the step over elements without the key, on inmemory and Lucene; in one run, whose counts vary somewhat with the ids which the vertices get, master gave another result for 239 of them on Lucene and 234 on inmemory.Follow-ups, left out here: leaving out the edges without the key of an order which a vertex-centric index sorts by bounding the slice, rather than in memory, would save the 7–10% above; a missing value of a sort key is written with a -1 flag byte, so such edges sit at the end of an ascending slice and at the start of a descending one, and strings need care. Merged with Apply the limits, ranges and orders of or() branches to each element #5106,
foldInOrderis only called with no containers of a branch, so itslocalOrderBypath is dead code, and so is the check of branch orders invalidFoldInHasContainerfor the folding ofor()steps, which that PR does byvalidFoldInOrBranch. The rebase onto it removes that check and its helpers, and the changelog's item on suchor()steps, which no longer holds once both are merged, as that PR folds a branch's order in as ahas(key); thelocalOrderBypath can go in a follow-up.QueryProcessorand Find the vertices a transaction changed for its graph queries by key and value #5023's bound are unchanged: a query which keeps the elements without a value, as a query of the API does, places them last, which the bound's range leaves out; in a traversal's query the condition leaves them out before.🤖 Generated with Claude Code