fix(server): make query result batch boundaries explicit - #3193
fix(server): make query result batch boundaries explicit#3193contrueCT wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3193 +/- ##
============================================
+ Coverage 37.77% 37.96% +0.18%
- Complexity 6560 6619 +59
============================================
Files 800 802 +2
Lines 68960 69116 +156
Branches 9166 9193 +27
============================================
+ Hits 26052 26241 +189
+ Misses 39841 39815 -26
+ Partials 3067 3060 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
There was a problem hiding this comment.
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.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The batch rewrite holds up at this head. BatchIterator close and suppression semantics, prefetch preservation across backendBatches into mapBatches, null mapper returns, filter ordering, root-of-chain visibility flags and paging metadata propagation all check out, and the previous review's Invalid PageState 'null' blocker is fixed by the query.paging() bypass. Two minor points below: the vertex and edge caches quietly lost their queryNeedsPostFilter exclusion, and PageResults.query()/get() are now dead code. Neither blocks.
Evidence: static reading of the exact-head diff plus the surrounding unchanged code, and a local build of 05186a0 (hugegraph-struct plus hugegraph-server/{hugegraph-core,hugegraph-api,hugegraph-test}) where the seven selected unit classes give Tests run: 78, Failures: 0, Errors: 0, matching the PR body. Backend suites were not re-run here; InMemoryDBStore.supportsQueryByPage() is false, so a local unit run cannot reach the paging and cache paths this PR reworks.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The batch rework holds up at this head, and four of my earlier points are addressed: the vertex cache now has the paging() bypass (CachedGraphTransaction.java:322), PageResults.emptyIterator() returns a fresh instance (QueryList.java:362-365), registerPrivateActions() matches the renamed members again, and the dead PageResults.query()/get() accessors are gone. My threads on the shared edgesCache monitor (:412) and the dropped queryNeedsPostFilter exclusion (:372) are still open. New here: the iterator plumbing allocates per surviving element at every stage, the TTL stage is now unconditional, multi-branch edge queries lost their eager validation, and the edge cache copies its group index on reads. Evidence: exact-head diff for cf40300 against origin/master, plus Query.toString():610, CoreOptions.QUERY_BATCH_SIZE default 1000, style/checkstyle.xml:23,59. All 24 check runs on cf40300 are green.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The batch rework holds at this head, and 9ad4a27e addresses all seven points from the cf40300 round: map allocation, the TTL short-circuit, the branch-validation note plus testLaterBranchFailureClosesEarlierBranch, the read-path copy, the batch-key size, the unused imports and reorders in GraphTransaction.java, and the offset message. Two caveats. First, only half of the base TTL short-circuit came back: base filterExpiredResultFromBackend returned early on supportsTtl() || query.showExpired(), while GraphTransaction.java:1982-1992 returns early on supportsTtl() alone and still builds a filter stage to evaluate context.showExpired() per element. That is fine given the per-batch context, but the wrapper is not gone for a showExpired query on a non-TTL store. Second, the backend suites were not re-run here. Three minor points below; none blocks. My earlier threads on the shared edgesCache monitor (:414) and the dropped queryNeedsPostFilter exclusion (:372) are unchanged at this head.
What I checked and found sound: the compact "" batch label cannot collide, because Query.toString():588 always starts with `Query and IdPrefixQuery/IdRangeQuery build on super.toString(), so no QueryId.asString() is empty. Off-heap storage of the nested List<Object> round-trips: OffheapCache.ValueType.valueOf maps String to STRING(DataType.TEXT) and serializeList/deserializeList recurse (OffheapCache.java:281-297). The ExtendableIterator(candidates, source) wrapper at CachedGraphTransaction.java:409-410 does not strand the backend cursor: hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/iterator/ExtendableIterator.java:69-90 closes every iterator still in itors, with suppression.
Evidence: static reading of the exact-head diff against 36811483a plus the surrounding unchanged code. Executed: a local JDK 11 build of hugegraph-struct and hugegraph-server/{hugegraph-core,hugegraph-api,hugegraph-test} at 9ad4a27e running the seven selected unit classes gives Tests run: 80, Failures: 0, Errors: 0, matching the PR body. InMemoryDBStore.supportsQueryByPage() is false (InMemoryDBStore.java:442-444), so a local unit run cannot reach the paging paths this PR reworks. All 24 check runs on 9ad4a27e are green.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: At 00e365d the batch rework holds, and the three points left from the last round are addressed: the unused QueryResults.queries() diagnostics are removed, the leaf-level paging() test in fetchVertexBatch is explained in a comment, and the QueryList import order is restored. All earlier threads are resolved. Batch close and suppression semantics, the shared cursor, raw-empty versus filtered-empty pages, copy-on-write edge-cache groups, per-batch visibility and TTL context, and the sandbox method filter all check out. Evidence: static review of the full diff against 3681148 and the unchanged code around it. The seven focused unit classes (QueryResultsTest, QueryListTest, GraphTransactionTest, CachedGraphTransactionTest, IdHolderTest, QueryTest, PageStateTest) give Tests run: 80, Failures: 0, Errors: 0 on JDK 11 at 00e365d. All 24 checks at this head are green.
Purpose of the PR
Closes #3190.
QueryResultscurrently detects a query boundary by probinghasNext()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
QueryBatchandQueryResultContextto carry the input IDs, ordering decision, visibility flags and effective results filter with the results that produced them.QueryResults,QueryListandPageEntryIteratorcompose explicit batches. RemovequeryVersion/currentQueriesboundary inference and shared results-filter propagation.QueryResults.queries()diagnostics list; query identity remains in each batch context.PageState. Give each empty page an independent batch cursor, preventing one consumer from exhausting another consumer's cursor.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_KEYand physical-key cursors are outside this change.Multi-branch edge queries activate branches lazily. Errors raised while preparing a branch (including query optimization and index preparation) or starting its backend read can therefore surface from
hasNext()/next()rather than the initial query call; earlier branches may already have yielded results. Validation reached while flattening the request still runs immediately, and the single-branch path retains immediate branch activation. This avoids preparing sibling branches before the current batch has finished.Verifying these changes
VertexCoreTest,EdgeCoreTestand cache regressions.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 afterhasNext()and duplicate/accumulating query diagnostics; the shared cursor fixed those regressions, and the unused diagnostics API has since been removed. The paging regression traverses 10,000 realPageEntryIteratorpages and checks each batch context, results and page activation count.Verification on JDK 17 and Maven 3.9.16:
00e365db9ad4a27ecf40300205186a0eQueryListTest17788a4a17788a4aVertexCoreTest,EdgeCoreTestc694bb0aVertexCoreTest,EdgeCoreTestc694bb0ac694bb0aBoth new review regressions failed on
c694bb0a: primary-key paging raisedInvalid 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 baseline36811483: three tests pass, whiletestQueryOutEdgesOfVertexBySortkeyWithMoreFieldsInPagefails in its body withCardinality from code 0. This existing HStore serialization error remains unresolved; the 26-test selection above is not a full HStore-suite pass.Reproduction commands
Formatting and full reactor compilation passed at
9ad4a27eand again at00e365db. The diagnostics cleanup also passed all 80 selected tests, including the revised batch-context and 10,000-page activation checks. The two additional tests cover deferred branch failure/cleanup in the shared flat-map iterator and compact root cache labels coexisting with distinct batches without mutating previously published values. Existing off-heap, lifecycle and cache-limit regressions pass in the 80-test selection. No throughput benchmark or backend-suite rerun was performed for this allocation/cache follow-up. Verification used-Drat.skip=true; the complete UnitTestSuite, API and TinkerPop suites were not run. The 78-test unit verification used Java files matching05186a0ebyte-for-byte. The accessor/comment cleanup atcf403002passed formatting, full reactor compilation and the 37 targeted tests listed above. 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?
Documentation Status
Doc - TODODoc - DoneDoc - No Need