Skip to content

fix(server): make query result batch boundaries explicit - #3193

Open
contrueCT wants to merge 5 commits into
apache:masterfrom
contrueCT:task/issue-3190-query-batch-boundaries
Open

fix(server): make query result batch boundaries explicit#3193
contrueCT wants to merge 5 commits into
apache:masterfrom
contrueCT:task/issue-3190-query-batch-boundaries

Conversation

@contrueCT

@contrueCT contrueCT commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Purpose of the PR

Closes #3190.

QueryResults currently detects a query boundary by probing hasNext() and then checking whether the active query changed. That probe can activate the next index query or backend page before the current batch finishes processing. Ordering is also selected from the first active segment, so an unsorted batch [1] followed by a batch with input IDs [3, 2] can incorrectly produce [1, 2, 3].

This change gives each batch its own results and captured processing context. Parsing, TTL checks, residual filtering and input-order restoration finish within that batch before the caller consumes a flattened stream.

Main Changes

  • Introduce QueryBatch and QueryResultContext to carry the input IDs, ordering decision, visibility flags and effective results filter with the results that produced them.
  • Make QueryResults, QueryList and PageEntryIterator compose explicit batches. Remove queryVersion/currentQueries boundary inference and shared results-filter propagation.
  • Apply vertex and edge processing within each batch, preserving public/internal visibility, diagnostics and index-cleanup ownership. Keep native scans on their original single-batch path so backend page tokens and capacity checks remain intact.
  • Cache candidates before residual matching and reapply the current batch context on hits. Preserve mixed hit/miss input ordering, the original edge-cache request key, off-heap serialization, TASK/SERVER query types and the RamTable fast path.
  • Share the remaining batch cursor between element iteration, mapping and materialization, preserving prefetched results and page metadata. Keep queries() diagnostics bounded to the current batch and clear them on close.
  • Preserve the existing holder boundary when a backend page has no raw records. Keep following the cursor when an existing batch is emptied by parsing or TTL filtering, and retain negative edge-cache entries. This avoids scanning stale label-index pages during HStore schema cleanup.
  • Bypass the vertex cache for paged backend queries so primary-key optimization retains PageState. Give each empty page an independent batch cursor, preventing one consumer from exhausting another consumer's cursor.
  • Synchronize reflection filtering with the renamed transaction helpers and the cache overrides. Add lifecycle, edge-cache boundary, and index-cleanup side-effect regressions through the actual query-processing stages.
  • Make batch wrappers close idempotently and preserve the primary exception when cleanup also fails. Native iterators that close themselves still rely on their existing backend close contract.

Explicit query batch boundaries: finish processing A with its own filter and ordering context before activating B.

The flattened consumer can advance after A is exhausted; an iterator operating inside A never probes B to discover A's boundary. HStore partition merging, ORDER_BY_KEY and physical-key cursors are outside this change.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, including VertexCoreTest, EdgeCoreTest and cache regressions.
  • Need tests and can be verified as follows:

Three focused tests first failed on the old implementation: mixed ordering returned [1, 2, 3] instead of [1, 3, 2], and the next-query and next-page fetch counters reached 2 when only 1 batch should have been activated. All now pass. Additional regressions fail on the previous PR head for materialization/mapping after hasNext() and duplicate/accumulating query diagnostics; they pass with the shared cursor. The paging regression traverses 10,000 real PageEntryIterator pages and checks that only the current query is retained.

Verification on JDK 17 and Maven 3.9.16:

Scope Snapshot Selected Skipped Failures / errors
Seven focused unit-test classes 05186a0e 78 0 0 / 0
RocksDB: primary-key/paging cases and QueryListTest 17788a4a 23 0 0 / 0
HStore: primary-key paging with cold/warm cache, filtered-empty and missing results 17788a4a 1 0 0 / 0
Memory: VertexCoreTest, EdgeCoreTest c694bb0a 436 69 0 / 0
RocksDB: VertexCoreTest, EdgeCoreTest c694bb0a 436 29 0 / 0
HStore: search/joint-index, input-order and paging regressions c694bb0a 26 0 0 / 0

Both new review regressions failed on c694bb0a: primary-key paging raised Invalid PageState 'null', and interleaved empty-page consumption exposed the shared cursor. They pass with this follow-up. The primary-key test covers cold and warm caches, residual filtering to an empty result, a missing primary key, and terminal page metadata.

The latest supplement adds 10 tests. Seven isolated fault-injection variants are rejected by the new tests: omitted holder/source closing, a changed cache capacity boundary, a discarded oversized-batch tail, and incorrect cleanup decisions for invisible, mismatching, or matching-with-stale-index records. The unchanged implementation passes all selected diagnostic tests. Reflection-filter entries were checked against 15 actual declared methods across the transaction and cache classes.

Coverage includes raw-empty backend pages versus filtered-empty batches, missing and expired vertices through cache materialization, empty batches/holders, null and expanded mapper results, mixed ordering in both directions, real page-fetch counters and cursor metadata, limits, cross-batch materialization capacity, materialization after partial consumption, page metadata after closing the source, early close and suppressed exceptions, residual filtering on warm vertex caches, and an off-heap edge-cache hit with zero backend reads. The HStore selection also passed on the pre-change baseline using the same isolated PD/Store setup.

At c694bb0a, a separate four-method HStore reproduction exercises schema cleanup after creating 10,000 edges per label. Before the empty-page fix, three subsequent tests timed out in setup. With the fix, cleanup completes and the sequence matches baseline 36811483: three tests pass, while testQueryOutEdgesOfVertexBySortkeyWithMoreFieldsInPage fails in its body with Cardinality from code 0. This existing HStore serialization error remains unresolved; the 26-test selection above is not a full HStore-suite pass.

Reproduction commands
mvn test -pl hugegraph-server/hugegraph-test -am -P unit-test \
  -Dtest=QueryResultsTest,QueryListTest,GraphTransactionTest,CachedGraphTransactionTest,IdHolderTest,QueryTest,PageStateTest \
  -DfailIfNoTests=false -Drat.skip=true

# Run once with memory, then with rocksdb
mvn test -pl hugegraph-server/hugegraph-test -am -P core-test,memory \
  -Dtest=VertexCoreTest,EdgeCoreTest -DfailIfNoTests=false -Drat.skip=true

# Requires an isolated, initialized PD/Store and an HStore properties file
mvn test -pl hugegraph-server/hugegraph-test -am -P core-test,hstore \
  -Dbackend=hstore -Dconfig_path=/path/to/hstore-test.properties \
  '-Dtest=VertexCoreTest#testQueryByJointIndexesWithSearch*+testQueryByTextContainsPropertyOrderByMatchedCount*+testQueryByRangeIndexKeeps*+testQueryByPage*+testQueryByMultiLabelInPage,EdgeCoreTest#testQueryEdgeByPage*+testQuery*EdgesOfVertexInPaging' \
  -DfailIfNoTests=false -Drat.skip=true

# Four-method HStore cleanup reproduction (baseline has one serialization error)
mvn test -pl hugegraph-server/hugegraph-test -am -P core-test,hstore \
  -Dbackend=hstore -Dconfig_path=/path/to/hstore-test.properties \
  '-Dtest=EdgeCoreTest#testQueryEdgesWithLimitOnSuperVertexAndFilterProp+testQueryByUnionHasDate+testQueryOutEdgesOfVertexBySortkeyWithMoreFieldsInPage+testUpdateEdgeProperty' \
  -DfailIfNoTests=false -Drat.skip=true

# Latest review follow-up on RocksDB
mvn test -pl hugegraph-server/hugegraph-test -am -P core-test,rocksdb \
  '-Dtest=QueryListTest,VertexCoreTest#testQueryByPrimaryValues*+testQueryByPage*+testQueryByMultiLabelInPage' \
  -DfailIfNoTests=false -Drat.skip=true

# Repeat the new primary-key case with the HStore configuration above:
# -Dtest=VertexCoreTest#testQueryByPrimaryValuesInPageWithVertexCache

mvn editorconfig:format
mvn clean compile -Dmaven.javadoc.skip=true -Drat.skip=true
git diff --check

Formatting and full reactor compilation passed. Verification used -Drat.skip=true; the complete UnitTestSuite, API and TinkerPop suites were not run. The latest unit verification used Java files matching 05186a0e byte-for-byte. The backend suites remain labeled with the snapshots actually tested and were not rerun for this test/auth-registration supplement.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API (Gremlin/REST interfaces)
  • Other affects (internal query iteration, paging and cache lifecycle)
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.76613% with 145 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.90%. Comparing base (3681148) to head (05186a0).

Files with missing lines Patch % Lines
...org/apache/hugegraph/backend/query/QueryBatch.java 58.18% 35 Missing and 11 partials ⚠️
...g/apache/hugegraph/backend/query/QueryResults.java 68.49% 44 Missing and 2 partials ⚠️
.../apache/hugegraph/backend/tx/GraphTransaction.java 64.78% 15 Missing and 10 partials ⚠️
...ugegraph/backend/cache/CachedGraphTransaction.java 76.38% 5 Missing and 12 partials ⚠️
...ache/hugegraph/backend/page/PageEntryIterator.java 79.16% 1 Missing and 4 partials ⚠️
...a/org/apache/hugegraph/backend/page/QueryList.java 89.28% 3 Missing ⚠️
...apache/hugegraph/backend/query/ConditionQuery.java 50.00% 1 Missing and 1 partial ⚠️
...he/hugegraph/backend/query/QueryResultContext.java 97.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3193      +/-   ##
============================================
+ Coverage     37.77%   37.90%   +0.12%     
- Complexity     6560     6601      +41     
============================================
  Files           800      802       +2     
  Lines         68960    69092     +132     
  Branches       9166     9186      +20     
============================================
+ Hits          26052    26191     +139     
- Misses        39841    39844       +3     
+ Partials       3067     3057      -10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@contrueCT
contrueCT marked this pull request as draft September 5, 2026 04:20
@contrueCT
contrueCT marked this pull request as ready for review September 5, 2026 07:42

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The batching design holds up. BatchIterator close and exception semantics, the shared cursor across mapBatches/flatMap, the raw-empty-page versus filtered-empty-batch distinction, and rebuilding the batch context on an edge-cache hit all check out. One blocker: moving the QueryList fetcher up from super::query to fetchVertexBatch puts the vertex cache inside the paging machinery, and the joined cache-plus-backend result it returns carries no page metadata, so a paged primary-key vertex query dies in PageInfo.pageState. Please add the paging() bypass plus a VertexCoreTest case for that shape; the other four comments are optional.

Evidence: the chain is traced through unmodified code and spelled out inline on CachedGraphTransaction.java:322, including why the existing green paging tests do not reach it. Static analysis only, not executed against a page-capable backend. CI at c694bb0 is green on all 24 checks.

@contrueCT
contrueCT marked this pull request as draft September 5, 2026 14:37
@contrueCT
contrueCT marked this pull request as ready for review September 5, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improve] Make query-result batch boundaries explicit

2 participants