Skip to content

perf(vindex): configure independent range-read concurrency - #720

Open
jerry-024 wants to merge 4 commits into
apache:mainfrom
jerry-024:perf/vindex-range-read-concurrency
Open

perf(vindex): configure independent range-read concurrency#720
jerry-024 wants to merge 4 commits into
apache:mainfrom
jerry-024:perf/vindex-range-read-concurrency

Conversation

@jerry-024

@jerry-024 jerry-024 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Purpose

Decouple Vindex range-read I/O concurrency from global-index execution concurrency so each limit can be tuned independently.

Brief change log

  • Add the validated global-index.range-read-thread-num table option with a default of 32.
  • Share the range-read semaphore across Vindex readers and clones so the configured limit applies to the whole search.
  • Thread the independent range-read limit through vector-search planning and execution.
  • Add unit coverage for independent concurrency limits, ordering, failure recovery, and range-read accounting.

Tests

  • cargo +1.97.0 fmt --all -- --check
  • cargo +1.97.0 test -p paimon vindex::range_reader (16 passed)

API and Format

Adds the global-index.range-read-thread-num table option. There is no storage-format change.

Notes

  • The default remains 32 pending the 10M A/B benchmark result.

@jerry-024
jerry-024 marked this pull request as draft August 17, 2026 02:23
@jerry-024 jerry-024 changed the title feat: add vector search timing logs perf(vindex): decouple range reads and bound batch memory Aug 17, 2026
@jerry-024
jerry-024 force-pushed the perf/vindex-range-read-concurrency branch from f242b52 to 3ce7998 Compare August 17, 2026 03:50
@jerry-024 jerry-024 changed the title perf(vindex): decouple range reads and bound batch memory perf(vindex): configure independent range-read concurrency Aug 17, 2026
@jerry-024
jerry-024 force-pushed the perf/vindex-range-read-concurrency branch from 0946fcd to 3ec12da Compare August 17, 2026 07:19
@jerry-024
jerry-024 marked this pull request as ready for review August 17, 2026 07:46

@shyjsarah shyjsarah 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.

Thanks for separating Vindex range-read concurrency and for the thorough runtime/concurrency tests. I found one scalability/fairness issue that should be addressed before merge, plus one documentation follow-up.

.push(merged.len());
}
let ranges: Vec<_> = merged.iter().map(|merged| merged.range.clone()).collect();
let fetched = self.fetch_range_batch(&ranges)?;

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.

[Major] Please keep the refill pipeline bounded instead of queueing every merged range at once.

try_join_all creates/polls a future for every range, so the semaphore limits only active FileRead calls—not queued permit waiters or completed Bytes retained until the slowest read finishes. Since Tokio's semaphore is FIFO, one large pread can also enqueue all of its ranges ahead of other Vindex readers sharing this limiter. This changes the old chunk barrier into O(R) queued state and can cause cross-reader head-of-line blocking.

Could this use a rolling FuturesUnordered/buffer_unordered(max_range_read_concurrency) window, carrying the original index to restore result order? That preserves immediate refill while keeping local waiters bounded. A regression test with two cloned readers sharing permits would also help verify that a large batch cannot monopolize the queue.

Comment thread crates/paimon/src/spec/core_options.rs Outdated
}

/// Maximum number of concurrent range reads shared by Vindex readers in one
/// search operation. This is independent of [`Self::global_index_thread_num`].

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.

[Minor] Please update the existing option documentation and migration notes for this new independent limit.

The current global_index_thread_num Rust docs still say it limits global-index/PK-vector I/O and that 1 gives strict sequential execution; docs/src/sql.md also documents only global-index.thread-num. Those statements are no longer true for Vindex range reads when this option is absent (the new default is 32). Please narrow the old option's documented scope, document global-index.range-read-thread-num and its default, and call out the changed upgrade behavior for tables with a non-default global-index.thread-num.

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.

2 participants