Test index intersection membership against a Set (#4932)

[batrived]

Fixes #4932.

SubqueryIterator streams the results of the first index and keeps the elements the other indexes also returned. Membership was tested with List.contains inside the filter, so every streamed element scanned the other list from the start, giving O(n·m).

This matters more than a typical contains-on-a-list nit because the list is deliberately unbounded. StandardJanusGraphTx passes Query.NO_LIMIT to processIntersectingRetrievals, with a comment explaining that this prevents incomplete intersections and therefore missed results. That makes subLimit Integer.MAX_VALUE, so the whole matching set of every other index is materialised, and the linear scan runs against all of it.

Building the set once outside the lambda makes the intersection O(n + m). This is the fix suggested in the issue.

Testing

The change is a performance fix with no intended behaviour change, so SubqueryIteratorTest pins the behaviour that the conversion must preserve rather than trying to measure time: only intersected ids survive; null (the single-index case) is not treated as an empty intersection; an empty intersection yields nothing; the limit still truncates; a repeated id in the other results is harmless; and the streamed order is preserved regardless of the order the other indexes reported.

That last group is worth having because a List and a HashSet differ in more than lookup cost — the null case and the ordering are exactly what a careless conversion would break.

One note on the trade-off

The set is a second structure over ids that are already materialised, so peak memory for that data roughly doubles while the iterator is open. It stays the same order as the list that already exists, and it replaces a quadratic scan, so I think it is clearly worth it.

If you would rather not hold both, the alternative is to have processIntersectingRetrievals return a LinkedHashSet instead of an ArrayList — it already builds a HashSet internally for each intersection step, and it has exactly one caller, this one. I did not do that here because it would change the meaning of the results.size() < limit loop condition when a sub-result contains duplicates, which deserves its own change rather than riding along with this one.

---

For all changes:

• Is there an issue associated with this PR? Is it referenced in the commit message?
• Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
• Has your PR been rebased against the latest commit within the target branch (typically master)?
• Is your initial contribution a single, squashed commit?

For code changes:

• Have you written and/or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0? — no new dependencies
• If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository? — not applicable
• If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository? — not applicable

[Copilot]

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.

Improves performance of index-intersection filtering in SubqueryIterator by avoiding repeated List.contains scans, and adds a focused unit test suite to pin intersection semantics.

Changes:

• Replace per-element List.contains membership checks with a precomputed HashSet lookup.
• Add SubqueryIteratorTest to verify intersection behavior (null/empty cases, limits, duplicates, and ordering).

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	Precomputes a Set for intersection membership to reduce intersection cost from O(n·m) to O(n+m).
janusgraph-core/src/test/java/org/janusgraph/graphdb/util/SubqueryIteratorTest.java	Adds unit tests covering semantics that could be affected by switching from list membership to set membership.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[porunov]

Applied — it is thenAnswer(invocation -> firstIndexResults.stream()) now, so each call gets a fresh stream.

[porunov]

Worth checking, but the requirement is not new here. otherResults is what QueryUtil.processIntersectingRetrievals returns, and for every index after the first it narrows the result with results.removeIf(o -> !subResultSet.contains(o)) against new HashSet<>(subResult). So membership among these ids is already hash based one call earlier. I checked the id types that actually reach this filter as well — Long, String for custom vertex ids, and RelationIdentifier, whose hashCode is Long.hashCode(relationId) while equals compares relationId and typeId, so equal instances always hash equal.

I have recorded the first half of that in the comment above the conversion so the next reader does not have to re-derive it.

[porunov]

No change needed — HashSet(Collection) already does exactly this. From the JDK 11 source:

public HashSet(Collection<? extends E> c) {
    map = new HashMap<>(Math.max((int) (c.size()/.75f) + 1, 16));
    addAll(c);
}

That is otherResults.size() against the 0.75 load factor, which is the sizing you are asking for, so there is no rehashing to avoid.

[porunov]

Reviewed, rebased on current master (no conflicts) and force-pushed as a single commit. One of Copilot's three comments is applied; I replied on the other two, both of which turned out not to need a change.

What I verified

The reasoning in the issue and the PR body holds up end to end:

• Query.NO_LIMIT is Integer.MAX_VALUE, and in processIntersectingRetrievals the guard if (Integer.MAX_VALUE / multiplier >= limit) subLimit = limit * multiplier; leaves subLimit at Integer.MAX_VALUE either way. So the other indexes really are materialised in full, and the quadratic factor really does run against all of it.
• StandardJanusGraphTx still passes NO_LIMIT on master, with the comment explaining that an incomplete intersection would miss results — so this is not a list that can be bounded away instead.
• processIntersectingRetrievals already narrows with new HashSet<>(subResult) for every index after the first, so the conversion is consistent with the surrounding code rather than introducing a new assumption.

On the tests

They pin behaviour rather than timing, which is the right call for this change, and they are not vacuous. I checked by making the conversion carelessly — treating a null otherResults as an empty set, which is the obvious way to get this wrong — and shouldKeepEveryIdWhenThereIsNoOtherIndex fails with expected: <[1, 2]> but was: <[]>. That is a silent wrong-results bug for every single-index query, so it is worth having pinned.

On the trade-off you raised

Doubling the peak memory of ids that are already materialised, in exchange for removing a quadratic scan over an intentionally unbounded list, is clearly the right side of that trade. I also agree with not folding in the LinkedHashSet return-type change: it changes what results.size() < limit means in the retry loop when a sub-result contains duplicates, and that deserves its own change with its own reasoning.

Verification

SubqueryIteratorTest 6/6 and BerkeleyLuceneTest — the multi-index query suite that actually exercises this intersection path — green on the rebased branch.