perf(vector): expose IVF execution work in EXPLAIN ANALYZE - #28622
Conversation
…xecution-observability-24097
…xecution-observability-24097
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
…xecution-observability-24097
XuPeng-SH
left a comment
There was a problem hiding this comment.
APPROVE at c5d4eee against base cd8e8d9. The previous P2 on cached PK/prefix StorageFilterInputRows is resolved; no new concrete blocker found.
Rechecked the former counterexample: after a successful cached search, ReadDataByFilter now adds MetaLocation().Rows() before the empty-selection early return. This uses the same physical-block denominator as the materialized vector path, without defeating cached search's no-materialization optimization. Cached and materialized branches are exclusive; output is still counted only after tombstones succeed. The new persisted-prefix cases assert 5 input / 4 output after tombstone removal and 5 / 0 for no match, so they directly exercise the missing branch rather than only constructing synthetic diagnostics.
Re-reviewed the current instrumentation and relevant delta: exact-membership fusion, whole/chunk/cached vector Top-K accounting, scored-row/null handling, per-reader accumulation across reads, scan-role aggregation and nanosecond units, one-time summary publication/drain, close/error ownership, and versioned heading transport/rendering. Per-reader collectors remain independent and synchronously borrowed; the change does not alter selected rows or distance decisions. The delta also gates time.Now() and the reader-stats slice behind collection (and the entry role for the slice), removing those disabled-path costs. Counters describe physical filter input, consumed chunks, and block-local work; these must not be interpreted as network-only bytes, unique cached bytes, or end-to-end wall time.
Evidence/limits: exact-head source and control-flow review, full current PR change map, relevant post-review delta and regression assertions, plus prior finding/fix discussion. Author reports focused/owning-package tests and full pre-push checks; I did not rerun native UT/BVT, benchmarks, or race tests and did not wait for CI. Head/base were rechecked unchanged immediately before submission.
aptend
left a comment
There was a problem hiding this comment.
Re-review complete at exact head c5d4eeeb298106f81559aa30aa06f614a3652259 (base/merge-base cd8e8d91343798f565f8ad0f8473cb995b8b361b). I read the full review/comment/thread history and verified the previously blocking cached-search accounting path against the code rather than relying on the resolution: after a successful persisted cached search, StorageFilterInputRows now records the block row count even for empty matches, while output remains post-tombstone. The new deterministic 5→4 and 5→0 cases cover both affected outcomes.
I re-reviewed the incremental diff since the prior reviewed head d64a4da1ab88eba1ce6146abd4f51be884c584a7 and the complete 19-file PR diff, including execution ownership/publication/drain, prepare-vs-reader DOP aggregation, hidden-relation timing/counting, storage filter and Top-K accounting, chunk/cache/byte counters, error/cancellation behavior, remote-safe diagnostic encoding, and EXPLAIN aggregation. I found no remaining blocking issue.
Exact-head validation:
- focused diagnostic, plan-reader/DOP, objectio Top-K, EXPLAIN, and blockio persisted/cached-filter tests passed across
./pkg/vectorindex,./pkg/vectorindex/ivfflat,./pkg/objectio,./pkg/sql/plan/explain, and./pkg/vm/engine/tae/blockiowith the repository CGo prerequisites git diff --checkpassed
Merge Queue Status
This pull request spent 2 minutes 37 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
Related to #24097
What this PR does / why we need it:
Expose bounded IVF execution work in EXPLAIN ANALYZE, including hidden-relation blocks and rows, storage-filter selectivity, exact distance evaluations, vector chunk/cache bytes, block Top-K output, DOP reader count, and stage timings.
Design and validation