Skip to content

fix(hstore): preserve range-index ordering across partitions - #3140

Merged
imbajin merged 8 commits into
apache:masterfrom
contrueCT:task/issue-3053-hstore-range-index-ordering
Aug 26, 2026
Merged

fix(hstore): preserve range-index ordering across partitions#3140
imbajin merged 8 commits into
apache:masterfrom
contrueCT:task/issue-3053-hstore-range-index-ordering

Conversation

@contrueCT

@contrueCT contrueCT commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Purpose of the PR

HStore range-index queries with a limit, offset, or page cursor require globally ordered backend keys. The previous multi-partition path exposed partition iteration order and an internal storage cursor, which could return the wrong limited slice or skip entries on continuation.

This PR adds an explicit ordered range-scan path while leaving unbounded count, joint-index, and cleanup scans on the existing path.

Typical Gremlin examples

These are the queries this change is meant to make predictable on HStore:

g.V().has('date', gte('2013')).limit(2)
g.V().has('age', gte(30)).range(0, 10)
g.V().has('score', gt(80)).limit(20)

Before this PR, the same logical query could return a slice in partition-local order. For example, when 2013 and 2014 live on different partitions, limit(2) could see 2014 before 2013.

After this PR, ordered range scans are merged by full key across partitions, so the client sees one global order end to end.

Main Changes

flowchart LR
    subgraph Store1["Store 1"]
        P1["Partition 1"] --> M1["Local K-way merge"]
        P2["Partition 2"] --> M1
    end
    subgraph Store2["Store 2"]
        P3["Partition 3"] --> M2["Local K-way merge"]
        P4["Partition 4"] --> M2
    end
    M1 --> G["Client global K-way merge"]
    M2 --> G
    G --> R["Ordered range-index results"]
Loading
  • Preserve the next unread physical index key as the HugeGraph page cursor and preserve ordered index IDs through the following element lookup.
  • Add ORDER_BY_KEY to the HStore scan protocol. Each Store merges its local leader-partition iterators, and the client globally merges one stream per Store instead of opening one stream per partition.
  • Use fixed 64-entry pages, request later pages only when a Store's current page is exhausted, and bound concurrent first-page initialization to eight streams.
  • Keep legacy scan ordering and cursor behavior unchanged for requests that do not explicitly select the ordered path. Client and Store nodes must be upgraded together before using ordered range scans.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • ClientSuiteTest: 13 tests passed.
    • ServerSuiteTest: 6 tests passed.
    • HstoreSessionsImplTest and HstoreTableTest: 8 tests passed.
    • GraphIndexTransactionTest, IdHolderTest, and QueryResultsTest: 4 tests passed.
    • git diff --check passed.

Does this PR potentially affect the following parts?

Documentation Status

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

@contrueCT
contrueCT marked this pull request as ready for review August 6, 2026 15:08
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. api Changes of API perf store Store module labels Aug 6, 2026

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The ordered range-index path has a multi-batch ordering defect and is unsafe during mixed-version Store upgrades. Evidence: static review of QueryList/QueryResults and NodeTxSessionProxy/store_stream_meta.proto at head b45fded.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The ordered range-index path still has correctness, cursor, resource, and compatibility defects beyond the existing review coverage. Evidence: exact-head static review of QueryResults, ScanUtil, OrderedKvIterator, OrderedMultiPartitionIterator, and public scan contracts at b45fded.

@contrueCT
contrueCT requested a review from imbajin August 8, 2026 15:41
@imbajin

imbajin commented Aug 12, 2026

Copy link
Copy Markdown
Member

A few potential follow-up performance directions to keep in mind after this PR lands. These are not required for the correctness fix in this PR, and each item should be validated by benchmark before we decide whether it is worth implementing.

Suggested benchmark coverage first:

  • small limit on many partitions
  • medium limit with sparse hits
  • large limit / page cursor continuation
  • the same query shape on 1, 4, 16, and 64 partitions
  • warm-cache and cold-cache runs, especially when network latency is visible
  1. Partition pruning / fan-out reduction

Idea: avoid opening streams for partitions that cannot possibly match the range. In practice, this is like asking only the shelves that may contain the target book, instead of checking every shelf.

+---------------- ordered range query ----------------+
|                                                     |
|      partition metadata / range boundary check       |
|                                                     |
+-----------+-----------+-----------+-----------+-----+
            |           |           |           |
          match       skip        match       skip
            |                       |
       +----v----+             +----v----+
       |  P1     |             |  P3     |
       | stream  |             | stream  |
       +---------+             +---------+

Why it may help: it directly reduces RPC count, merge width, and client heap pressure. This is likely the highest-ROI direction if benchmark shows the current fan-out is material.

  1. Adaptive page size

Idea: replace the fixed 64-entry page with a size chosen from limit, partition count, and observed latency. This is like choosing the bucket size based on how much water is needed and how far the well is.

+---------------- page-size policy ----------------+
| query shape                  | possible choice    |
+------------------------------+--------------------+
| small limit                  | smaller first page |
| many partitions              | moderate page      |
| high network round-trip cost | larger page        |
+------------------------------+--------------------+

Why it may help: it can reduce wasted reads for small limits and reduce round trips for larger or higher-latency scans. This is mostly a tuning policy, so it is a good candidate only after baseline numbers show the fixed size is suboptimal.

  1. Prefetch / pipeline

Idea: start fetching the next page before the current page is fully consumed. The goal is to hide network wait behind result consumption.

+---------------- time ---------------->

consume page N:      [====================]
fetch page N + 1:              [====================]
consume page N + 1:                       [====================]

effect: network wait is partially overlapped with iteration

Why it may help: it can improve tail latency when network RTT dominates. The tradeoff is more state: cancellation, timeout, backpressure, and cleanup paths all need to stay correct.

  1. Deeper merge pushdown

Idea: let Store side merge more aggressively so the client receives fewer ordered streams. This is like letting each Store produce one already-merged local stream before the client performs the final merge.

+---------------- Store A ----------------+
|  P1 stream   P2 stream   P3 stream       |
|      \          |          /             |
|       +---------v---------+              |
|       |  local ordered    |              |
|       |  merge in Store   |              |
|       +---------+---------+              |
+-----------------+------------------------+
                  |
                  v
+---------------- Client -----------------+
|        final merge across Stores         |
+------------------------------------------+

Why it may help: it reduces client-side merge pressure and can lower the number of streams the client manages. It is also the most structural option because it tends to touch cursor semantics, paging state, and protocol/versioning.

Suggested follow-up order:

  1. benchmark the current baseline
  2. evaluate partition pruning
  3. evaluate adaptive page size
  4. evaluate prefetch / pipeline
  5. consider deeper pushdown only if benchmark data justifies the added protocol and state complexity

My recommendation is to keep this PR focused on correctness and treat the items above as benchmark-driven follow-up candidates, not as changes to add into this PR.

@contrueCT
contrueCT force-pushed the task/issue-3053-hstore-range-index-ordering branch from ea9cce8 to b3addea Compare August 16, 2026 15:33
@imbajin

imbajin commented Aug 26, 2026

Copy link
Copy Markdown
Member

⚠️ Important — keep ordered-scan initialization fail-fast after the eight-worker pool is saturated

OrderedKvIterator.initialize() previously submitted every Store first-entry task before reading any completion. The shared initializer uses eight workers, a SynchronousQueue, and CallerRunsPolicy; once all workers were busy, the next submit() could execute firstEntry() on the query/coordinator thread. If that Store blocked, an earlier Store failure could already be queued but remain unobserved, delaying cancellation and stream cleanup.

Before

submit Store 1..8 -> worker pool
submit Store 9    -> coordinator runs it inline and blocks
Store 2 fails     -> failure waits unobserved in completion queue

After

submit at most 8 -> wait for one completion
completion fails -> cancel and close immediately
completion succeeds -> submit the next Store and keep the window full
ordered-scan-sliding-window

Sliding-window data structures

flowchart LR
    P["iterators<br/>nextSource points to the next pending Store"]

    subgraph W["Sliding window: inFlight <= 8"]
        E["INITIALIZER<br/>8 worker threads"]
        F["futures<br/>submitted async tasks for cancellation"]
    end

    C["CompletionService<br/>completed Future queue"]
    H["PriorityQueue of SourceEntry<br/>first entry from each Store"]
    X["Failure cleanup<br/>cancel futures and close all sources"]

    P -->|"submit: nextSource++, inFlight++"| E
    E -.->|"record Future"| F
    E -->|"task completes"| C
    C -->|"success: addFirst(), inFlight--"| H
    C -->|"one slot is free: submit next Store"| P
    C -->|"failure"| X
Loading

For example, with 40 Stores:

initial
  inFlight = [S1 ... S8]       pending = [S9 ... S40]

after S3 completes
  merge heap += first(S3)
  inFlight = [S1 S2 S4 ... S8 S9]
  pending  = [S10 ... S40]

The window remains full while work is available, but the coordinator always drains a completion before opening the next slot. A failure therefore reaches take().get() before a later Store can block the coordinator.

Implemented in b6644a908: if the shared pool rejects work, caller-thread fallback is used only when this query has no in-flight task whose failure could be hidden. The regression coverage includes the 9-Store failure case and both rejection paths.

- keep at most eight Store initialization tasks in flight
- observe completed failures before submitting more sources
- preserve safe fallback under shared-pool saturation
- cover nine-Store and rejection paths
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.49057% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.73%. Comparing base (431f6e6) to head (3941b1c).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
...g/apache/hugegraph/backend/query/QueryResults.java 67.53% 21 Missing and 4 partials ⚠️
...he/hugegraph/backend/store/hstore/HstoreTable.java 0.00% 22 Missing ⚠️
.../hugegraph/store/business/BusinessHandlerImpl.java 0.00% 10 Missing ⚠️
...graph/backend/store/hstore/HstoreSessionsImpl.java 0.00% 8 Missing ⚠️
...va/org/apache/hugegraph/backend/page/IdHolder.java 45.45% 6 Missing ⚠️
...che/hugegraph/store/client/NodeTxSessionProxy.java 88.67% 3 Missing and 3 partials ⚠️
...ava/org/apache/hugegraph/store/client/NodeTkv.java 69.23% 4 Missing ⚠️
...java/org/apache/hugegraph/backend/query/Query.java 33.33% 1 Missing and 1 partial ⚠️
...he/hugegraph/backend/tx/GraphIndexTransaction.java 66.66% 1 Missing and 1 partial ⚠️
...hugegraph/backend/store/hstore/HstoreSessions.java 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3140      +/-   ##
============================================
- Coverage     41.06%   37.73%   -3.34%     
- Complexity      519     6527    +6008     
============================================
  Files           771      800      +29     
  Lines         65962    68821    +2859     
  Branches       8766     9127     +361     
============================================
- Hits          27088    25969    -1119     
- Misses        36008    39810    +3802     
- Partials       2866     3042     +176     

☔ 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 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up in 3941b1c91 makes two small initialization improvements on top of the bounded-concurrency change:

  • Multiple Stores: after waiting for one completion, drain every completion already queued before refilling the 8-source window. This makes an already-completed failure visible before another Store stream (for example, S9) is started.
  • Exactly one Store: initialize its first entry inline and bypass executor/future dispatch, since the caller must wait for that entry anyway. The previous interruption contract is preserved: restore the interrupt flag, report the same initialization error, and close the source.

The ordered merge, concurrency bound, cancellation, and cleanup behavior remain unchanged.

Ordered scan initialization follow-up

@github-project-automation github-project-automation Bot moved this from In progress to In review in HugeGraph PD-Store Tasks Aug 26, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Aug 26, 2026
@imbajin
imbajin merged commit 2ae7370 into apache:master Aug 26, 2026
18 of 19 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in HugeGraph PD-Store Tasks Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Changes of API lgtm This PR has been approved by a maintainer perf size:XXL This PR changes 1000+ lines, ignoring generated files. store Store module

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug] HStore range-index scans don't guarantee global ordering or stable paging

2 participants