Cache a subquery result set only when it is complete (#4931) - #4941
Open
batrived wants to merge 2 commits into
Open
Cache a subquery result set only when it is complete (#4931)#4941batrived wants to merge 2 commits into
batrived wants to merge 2 commits into
Conversation
SubqueryIterator cached its collected ids on close whenever the element iterator had no further elements. That is true both when the index ran out of results and when the limit stopped the stream, so a truncated prefix of the subquery results was stored as if it were the whole answer. The limit is not part of the cache key of such a subquery. JointIndexQuery propagates a new limit to its subqueries only when it holds a single one, so for a joint query the first subquery keeps the NO_LIMIT it was built with, and two graph queries which differ only in their limit produce the same key. A later query in the same transaction asking for more results was therefore served the shorter list. Count the elements the limit let through, and cache only when fewer than the limit were emitted, which means the limit never stopped anything. A limit equal to the number of results is not cached either, because the index running out and the limit being reached cannot be told apart at that point. That costs a repeated index call, where caching a possibly short result set costs missing results. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mad
enabled auto-merge
August 12, 2026 04:52
mad
disabled auto-merge
August 12, 2026 04:52
…ead limit The previous commit declined to cache any result set which the read limit truncated. That is wider than necessary. SubqueryCache stores a result list against the limit of the subquery which produced it, and serves that list only to a later query whose limit is no larger, so a truncated list is safe to store while the subquery carries the limit which truncated it. JointIndexQuery.updateLimit propagates the joint limit into the subquery only when there is a single subquery, so that condition holds there. It does not hold once a joint query has more than one subquery: the cache then records the wider subquery limit for a list read under the narrower joint limit, and serves too few results to a later query whose limit falls between the two. That is the case worth declining. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
batrived
force-pushed
the
fix/4931-subquery-cache-truncated
branch
from
August 12, 2026 17:04
4aba6b4 to
d58379e
Compare
There was a problem hiding this comment.
Pull request overview
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.
Fixes incorrect subquery caching when a joint query’s stream is short-circuited by limit(), preventing a truncated prefix from being cached under a key that can later be reused for a larger limit.
Changes:
- Track how many elements are actually emitted by the limited stream and gate caching on whether the cached list is “safe” for future queries.
- Add unit tests covering truncated vs exhausted reads, no-limit behavior, and empty result set caching.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| janusgraph-core/src/main/java/org/janusgraph/graphdb/util/SubqueryIterator.java | Adds emitted-count tracking and a safety predicate to avoid caching truncated prefixes under wider subquery limits. |
| janusgraph-core/src/test/java/org/janusgraph/graphdb/util/SubqueryIteratorCacheTest.java | Introduces tests to validate caching behavior across limit/exhaustion scenarios and empty results. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+104
to
+110
| @Test | ||
| public void shouldCacheAResultSetWhoseIndexRanOutAsTheLimitWasReached() { | ||
| //The index ran out at the same moment the limit was reached. Which of the two stopped the read is unknown, so | ||
| //the list counts as truncated, and the subquery carrying the same limit is what makes it safe to store | ||
| runQuery(ALL_MATCHING_IDS.size(), ALL_MATCHING_IDS.size()); | ||
| assertEquals(ALL_MATCHING_IDS, captureCachedResult()); | ||
| } |
|
|
||
| @Test | ||
| public void shouldCacheAnEmptyResultSet() { | ||
| when(subQuery.getProfiler()).thenReturn(QueryProfiler.NO_OP); |
Comment on lines
+70
to
+71
| private List<Object> captureCachedResult() { | ||
| final ArgumentCaptor<List<Object>> cached = ArgumentCaptor.forClass(List.class); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4931.
SubqueryIterator.closecached its collected ids whenever the element iterator had no further elements. That is true both when the index ran out of results and when.limit(limit)short-circuited the stream, so a truncated prefix of the subquery results was stored as if it were the whole answer.That prefix is then served under a key which does not distinguish it.
JointIndexQuery.updateLimitpropagates a new limit to its subqueries only when it holds exactly one, so for a joint query the first subquery keeps theNO_LIMITit was constructed with, andIndexQuery.equals/hashCodetherefore produce the same key for two graph queries differing only in their limit. The result is the reproduction from the issue:limit(10)thenlimit(1000)in the same transaction returns roughly 10.Change
This is the first of the three options in the issue, which you noted is the least invasive. The stream is the only thing that can short-circuit, so the number of elements the limit let through tells us what happened: fewer than the limit means the limit never stopped anything and the index was read to the end.
I did not touch
updateLimit. Propagating the limit to subquery 0 would make the key correct, but it would also change what the first index is asked for, which affects the intersection —StandardJanusGraphTxdeliberately passesNO_LIMITtoprocessIntersectingRetrievalsto keep intersections complete, so limiting subquery 0 needs its own reasoning.One deliberate trade-off
A limit exactly equal to the number of results is also not cached. At that moment the index running out and the limit being reached are indistinguishable without pulling one more element. Declining to cache costs a repeated index call on a later identical query; caching a set that may be short costs silently missing results. I chose the former, and
shouldNotCacheWhenTheLimitIsExactlyTheNumberOfResultspins that choice so it is visible rather than accidental.If you would rather keep the caching in that case, the alternative is to probe the source iterator for one more element before deciding, which means holding it separately from the mapped stream.
Testing
SubqueryIteratorCacheTestcovers the truncated case, the exhausted case, the no-limit case, the exactly-at-limit case, and an empty result set, which is a complete answer and should still be cached. I confirmed the two "should not cache" tests fail againstmasterwithout the change and pass with it.Note on overlap
This touches the same stream chain in
SubqueryIteratoras my PR for #4932, so whichever merges second will need a trivial rebase. They are independent changes: #4932 changes the intersection filter, this one adds the emitted-count and the caching condition.For all changes:
master)?For code changes: