Skip to content

[Improve] Make query-result batch boundaries explicit #3190

Description

@contrueCT

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:

long queryVersion = 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:

segment A: mustSortByInput = false, backend = [1]
segment B: mustSortByInput = true,  input = [3, 2], backend = [2, 3]

expected: [1, 3, 2]
possible current result: [1, 2, 3]

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.

Non-goals

  • Do not redesign the HStore partition merge, ORDER_BY_KEY protocol, or physical-key cursor introduced by fix(hstore): preserve range-index ordering across partitions #3140.
  • 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.
  • Existing fix(hstore): preserve range-index ordering across partitions #3140 range-index ordering, limit, offset, paging, and cursor behavior remains unchanged.
  • The vertex and edge regressions from [Bug] HStore combined range/search index test intermittently drops results #3180/fix(server): filter index results before input ordering #3182 pass on the applicable memory, RocksDB, and HStore test profiles.
  • 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.

References

I will continue to follow up on this issue.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions