Skip to content

fix(ivfflat): restore exact PRE search domains - #28002

Open
aunjgr wants to merge 11 commits into
matrixorigin:mainfrom
aunjgr:fix/distributed-ivfflat-search-domain-27854
Open

fix(ivfflat): restore exact PRE search domains#28002
aunjgr wants to merge 11 commits into
matrixorigin:mainfrom
aunjgr:fix/distributed-ivfflat-search-domain-27854

Conversation

@aunjgr

@aunjgr aunjgr commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #27854

What this PR does / why we need it:

Treat scalar PRE membership as a required exact search domain and preserve the domain-before-search phase in typed VECTOR_INDEX_SCAN.

Keep required-domain scans on the coordinator CN while restoring local reader DOP with disjoint shard identities, reader-owned child contexts, and one synchronized snapshot clone per search generation. Share centroid and membership state without sharing mutable readers.

Apply exact membership before bounded storage Top-K admission so nearer nonmembers cannot under-fill results. Cross-CN domain fan-out remains deferred until representative Wiki-10M performance and resource evidence supports it, so this change claims no new MORPC capability version.

Treat scalar PRE membership as a required search domain and reuse broadcast HashBuild streams to seal the same exact domain on every vector-scan CN. Gate distributed placement for mixed-version and unsupported execution shapes while preserving fail-closed local fallback semantics.

Restore per-CN DOP with disjoint combined shard identities, reader-owned child contexts, and one synchronized snapshot clone per local reader generation. Share centroid cache and serialized membership state without sharing mutable reader state.

Apply membership before bounded local Top-K admission so nearer nonmembers cannot under-fill results, and strengthen the matrixorigin#27854 distributed BVT with adversarial distractors.
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH 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.

Request changes on exact head 100959ea6522.

  1. [P1] Preserve reader cardinality for an empty required domain. RuntimeFilter_DROP is correctly interpreted as an exact empty domain, but buildReaders returns exactly one EmptyReader at pkg/sql/compile/scope.go:1687-1690. The new vector path sets Mcpu > 1; buildScanParallelRun then creates Mcpu scopes and indexes readers[i]. A PRE predicate matching zero rows therefore reaches an index-out-of-range panic instead of returning an empty result. Return one empty reader per effective Mcpu and add a test that drives the DROP terminal through the parallel-scan construction path with Mcpu > 1.

  2. [P1] Do not reuse cumulative MORPC version 43. This head declares MORPCVersion43 for the new required-domain plan field, while open #27553 already declares version 43 for the MongoDB explicit-query payload (889d92833751); the latest #27756 head also records 43 as reserved by #27553 and moves its own gate to 44. Sharing the same rollout gate means a deployment can advertise version 43 while a receiver understands only one of the two independent wire additions. Rebase the current integration state and allocate a unique next version, with below/at-gate compatibility coverage.

  3. [P2] Close the distributed design and performance acceptance gate. This changes a wire contract, cross-CN placement, and reader concurrency, but the same-PR design is already marked implemented without an approved design phase. Its own acceptance requires 1-CN versus multi-CN result equivalence and evidence that DOP reduces the entries-scan critical path without recall loss. The added BVT has only 124 rows and does not force or observe multi-CN × multi-reader execution; no #27854-scale before/after data is provided. This matters because membership now disables storage Top-K and creates per-reader lookup state, so green functional CI cannot establish the latency/CPU/memory tradeoff. Please provide a deterministic topology test (including empty domain) and representative 1-CN/3-CN+DOP measurements for QPS/latency, CPU, peak query memory or membership-state amplification, scanned vectors/bytes, and recall, then update the design with the accepted decision and evidence.

…flat-search-domain-27854

# Conflicts:
#	pkg/vectorindex/ivfflat/plan_reader.go
Preserve parallel reader cardinality for exact empty domains and keep adaptive searches on one reader when they own a mutable cursor.

Build one admitted exact integer membership filter, metadata version, and centroid route per CN search generation. Share them across disjoint readers while storage intersects membership before distance-range and Top-K admission; retain the exact local fallback for unsupported domains.

Add forced multi-CN empty/result equivalence, ownership and race coverage, and persisted 768-dimensional benchmark evidence for the bounded storage path.
…flat-search-domain-27854

# Conflicts:
#	pkg/defines/const.go
#	pkg/pb/plan/plan.pb.go
#	pkg/sql/plan/deepcopy.go
#	pkg/sql/plan/deepcopy_test.go
#	proto/plan.proto

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

Deep re-review of exact head 71d0720e12825be174147231e13759f5eea5215a against merge base e0a87386511de6c3a1379a5024b269b84b5ca1c8. I read the complete review/conversation history and issue #27854, rechecked the full diff, and verified the previous empty-domain reader-cardinality failure is fixed. I also audited source-PK membership binding, per-CN/local shard identity, shared filter/snapshot ownership, partial-open cleanup, cancellation, and membership-before-storage-Top-K behavior.

Two P1 merge blockers remain: the cumulative MORPC version is concurrently allocated by two other open PRs, and the design's explicit Wiki-10M distributed performance merge gate is still unfulfilled. These are independently verified below rather than deferred to the existing reviews.

Validation passed with CGO third-party artifacts: affected planner/compiler/APPLY/IVFFlat package tests; focused ownership, protocol-gate, empty-domain, and multi-CN tests under -race -count=10; TestIssue27854RequiredVectorDomainOnMultiCN; and go vet for the affected packages.

Comment thread pkg/defines/const.go Outdated
Comment thread docs/design/distributed_ivfflat_search_domain.md Outdated
…flat-search-domain-27854

# Conflicts:
#	pkg/defines/const.go
#	pkg/pb/plan/plan.pb.go
@aunjgr

aunjgr commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Updated exact head 58b46b6554387078c28178f2047f12d159b438c9 after merging newest upstream/main.

The MORPC allocation blocker is resolved:

  • authoritative main now owns v44 for the validated MongoDB explicit-query payload;
  • required vector-search domains now own MORPCVersion45;
  • MORPCLatestVersion, the planner runtime gate, below-gate v44 test, at-gate v45 test, and design compatibility section agree;
  • plan.pb.go was regenerated from the combined main+PR schema, preserving both MongoDB and required-domain fields.

Validation on the pushed post-merge head:

  • check.sh --pre-push upstream/main: pass, zero lint issues
  • full pkg/sql/plan, pkg/sql/compile, and pkg/pb/plan: pass
  • all pkg/vectorindex/ivfflat/..., pkg/sql/colexec/apply, pkg/indexplugin/search, and pkg/vectorindex/sqlexec: pass
  • TestIssue27854RequiredVectorDomainOnMultiCN: pass after final main merge, including local/multi-CN equivalence and exact empty domain

The Wiki-10M performance gate is not resolved. This workspace has neither the Wiki-10M dataset nor the required one-CN/three-CN deployment, so I am not substituting the 8,192-row storage microbenchmark or functional CI for that evidence. That review thread should remain open, and I am not re-requesting review until the declared QPS/latency/CPU/peak-memory/scanned-bytes/recall run is attached.

@XuPeng-SH XuPeng-SH 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.

Deep re-review of exact head 58b46b6554387078c28178f2047f12d159b438c9, including the complete prior review history and the current #27854 acceptance contract.

The earlier empty-domain cardinality, filter-before-storage-Top-K, shared-domain/route ownership, snapshot-clone, and v43/v44 findings are fixed on this head. I rechecked combined CN/local-reader shard identities, partial-open cleanup, cancellation, exact membership lifetime, global Top-K recoverability, protocol fallback, malformed/PASS/DROP terminals, and correlated/adaptive fallbacks. Focused exact-head tests pass; the shared-session and shared-snapshot concurrency tests each pass under -race -count=20; the forced multi-CN SQL regression also passes.

[P1] The PR still has not met its own representative performance merge gate, so this head does not yet establish that it fixes #27854 without a distributed resource regression. The linked failure is Wiki-10M at 100 concurrent queries. The 8,192x768 persisted-object microbenchmark usefully proves that a preselected row mask avoids embedding materialization, but it does not exercise complete-domain HashBuild/broadcast on every CN, per-CN filter reconstruction, CN×DOP range scans, global candidate merge, or concurrent CPU/peak-memory amplification. The design explicitly requires one-CN versus three-CN+DOP Wiki-10M QPS/latency, CPU, peak memory, scanned bytes, and recall against current main/former TVF, with stated thresholds; the latest author comment explicitly confirms that evidence is still pending. Attach that evidence and update the design decision/status before merge. If the bounds fail, keep the distributed/DOP route gated or coordinator-local.

[P1 merge-order condition] MORPC v45 is concurrently claimed by five other live independent heads: #28055 (binary-string semantics), #27972 (window partitioning), #27914 (grouping-set projection), #27903 (Parquet fanout), and #27716 (information_schema.VIEWS). Main still ends at v44, so this PR may own v45 only if it is explicitly selected to merge first. If any other v45 capability lands first, rebase latest main and move this field, planner gate, tests, generated protobuf expectations, and design references to the next unique cumulative version before merging; do not resolve the const conflict while retaining an aliased gate.

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

Deep re-review of exact head 58b46b6554387078c28178f2047f12d159b438c9, against current PR base/merge-base b772cc02357e3570ed0114f21c9983ac952dffe1 and previous aptend-reviewed head 71d0720e12825be174147231e13759f5eea5215a. I read the complete review, inline reply, thread, PR-comment, and issue #27854 history. git range-diff confirms the three feature commits are unchanged; the increment merges newer main and resolves the prior v44 conflict by moving this capability to v45. I rechecked the complete 26-file PR diff: exact required-domain terminals, source-PK typing, broadcast-domain consumption, coordinator fallback, CN×reader shard identities, empty-domain cardinality, child contexts, shared snapshot/centroid/filter ownership, partial-open cleanup, storage-filter-before-Top-K, and global candidate recovery. The prior empty-domain and v44 findings are closed.

Two merge blockers remain:

  1. The PR's own representative performance gate is explicitly unfulfilled. Issue #27854 is Wiki-10M, 768 dimensions, three CNs, 100 concurrent queries; its newest main evidence is still only 15.76 QPS for threshold PRE versus recent 70.81/73.98/80.38 QPS, while this PR supplies only an 8,192-row storage-boundary microbenchmark. That benchmark excludes the new end-to-end costs: complete-domain HashBuild/broadcast per CN, filter reconstruction, CN×DOP ranges, global candidate merge, and CPU/peak-memory amplification at concurrency. The author reply and design both explicitly state Wiki-10M evidence remains a merge gate. Attach the declared one-CN/three-CN+DOP comparison against current main/former TVF and show every written threshold is met; otherwise keep the distributed route gated/coordinator-local.
  2. v45 is currently a merge-order condition, not a unique capability allocation. Authoritative main 2112809dbf0cf6d1f8a09e8bd260606fc6bb5677 still ends at v44, while current live heads #28055, #27972, #27914, #27903, and #27716 independently advertise v45 for unrelated wire/runtime capabilities. This head is safe only if it is explicitly selected to land first. If any competitor lands first, rebase and move the field, gate, generated descriptor, tests, and design to the next cumulative version before merge.

Exact-head validation passed with the required CGO/thirdparties setup: full pkg/sql/plan, pkg/sql/compile, pkg/sql/colexec/apply, pkg/pb/plan, all pkg/vectorindex/ivfflat/..., and pkg/vectorindex/sqlexec tests; TestIssue27854RequiredVectorDomainOnMultiCN; focused ownership/protocol/empty-domain tests under -race (count=20 for IVFFlat and count=10 for compile); go vet on affected packages; and git diff --check. GitHub CI is terminal and green, but functional CI cannot close the explicit issue-scale performance gate.

Comment thread docs/design/distributed_ivfflat_search_domain.md Outdated
Comment thread pkg/defines/const.go Outdated
Preserve exact domain-before-search semantics, storage membership-before-Top-K, and local reader DOP while forcing required PRE domains onto the coordinator CN. This removes unmeasured cross-CN domain replication and fan-out until Wiki-10M performance evidence exists. Drop the unused MORPC v45 claim and update the design and public regression to make coordinator placement the fail-closed contract.
@aunjgr aunjgr changed the title fix(ivfflat): restore distributed exact PRE search fix(ivfflat): restore exact PRE search domains Sep 3, 2026
@aunjgr
aunjgr requested review from XuPeng-SH and aptend September 3, 2026 14:40

@XuPeng-SH XuPeng-SH 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.

Deep re-review on exact head 09c15e8a205566d38b2b900b632ea7a10a07be3a.

The implementation blockers from earlier heads are closed: the required domain is now coordinator-local, so there is no cross-CN wire/version claim or domain replication; empty-domain reader cardinality, exact filter-before-storage-Top-K, one shared snapshot/filter/centroid route per local generation, disjoint local shards, cancellation, partial-open cleanup, malformed/PASS/DROP behavior, and final SQL recheck are coherent. I found no new deterministic correctness, ownership, hang, or unbounded-growth defect in the full current diff.

[P1][performance acceptance] The PR still does not show that this exact coordinator-local head fixes #27854 at the workload where the bug exists.

The issue is a Wiki-10M, 768-dimensional, 100-concurrent-query endpoint regression (historically ~70-80 QPS in recent controls, then 15.76 QPS, and originally ~0.37 QPS/timeouts). The only measurement remains the 8,192-row storage microbenchmark. It proves the new primitive avoids embedding materialization and is faster in isolation, but it does not include the complete PRE-domain HashBuild, local DOP scheduling/ranges, candidate merge/final recheck, concurrent CPU/query-memory pressure, or actual endpoint latency/QPS. Functional two-CN BVT and green CI cannot prove a performance bug is fixed.

The previous distributed fan-out concern is no longer applicable and I am not asking for the deferred cross-CN route's 1-CN-vs-3-CN enablement gate. The missing gate is narrower: run the actual coordinator-local exact head against the issue's representative 3-CN/100-concurrency Wiki-10M workload (or a demonstrably equivalent reduced-duration A/B) versus exact main, and attach QPS/latency, recall, CPU/peak memory, and scanned-vector/byte evidence. It must show the target regression is removed without a new resource regression before this performance-fix PR is merge-ready.

The current author comments also explicitly state that representative Wiki-10M evidence is unavailable/pending.

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

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants