You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #3140 introduced Server-side input-ID ordering for HStore query results. Issue #3180 later exposed an interaction between that ordering path and flattened index queries: InputOrderIterator can probe the next query while detecting the current query boundary, and QueryList can update the shared origin ConditionQuery resultsFilter during that probe.
PR #3182, reviewed at head acbee16, is intended as a bounded correctness hotfix. It applies index-candidate filtering before input-order restoration and protects cache paths that could otherwise bypass that filtering. This fixes the reported failure without redesigning the iterator boundary itself.
A follow-up refactor is needed so correctness no longer depends on the buffering and side effects of ordinary Iterator.hasNext() calls.
Current problems
1. Boundary detection can activate the next query
InputOrderIterator.fetchBatch() currently uses the following sequence:
longqueryVersion = this.queryResults.queryVersion();
List<Query> queries = this.queryResults.currentQueries();
do {
results.add(this.origin.next());
} while (this.origin.hasNext() &&
queryVersion == this.queryResults.queryVersion());
The origin.hasNext() call is not a read-only boundary check. It can advance FlatMapperIterator or QueryTrackingIterator into the next flattened query. QueryList.IndexQuery may then update the shared origin query's resultsFilter before InputOrderIterator observes that queryVersion changed.
PR #3182 moves filtering into a safer position, but the underlying contract remains implicit: the current element must be filtered and buffered before another hasNext() call can cross the query boundary.
2. Input-order restoration is installed from the first active segment
QueryResults.keepInputOrderIfNeeded() decides whether to create InputOrderIterator from currentQueries after the first successful origin.hasNext() call.
This can miss a later segment whose ordering requirement differs from the first segment:
Input-order requirements must therefore be evaluated per query batch, not once for the flattened result stream.
3. Query context is represented by mutable stream-wide state
queryVersion, currentQueries, resultsFilter, input IDs, and mustSortByInput collectively describe the active query segment, but they are stored or observed through different objects. Mapper, filter, flat-map, paging, and cache paths do not share one explicit boundary contract.
This makes it difficult to prove that each result is parsed, TTL-checked, condition-filtered, and ordered with the context that produced it.
4. Filtering stages have implicit and asymmetric ownership
PR #3182 separates index-candidate matching from invalid-record filtering, but the ownership of diagnostics and cleanup remains easy to misread.
For ordinary queries, filterUnmatchedRecords() deliberately lets hidden records and records with deleting labels pass through without running rightResultFromIndexQuery(). The downstream filterInvalidRecords() then calls warnLeftRecord() and applies visibility rules. An undefined label is named ~undefined and is therefore hidden, so with the default showHidden=false the later warning inside filterUnmatchedRecords() is unreachable. It is reachable only for callers that enable showHidden.
Internal callers may use filterUnmatchedRecords() without the downstream invalid-record filter. Their access, warning, and left-index cleanup behavior therefore differs from the public query path by design.
The refactor must assign one explicit owner to each operation and preserve their order:
residual index-condition matching;
hidden, deleting-label, and undefined-label visibility;
left-record diagnostics;
asynchronous left-index cleanup.
A query-boundary change must not silently suppress, duplicate, or move these side effects to internal queries that previously bypassed them.
5. Cache eligibility depends on when optimization metadata becomes final
The vertex and edge cache paths currently evaluate queryNeedsPostFilter() at different points.
The vertex path checks before the backend call. This is valid because optimized vertex fallbacks arrive as an IdQuery whose origin ConditionQuery already carries PRIMARY_KEY or INDEX, and querying that IdQuery does not run optimizeQueries again.
The edge path must check both before and after super.queryEdgesFromBackend(query), because backend processing can still promote optimized() through the origin-query chain. Caching the result after that promotion could otherwise bypass required condition filtering.
This timing contract is live behavior, not a defensive implementation detail. The new batch model must either remove the timing asymmetry by carrying finalized cache/filter metadata or represent the two decision points explicitly and test them.
Goal
Introduce an explicit query-batch or query-segment boundary for query result iteration.
The implementation does not have to use a class named QueryBatch, but it must establish these invariants:
A batch carries the query metadata required to process its results, including its query or query snapshot, input IDs, input-order requirement, and filter context.
Each result is processed with the context of the batch that produced it.
Parse, TTL filtering, condition filtering, and input-order restoration complete at a batch-aware boundary before that context is discarded.
Checking whether the current batch has more results does not activate the next query or fetch the next backend page.
Mixed batches can independently choose whether input-order restoration is required.
Ordinary Iterator.hasNext() buffering behavior is not part of the correctness contract.
Suggested direction
Prefer an explicit batch abstraction exposed by QueryResults and produced by QueryList/PageEntryIterator. InputOrderIterator should consume one batch directly instead of inferring boundaries by comparing queryVersion after calling origin.hasNext().
A smaller boundary-aware iterator API, such as hasNextInCurrentQuery() plus an explicit operation that advances to the next query, may also be considered. However, the boundary must propagate through QueryTrackingIterator, MapperIterator, FilterIterator, FlatMapperIterator, page iteration, and any other wrapper that can advance the origin. Adding such a method only to InputOrderIterator would leave the same implicit coupling elsewhere.
Scope
The expected implementation area includes:
QueryResults and InputOrderIterator
QueryList and QueryTrackingIterator
PageEntryIterator and BatchIdHolder paths
Mapper, filter, and flat-map wrappers that can cross a query boundary
GraphTransaction processing order
Cache behavior when a query still requires condition filtering
After the explicit boundary is in place, reassess whether queryVersion/currentQueries and the post-filter cache guards introduced by #3182 can be removed or simplified. Removal is not a requirement unless equivalent correctness and cache behavior are demonstrated.
Do not change the public Gremlin, REST, or graph-query result semantics.
Do not introduce a request-level fallback between old and new query execution paths.
Do not combine unrelated cache-policy or index-planner changes into this refactor.
Acceptance criteria
Add focused RED tests before replacing the current boundary behavior.
Two flattened queries with different filters retain the correct results for both batches; activating query B cannot cause an element from query A to be evaluated with filter B.
Mixed ordering requirements work in both directions, including a first batch with mustSortByInput=false followed by a batch with mustSortByInput=true.
Empty intermediate batches do not leak filter or ordering state into adjacent batches.
Mapper results that are null or expand one backend entry into multiple elements preserve the originating batch context.
Multiple BatchIdHolder batches are processed independently.
The final element of a batch still receives TTL and condition filtering before the next batch becomes active.
Exhausting or inspecting the current batch does not fetch or prepare the next backend page; tests assert backend/page fetch count, not only consumed element count.
Warm-cache PRIMARY_KEY queries, PRIMARY_KEY queries with residual conditions, and vertex/edge INDEX + LABEL queries remain correct.
Residual-condition matching, invalid-record visibility, left-record diagnostics, and asynchronous left-index cleanup each have an explicit owner. Ordinary, internal, showHidden, and showDeleting query paths preserve their current warning, visibility, and cleanup behavior without duplicate side effects.
Vertex and edge cache eligibility remains correct across query optimization. Tests cover the vertex pre-backend decision and the edge post-backend re-check, or prove that finalized batch metadata makes the asymmetry unnecessary.
Close and exception propagation still release the active and remaining iterator/page resources exactly once.
Background
PR #3140 introduced Server-side input-ID ordering for HStore query results. Issue #3180 later exposed an interaction between that ordering path and flattened index queries: InputOrderIterator can probe the next query while detecting the current query boundary, and QueryList can update the shared origin ConditionQuery resultsFilter during that probe.
PR #3182, reviewed at head acbee16, is intended as a bounded correctness hotfix. It applies index-candidate filtering before input-order restoration and protects cache paths that could otherwise bypass that filtering. This fixes the reported failure without redesigning the iterator boundary itself.
A follow-up refactor is needed so correctness no longer depends on the buffering and side effects of ordinary Iterator.hasNext() calls.
Current problems
1. Boundary detection can activate the next query
InputOrderIterator.fetchBatch() currently uses the following sequence:
The origin.hasNext() call is not a read-only boundary check. It can advance FlatMapperIterator or QueryTrackingIterator into the next flattened query. QueryList.IndexQuery may then update the shared origin query's resultsFilter before InputOrderIterator observes that queryVersion changed.
PR #3182 moves filtering into a safer position, but the underlying contract remains implicit: the current element must be filtered and buffered before another hasNext() call can cross the query boundary.
2. Input-order restoration is installed from the first active segment
QueryResults.keepInputOrderIfNeeded() decides whether to create InputOrderIterator from currentQueries after the first successful origin.hasNext() call.
This can miss a later segment whose ordering requirement differs from the first segment:
Input-order requirements must therefore be evaluated per query batch, not once for the flattened result stream.
3. Query context is represented by mutable stream-wide state
queryVersion, currentQueries, resultsFilter, input IDs, and mustSortByInput collectively describe the active query segment, but they are stored or observed through different objects. Mapper, filter, flat-map, paging, and cache paths do not share one explicit boundary contract.
This makes it difficult to prove that each result is parsed, TTL-checked, condition-filtered, and ordered with the context that produced it.
4. Filtering stages have implicit and asymmetric ownership
PR #3182 separates index-candidate matching from invalid-record filtering, but the ownership of diagnostics and cleanup remains easy to misread.
For ordinary queries, filterUnmatchedRecords() deliberately lets hidden records and records with deleting labels pass through without running rightResultFromIndexQuery(). The downstream filterInvalidRecords() then calls warnLeftRecord() and applies visibility rules. An undefined label is named ~undefined and is therefore hidden, so with the default showHidden=false the later warning inside filterUnmatchedRecords() is unreachable. It is reachable only for callers that enable showHidden.
Internal callers may use filterUnmatchedRecords() without the downstream invalid-record filter. Their access, warning, and left-index cleanup behavior therefore differs from the public query path by design.
The refactor must assign one explicit owner to each operation and preserve their order:
A query-boundary change must not silently suppress, duplicate, or move these side effects to internal queries that previously bypassed them.
5. Cache eligibility depends on when optimization metadata becomes final
The vertex and edge cache paths currently evaluate queryNeedsPostFilter() at different points.
The vertex path checks before the backend call. This is valid because optimized vertex fallbacks arrive as an IdQuery whose origin ConditionQuery already carries PRIMARY_KEY or INDEX, and querying that IdQuery does not run optimizeQueries again.
The edge path must check both before and after super.queryEdgesFromBackend(query), because backend processing can still promote optimized() through the origin-query chain. Caching the result after that promotion could otherwise bypass required condition filtering.
This timing contract is live behavior, not a defensive implementation detail. The new batch model must either remove the timing asymmetry by carrying finalized cache/filter metadata or represent the two decision points explicitly and test them.
Goal
Introduce an explicit query-batch or query-segment boundary for query result iteration.
The implementation does not have to use a class named QueryBatch, but it must establish these invariants:
Suggested direction
Prefer an explicit batch abstraction exposed by QueryResults and produced by QueryList/PageEntryIterator. InputOrderIterator should consume one batch directly instead of inferring boundaries by comparing queryVersion after calling origin.hasNext().
A smaller boundary-aware iterator API, such as hasNextInCurrentQuery() plus an explicit operation that advances to the next query, may also be considered. However, the boundary must propagate through QueryTrackingIterator, MapperIterator, FilterIterator, FlatMapperIterator, page iteration, and any other wrapper that can advance the origin. Adding such a method only to InputOrderIterator would leave the same implicit coupling elsewhere.
Scope
The expected implementation area includes:
After the explicit boundary is in place, reassess whether queryVersion/currentQueries and the post-filter cache guards introduced by #3182 can be removed or simplified. Removal is not a requirement unless equivalent correctness and cache behavior are demonstrated.
Non-goals
Acceptance criteria
Add focused RED tests before replacing the current boundary behavior.
References
I will continue to follow up on this issue.