[improvement](lance) Add ANN prefilter optimization and profiles (branch-4.1) - #68615
Conversation
### What problem does this PR solve? Related PR: lance-format/lance#9599, lance-format/lance-c#89 Problem Summary: Complete index-segment vector scans without predicates materialize an unnecessary row-ID allowlist. Pin upstream lance-c with the Lance development-branch optimization and metric-based regressions. Remove superseded local patches, retaining only the Foyer cache integration with object-store compatibility and bounded response-metadata reuse. ### Release note Avoid redundant prefilter row-ID construction for unfiltered vector queries covering complete visible index segments. ### Check List (For Author) - Test: Lance prefilter and segment-contract tests; lance-c and Foyer unit/C API suites; Rust format and Clippy; C/C++ executable tests; dependency downloader patch lifecycle, checksum, and fallback regressions. - Behavior changed: Yes, eligible vector scans skip redundant prefilter work; predicates, partial coverage, deletions, and flat fallback retain semantics. - Does this need documentation: No; no SQL or configuration interface change.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Complete static review of apache/doris PR #68615 at head 29f3df9 (base 4750b1c). Opinion: no new actionable finding and no existing P0/P1 inline finding. All changed paths and the three recorded risk areas have a conclusion.
Critical checkpoints:
- Goal and evidence: the PR pins lance-c/Lance revisions that contain the complete-segment prefilter guard and loader metrics, removes the superseded patch chain, and retains the Foyer cache patch. Exact pinned source includes targeted ANN/FTS and C API regression cases; the downloader regression is added here. These tests were inspected, not executed.
- Scope: the integration changes are concentrated in the third-party pin, patch lifecycle, retained cache patch, and one downloader regression. No FE/BE source or protocol interface is changed.
- Concurrency: shared cache state uses Foyer synchronization; dataset counters use relaxed atomics for counters; wrapper registry map operations hold a mutex and release it before dropping intermediate wrappers. No new lock-order or thread-entry issue was substantiated.
- Lifecycle and initialization: one shared session creates dataset cache scopes; restore attaches a fresh scope; writer-created handles begin without one. The new SHA-named source directory and Foyer marker govern patch lifecycle. No cross-translation-unit static initializer was introduced.
- Configuration: no new Doris runtime setting is added. The retained C cache options validate directory and capacities; dynamic configuration behavior is unchanged.
- Compatibility: the pinned C interface supplies Doris's Lance calls and the removed patch APIs; static OSS transport remains installed. The exact upstream source was checked because its history diverges from the prior Lance pin. Runtime and data-format compatibility were not independently exercised under this review contract.
- Parallel paths and conditions: whole/ranged get_opts and batched get_ranges, conditional/versioned bypass, direct data-file selection, ANN/FTS/scalar segment selection, deletion masks, and unindexed fallback were traced. The prefilter shortcut requires complete selected fragment coverage and no actual predicate.
- Tests and expected results: upstream tests cover partial/complete coverage, predicates, stable/physical IDs, deletion and fallback; retained cache tests cover reads, EOF, restore, and disk reuse; the new downloader script covers fresh, repeated, re-extracted, generic-marker, and patch-rejection states. No .out result changes. No build or test was run, as instructed.
- Errors and observability: failed patch application aborts before marking success or building; origin read failures propagate; cache lookup warnings and existing scan/cache metrics provide diagnostics. No material new observability gap was substantiated.
- Persistence, transactions, and writes: Doris transaction, EditLog, visible-version, and FE/BE variable paths are unchanged. Foyer's persistent cache remains restricted to immutable data-file reads; no new Doris write path was introduced.
- Performance and memory: the intended guard skips redundant row-ID prefilter loading; bounded metadata reuse avoids repeated HEAD requests. The additional metadata budget is one eighth of configured cache memory. No new material performance or memory issue was substantiated.
- Other concerns: a possible S3 endpoint cache-key collision predates this PR in the merge-base Foyer patch and older Lance provider, so it was not duplicated as a new finding.
Review completion: one convergence round with two normal full-review passes and two separate focused risk passes; all returned NO_NEW_VALUABLE_FINDINGS. The main final sweep found no unresolved candidate. Live base/head and inline comments were refreshed before submission and matched the review bundle; there were no existing inline comments. User focus: none was supplied.
### What problem does this PR solve? Related PR: lance-format/lance#9599, lance-format/lance#9537, lance-format/lance-c#89 Problem Summary: Pin the reviewed Lance prefilter documentation and expanded partial-coverage regression through upstream lance-c. Include the C API regression verifying identical 4-bit PQ candidates and distances with and without an all-row prefilter. The dependency already contains the final merged exact FastScan scoring fix. Refresh the immutable archive checksum; the retained Foyer patch is unchanged. ### Release note None ### Check List (For Author) - Test: 138 Lance prefilter tests, 62 PQ tests, and 443 lance-c Rust tests passed. Fresh/idempotent/re-extracted/invalid-patch downloader regressions passed on both Doris branches; shell syntax and diff checks passed. - Behavior changed: No; this update carries upstream documentation and regression coverage. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
COMPLETE static review of PR #68615 at 4a86e23553670f3a630037bb85ecc020d42c0fb3 against base 4750b1c988fd6acca403f2211c6882941f74d902. I reviewed all 12 changed paths and the affected upstream/Doris read paths. I found no new actionable issue and confirmed no existing P0/P1 inline finding.
Goal, scope, and tests. The change replaces the local Lance-C search/scanner patch chain with an immutable upstream pin and retains the Foyer data-file cache as one rebased patch. The pinned archive's MD5 and source directory match vars.sh. The pinned C header retains the public APIs added by the removed patches; the pinned Lance revision includes the full-snapshot/FTS and segment-prefilter fixes. Upstream regression sources cover full and partial segment coverage, predicates, deletes, FTS metrics, multi-vector/scalar paths, and PQ scoring. The new shell script covers fresh, repeated, re-extracted, cached-marker, and rejected-patch scenarios, but it is not invoked by the current third-party CI workflow. This review did not run builds or tests.
Correctness and parallel paths. The upstream ANN shortcut checks the actual selected segment coverage and empty predicate before omitting the redundant row-ID prefilter; partial or unknown coverage and predicates keep the filtering path. The downstream dataset mask still handles deleted rows, unavailable fragments, and overlay blocks. I checked ordinary, vector, FTS, scalar-segment, multi-vector, and static OSS transport paths against the exact pinned source and Doris's Lance reader. Existing visible-version selection and read paths are unchanged. The Foyer wrapper remains limited to immutable direct data/*.lance children; conditional/versioned requests, writes, and other files use the origin store.
Focus, compatibility, and lifecycle. The new download gate is limited to the SHA-named Lance-C source directory and an independent Foyer marker. The archive check and patch exit status fail before success is marked. All 15 Foyer patch file hunks match the pinned source at fuzz 0; the patched Cargo manifest and lock parse and their dependency versions resolve statically. The rebased object_store wrapper, paginated listing, and GetResult extensions match the pinned APIs. The metadata cache is bounded to one eighth of the configured memory capacity. Existing weak-reference wrapper registration, dataset/session ownership, restore scopes, and short mutex use showed no new leak, deadlock, or unsafe static initialization. No new Doris config, FE/BE protocol field, persistent metadata, transaction, or write-format change is introduced; the existing cache setting remains restart-scoped.
Error handling, performance, and observability. Invalid cache options and patch failures return errors. The new metadata path reuses immutable-file HEAD results when safe and falls back to origin lookup on a miss; the existing per-dataset cache counters and warnings remain available. Search metrics for prefilter loads and rows remain in the pinned upstream revision. The conditions around the optimization and marker are documented in source. No distinct correctness, compatibility, performance, or coverage defect was substantiated after the complete changed-file and unresolved-point sweep.
User focus: no additional focus was provided. Review status: complete, static only; no new inline comments. The Foyer dependency rebase, upstream patch replacement, and cache-marker hypotheses were investigated and dismissed with exact-source evidence.
|
run buildall |
|
/review |
Gabriel39
changed the title
There was a problem hiding this comment.
Review conclusion
Complete static review of PR #68615 at a7ff3f86f1aa270bd5a4fffc58cd12a3627f2cd4: no new actionable findings. All 15 changed paths were covered. Three normal full-review and two separate risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS in the first round; I checked their conclusions against the reader, pinned dependencies, cache patch, and final changed-file list. No existing inline thread or independently confirmed P0/P1 comment was present. The user supplied no additional focus point.
Critical checkpoints
- Goal, scope, and proof: The PR replaces the local lance-c search patch chain with pinned upstream lance-c/Lance revisions, retains a rebased Foyer cache patch, avoids the redundant row-ID allowlist for eligible unfiltered ANN scans, and exposes ANN stage timers in the Doris profile. The pinned archive checksum matches
thirdparty/vars.sh; the removed C entry points and enum values remain available. Source traces support the requested behavior, and focused upstream and BE regression cases are present. This review did not run them. - Focus and parallel paths: The Doris edit adds profile mappings, a focused indexed multivector assertion, and timing documentation. The larger third-party diff removes superseded patches and rebases the one retained cache integration. I traced normal/scalar, single-vector, multivector, and prepared FTS paths, including partial index segments, appended unindexed fragments, filters, deletes, and overlay masks. When the full-segment no-predicate guard does not hold, Lance retains the filtered row-ID path; deletion and overlay masks still apply when it does. The scope guard and fallback are documented in the pinned code.
- Concurrency, lifecycle, and memory: Lance executes partition work concurrently, so its stage durations can overlap and must not be summed as query wall time. Doris registers the existing statistics callback before execution and closes the scanner while its callback context is alive; the final summary is collected on stream release. The Foyer wrapper keeps per-dataset atomic byte counters, a shared cache, and a short mutex-protected weak-wrapper registry; the registry lock is released before I/O. Reader close snapshots cache counters before dataset close. No new Doris thread, lock-order chain, cross-translation-unit static initializer, or BE-owned allocation path was introduced.
- Errors, visibility, persistence, and compatibility: The search configuration checks C API return codes. The scoped ANN path retains snapshot visibility through the selected-segment guard and downstream deletion, missing-fragment, and overlay masks. Foyer caches only immutable direct
data/*.lancereads; conditional, versioned, and extension-bearing requests bypass metadata admission, and missing cached blocks are fetched from the origin. The patch applied to the clean pinned source with zero fuzz in a read-only dry-run. No Doris EditLog, transaction, table-write, storage-format, or FE-to-BE variable change is in this PR. The upstream dependency moves from Lance v11 to v13 beta; its public C symbols and explicit legacy enum values were checked statically, but runtime mixed-version behavior was not exercised. - Configuration, performance, and observability: No new Doris configuration item or dynamic-change expectation is introduced; the existing shared Lance session remains process scoped. The upstream guard removes redundant row-ID materialization only for eligible ANN scopes. The rebased cache adds bounded in-memory response metadata to avoid repeat HEADs for cached range reads. The nine new
index_*_timekeys match the pinned Lance definitions and travel as nanoseconds through lance-c into the Doris profile; the operator baseline names also match. Counters cover coarse partition work, and the documentation explains nesting and paths that legitimately report zero. No latency or memory benchmark was run. - Tests and remaining validation: The changed BE test reads the indexed multivector fixture to completion and checks nonzero stage counters along with existing result and prefilter assertions. The new third-party shell case covers fresh/repeated extraction, marker reuse, and rejected patch application; upstream tests cover partial scopes and search variants. No expected-output file changed. Repository source, builds, and runtime tests were left untouched under the review instructions, so test outcomes and production latency remain unverified by this review; author-reported validation was not treated as independent evidence.
All initial suspicious mechanisms have evidence-backed conclusions in the shared review ledger. The final sweep found no unresolved candidate and no accepted inline issue; comments and existing_blocking_comment_ids are empty.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68615 at 8c7f614. Two review rounds converged. One new P2 issue is reported inline; no existing P0/P1 inline finding was present in the review context.
Critical checkpoints:
- Goal and tests: The PR consolidates local lance-c search patches into a pinned upstream revision and exposes ANN stage timings. The pinned archive hash, expected C APIs, FTS/scalar/vector/multi-vector and prefilter paths, and OSS transport were checked. The new BE test verifies timings after a full EOF read, but misses the reported early-stop path. The shell test covers patch-source setup rather than runtime search.
- Scope: The dependency switch, retained Foyer rebase, profile mapping, test, and documentation form a focused change. All 15 changed paths were swept.
- Concurrency: Lance stage timers accumulate across asynchronous partition tasks; profile counters receive nanosecond totals through the C callback. Foyer shared cache wrappers, atomic statistics, and mutex-protected wrapper tracking were reviewed; no distinct new race, lock-order, or heavy-lock defect was substantiated.
- Lifecycle: Dataset/session and wrapper ownership, re-extraction markers, scanner close, cancellation, and EOF were traced. The EOF-only callback leaves new ANN timings unpublished when a pushed SQL LIMIT retires a full block before another poll; see the inline finding. No other lifecycle defect was substantiated.
- Configuration and compatibility: No dynamic Doris configuration, FE-BE variable, network protocol, or Doris storage-format change is added. The pinned lance-c manifest and build script both require Rust 1.91, and its C API plus the retained Foyer patch cover Doris callers. The Foyer patch applies cleanly to the exact pinned archive in a read-only dry run.
- Parallel paths and conditions: Indexed and unindexed vector/FTS/scalar paths, prefilter and fallbacks, conditional/versioned cache bypass, and immutable data-file range reads were checked. Removed patch behavior was cross-checked against the pinned source; no separate new correctness issue was confirmed.
- Coverage and results: The added BE fixture is IVF_FLAT and exercises the asserted timing names on EOF. Its early-stop profile gap needs a regression case. No builds or runtime tests were run under the review-only instructions; author and source tests were inspected statically, not treated as execution evidence.
- Observability, performance, and memory: Timing names and units match the pinned source, and the documentation correctly warns that overlapping stages cannot be summed. The reported missing partial-scan profile is the observability defect. Cache key, metadata budget, batch range behavior, allocation and coarse timer overhead showed no other substantiated new regression.
- Persistence, writes, and core invariants: This diff changes external reads and a cache patch, not Doris transaction, EditLog, MoW delete bitmap, or write/publish paths. The FE still requires a fixed positive Lance version. No new data-visibility, error-propagation, or memory-ownership defect was substantiated.
- User focus: No additional focus was supplied. Existing inline threads in the review context were empty; no issue was duplicated.
This is a complete static review with one accepted inline finding.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review conclusion
Complete static review of apache/doris PR #68615 at head 1bdd1ea against base 4750b1c: no new actionable finding. All 15 changed paths and the three recorded risk areas have a conclusion. The current [P2] inline comment 4132705390 about ANN timings missing when a scan stops before EOF still applies; it is already reported and is not duplicated here. No existing P0/P1 inline finding remains, so existing_blocking_comment_ids is empty. One round converged: two normal full-review subagents and one separate upstream-migration risk subagent all returned NO_NEW_VALUABLE_FINDINGS. The main final sweep found no unresolved candidate.
Critical checkpoints
- Goal, scope, and proof: The PR pins an upstream lance-c/Lance revision that retains the complete-segment prefilter optimization and removed local patch APIs, keeps the rebased Foyer cache integration, and maps ANN stage timings into Doris profiles. The immutable archive digest matches vars.sh. The C API surface, upstream prefilter guard, metrics producer names, and BE consumer names were checked against the exact pin. The code changes are focused on dependency migration, profile registration, one BE regression, and documentation. Source tests exist, but this review did not execute them.
- Concurrency and lifecycle: Lance partition work is concurrent; the emitted stage durations are cumulative and can overlap. Doris registers the existing statistics callback before scanning, and the reader closes its scanner while its callback context remains alive. The callback publishes only after a derived stream reaches EOF, which explains the already reported P2 on early stop. The Foyer cache is shared by sessions, with dataset-scoped atomic counters and a short mutex around weak wrapper registration; wrapper destruction occurs outside that lock. Open and restore attach scopes, while writer-created handles start without a data cache. No new Doris thread, lock-order chain, cross-translation-unit initializer, or unreleased ownership cycle was substantiated.
- Configuration and compatibility: No new Doris runtime configuration or FE-to-BE variable is added. The rebased cache options validate directory and capacities; dynamic configuration behavior is unchanged. The pinned C interface retains Doris's FTS, scalar-segment, multivector, scanner, and cache symbols; the static OSS transport initializer remains reachable. This review inspected API and source compatibility, without independently exercising a rolling upgrade or runtime storage-format compatibility.
- Parallel paths and conditions: The upstream shortcut requires complete selected fragment coverage and no actual predicate. Partial coverage, filters, deletes, and unindexed tails retain their filtering or fallback paths. Normal, vector, multivector, FTS, scalar-segment, and OSS paths were traced. Foyer handles immutable direct data/*.lance whole-object and range reads; conditional and versioned requests bypass cached data, and missing blocks fall back to origin reads. Error status and patch failure paths propagate rather than marking success.
- Tests and results: The changed BE fixture verifies indexed multivector rows and six nonzero stage timers after consuming to EOF; it does not cover the existing early-stop issue. The new downloader script covers fresh and repeated extraction, existing markers, and rejected patches. Pinned upstream tests cover full and partial segment scopes, predicates, deletes, scoring, and metrics. No expected-output file changed. The task prohibits builds and runtime tests, so test results and production latency were not independently validated; author-reported results were not treated as proof.
- Observability, performance, and memory: Nine new index time names and operator baseline names match pinned producers and nanosecond profile units. The documentation identifies nested and overlapping durations and legitimate zero counters. The prefilter guard avoids redundant row-ID materialization in eligible searches. The rebased Foyer metadata cache is bounded to one eighth of configured data-cache memory and avoids repeat HEAD requests on usable immutable-file entries. No other material performance, allocation, or observability regression was substantiated.
- Persistence, transactions, writes, and core invariants: This diff changes external reads and cache integration, not Doris EditLog, transaction publication, visible-version selection, MoW delete bitmaps, FE/BE request fields, or table write formats. Cache errors retain origin-read/error behavior. No new data-visibility, write-atomicity, or memory-tracking violation was substantiated.
User focus: no additional focus was supplied. Review status: complete, static only; no new inline comment.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68615 at eb51a98 (base 4750b1c). One convergence round completed: the main risk scan, both full-coverage reviews, and the separate risk-focused review returned NO_NEW_VALUABLE_FINDINGS. All 15 changed files and both initial risk items received a final sweep. No new inline findings or confirmed existing P0/P1 issues.
The existing P2 early-stop ANN timing thread still applies to this head. The pinned lance-c callback publishes its execution summary only after a stream is polled to EOF, so closing after a full first block can leave the new Doris timers at zero. This is already reported and is not duplicated here.
Critical checkpoints:
- Goal, scope, and tests: the pinned lance-c/Lance revisions supply the complete-segment ANN prefilter path and the new stage timings; Doris maps the emitted keys into its scan profile. The BE multivector test checks populated timers on an EOF-drained indexed scan, and the new third-party script covers archive extraction and Foyer patch markers. The full-block early-stop test remains the known P2 gap. The changes are focused on the dependency replacement, retained cache patch, profile mapping, test, and documentation.
- Concurrency and lifecycle: Lance may search partitions concurrently; its timing summaries are cumulative and overlap parent stages. Doris profile updates use atomic counters, and the Foyer wrapper registry holds its mutex only for short identity lookups. The scanner is retired between splits and before its reader context is destroyed. The only identified retirement-time observability failure is the existing P2 thread; no separate lock-order, deadlock, static-initialization, or ownership issue was substantiated.
- Configuration and compatibility: no new Doris configuration, FE-BE variable, storage format, transaction log, or write protocol was added. The pinned archive checksum matches the configured value; the source directory is revision-specific. The pinned C API plus the retained Foyer patch retain the Doris-used scanner, FTS, scalar-segment, multivector, session, and cache entry points. The retained patch follows the pinned ObjectStore wrapper interface. Static inspection found no distinct rolling-upgrade or parallel-path regression.
- Correctness, conditional checks, and parallel paths: the selected segment's complete-fragment coverage gates the prefilter shortcut, while deletion and visibility handling remain in Lance's prefilter path. Vector, FTS, ordinary/scalar, cached/uncached, and multi-split paths were checked against the changed integration. No data-visibility, delete, or error-propagation defect was substantiated. Doris data writes and persistence are outside the changed scan path.
- Performance and observability: the named index-stage metrics match the pinned producer names and nanosecond units; ANN, sort/merge, take, and vector-distance operator baselines map to the execution-summary keys. Timers may overlap and are documented as cumulative. The upstream optimization avoids a redundant row-ID allowlist only when complete segment coverage proves it safe. No additional hot-path allocation or I/O regression was substantiated. The known early-stop profile gap is noted above.
- Validation and focus: this was static inspection only, as required by the review instructions; no Doris build, test, source edit, or runtime benchmark was performed. I independently checked the live base/head, the pinned archive MD5, source/API contracts, every changed path, and current inline comments. The user supplied no additional review focus.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Problem Summary: HTTP response extensions prevented immutable object metadata from entering the Foyer metadata cache. Every cached single-range read could therefore issue another origin HEAD. Cache ObjectMeta and Attributes regardless of transport extensions, without caching or replaying the extensions themselves. Fingerprint the retained patch so previously patched third-party source trees are refreshed when the patch changes. Preserve reuse for identical patches. ### Release note Avoid redundant metadata requests for cached Lance data-file reads. ### Check List (For Author) - Test: Real HTTP regression and Foyer unit tests; third-party downloader lifecycle checks; Rust formatting, shell syntax, and diff checks. - Behavior changed: Yes, cached immutable metadata avoids repeated origin HEAD requests. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review complete for head 065e490a0116b3d3c78978cb3c805d0a3a56f6ce after two convergence rounds, two normal coverage passes per round, a separate risk-focused pass per round, and a final sweep of all 15 changed paths. I found one new P2 test-contract issue, posted inline. The previously reported P2 at comment 4132705390 still applies: Lance publishes these timings only when a scan reaches EOF, so early LIMIT or cancellation can leave the new profile timers zero. I did not repost it. No existing P0/P1 inline finding was confirmed. There were no additional user focus points.
Critical checkpoints:
- Goal and proof: the PR pins upstream Lance search/prefilter fixes, retains the Foyer data cache, and exposes ANN stage timings. The pinned archive checksum, Doris-reachable C symbols, metric names, and nanosecond units matched the selected dependency. The added BE test exercises indexed multivector output and profile counters, but its new positive-duration assertion is stronger than the producer contract. The shell test covers extraction and marker states. This review did not execute tests or a Doris build, so runtime integration and performance remain unverified here.
- Scope and paths: the changes are focused on the Lance dependency, retained cache patch, profile wiring, test, and documentation. Scalar, FTS, multivector, ordinary scan, and row-ID fetch interactions were checked. The removed local patch chain's Doris-reachable API surface is present in the pinned source or retained Foyer patch. No new FE/BE transmitted variable, storage format, or Doris configuration item is introduced.
- Concurrency and lifecycle: ANN partition timers accumulate concurrent work and overlap parent stages; the profile documentation states that correctly. The scanner callback, stream EOF/close, dataset/session lifetime, and Foyer wrapper map and metadata cache were traced. No new lock-order or static-initialization issue was substantiated. The known EOF-only callback lifecycle defect remains the existing P2 thread.
- Guards, compatibility, and failure behavior: the Foyer metadata cache applies to unconditioned, unversioned immutable data-file reads and keys by store prefix and path; invalid ranges delegate to the origin. Patch fingerprints refresh changed cached source trees, and the archive name and MD5 match the pin. No additional correctness or compatibility failure was substantiated in the reviewed branches.
- Tests, observability, and performance: one valid zero-duration stage can fail the new BE test; the inline comment gives the correction. The new counters otherwise map to emitted metrics, with nested timing semantics documented. Metadata caching aims to avoid repeated HEAD requests; no independent benchmark or test run was performed. No Doris transaction, EditLog, managed data-write, or persistence path is changed. No other actionable issue remained after the final sweep.
| "LanceIndexDistanceTopKTime", "LanceIndexResultMaterializeTime"}) { | ||
| auto* counter = profile.get_counter(name); | ||
| ASSERT_NE(nullptr, counter) << name; | ||
| EXPECT_GT(counter->value(), 0) << name; |
There was a problem hiding this comment.
[P2] Check ANN timer emission without requiring every duration to be positive. The pinned lance-c timing test explicitly allows zero nanoseconds for short or uncontended stages; this four-result indexed scan requires six independent timers to be > 0. A valid zero CPU queue wait or fast stage therefore fails the BE test even when the metrics and results are correct. Assert presence and unit/kind, or use a workload with a guaranteed measurable stage for any positive-value check.
|
run buildall |
|
/review |
### What problem does this PR solve? Related PR: apache#68613 Problem Summary: Identical bucket/path names on distinct storage endpoints could reuse cached metadata or bytes. Warm range reads also repeatedly enqueued size records to disk. Regenerate the Foyer patch from its reviewed source fix and add macOS nounset coverage to the patch harness. ### Release note Cache entries are isolated by live object-store instance. New instances and process restarts start cold; same-instance hot reads avoid redundant size writes. ### Check List (For Author) - Test: 461 Rust tests; generated source equality; GNU patch and git apply; downloader lifecycle, shell syntax, and simulated macOS platform checks. - Behavior changed: Yes, cache isolation and warm-read disk write behavior. - Does this need documentation: Yes, included in the upstream README patch.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Complete static review of #68615 at head 4c1f03e50960bb9297b0cfb251cf01d9968f6e32. I found one new P2 performance regression in the Foyer cache namespace. The two existing P2 inline threads (ANN timings lost on early stop, and strict-positive timer assertions) still apply and are not repeated. I found no existing P0/P1 findings to carry forward. Two complete review rounds and the final 15-file sweep found no other substantiated new issue.
Critical checkpoints:
- Goal and proof: The PR replaces the old Lance C patch stack with a pinned source and Foyer patch, and adds ANN profile counters and documentation. The pinned source retains the removed C APIs and the new timer names/units match its Lance dependency. The default cache does not achieve warm reuse across ordinary fresh dataset opens (inline finding). The new repeated-open test expects remote reads on its second handle, so it does not prove that reuse.
- Scope: The pin, patch replacement, download handling, BE profile, tests, and documentation form one dependency update. All 15 changed paths and their relevant call chains were reviewed; no separate scope defect was substantiated.
- Concurrency: Scanner metric callbacks and shared-session cache attachment were traced. The new origin map serializes namespace insertion with a mutex; the reviewed paths showed no separate race, lock-order, or blocking-I/O-under-lock issue.
- Lifecycle: Lance's registry retains weak outer object-store references, while wrapper attachment replaces the dataset and Refs stores. Fresh Doris reader opens therefore get new cache namespaces even while an earlier wrapped handle lives (inline finding). Same-live-handle and derived-handle reads can still reuse entries. No separate reference cycle or release failure was found.
- Configuration: No new FE/BE configuration item or dynamic-setting path is introduced. The existing data-cache setting is enabled by default with a 100 GB disk tier, making the missed cross-open reuse material.
- Compatibility: The pinned archive checksum, patch preimages, static library, C API symbols, and Lance dependency revision were checked against the removed patch chain and Doris callers. No new FE/BE protocol or Doris storage-format change was found. The local persisted cache-key behavior is covered by the inline finding; no other rolling-upgrade issue was substantiated.
- Parallel paths and conditions: Normal, multi-vector, and prepared-FTS scanners share the metric registration path. Cache direct/range, metadata/size, derived wrapper, and paginated-list paths were reviewed; conditional/versioned metadata requests bypass the cache as intended. The patch checksum marker handles normal fresh, reused, stale, and failed patch states. No separate condition or parallel-path issue was substantiated.
- Tests and results: The new BE assertions, Rust cache tests, shell harness, and documentation examples were inspected. The reopened-handle cache test reinforces the inline finding; the strict-positive BE timer assertion is already reported in an existing thread. No builds or tests were run, as required by this review task, so runtime behavior is not independently verified.
- Observability: New ANN timer names, nanosecond units, and additive profile mappings agree with the pinned dependency. The missing early-stop publication is already reported in an existing thread. No additional logging or metric issue was substantiated.
- Transactions, persistence, and writes: This PR changes no Doris transaction, EditLog, data-write, or FE/BE variable-transfer path. Foyer disk-cache persistence is affected by the fresh namespace finding; the third-party source refresh writes its success marker only after patching succeeds.
- Performance and remaining issues: The cross-open Foyer miss is the sole accepted new finding. A short-block assembly concern was present in the renamed predecessor patch and is outside the changed hunks; the independent risk reviews found no further reachable new defect.
User review focus: none was supplied. This review is complete for the pinned head, based on static inspection only.
| + let reader = DataCacheReader { | ||
| + cache: self.cache.clone(), | ||
| + store_prefix: store_prefix.to_owned(), | ||
| + store_prefix: format!("{}\0{store_prefix}", origin_namespace(&original)), |
There was a problem hiding this comment.
[P2] Keep Foyer keys reusable across dataset opens. origin_namespace(&original) adds a random UUID for each object-store instance, but Dataset::with_object_store_wrappers replaces the registry's store and the shared registry retains only a weak reference. The next Doris reader therefore gets a new origin/UUID and misses every data and size block admitted by the previous query, including persisted disk entries; the added repeated-open test even expects remote reads on its second handle. With the data cache enabled by default, ordinary warm queries cannot benefit from its 100 GB disk tier. Keep a verified backend identity or shared origin stable across opens, and test a close/reopen warm scan.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
3 participants
Unfiltered ANN searches scoped to complete index segments can materialize redundant row-ID allowlists, and the existing scan profile leaves the remaining search work unexplained. Update the upstream dependency and expose ANN stage timings on
branch-4.1.This includes the dependency integration from #68613: adopt the merged lance-format/lance#9599 and lance-format/lance#9602 through lance-format/lance-c#89, retain the merged lance-format/lance#9537 PQ scoring fix, remove superseded local lance-c patches, and retain only the Foyer cache integration.
Add BE profile counters for index opening, partition loading/preparation, prefilter readiness, CPU queue wait, partition search, query lookup-table preparation, fused distance/TopK work, and result materialization. Export operator baselines for ANN, sort/merge, take, and vector-distance work. Extend the existing indexed multivector regression to check that stage timers reach the Doris profile, alongside existing result and prefilter assertions.
Timings accumulate across concurrent work and overlap parent stages. Distance/TopK includes candidate filtering in fused paths; these counters must not be summed to reconstruct wall time.
docs/lance-ann-profile.mddocuments the boundaries, supported paths, and relationship to the existing scanner and prefilter timers. The BE changes add observability; the prefilter optimization itself remains in the upstream dependency.Pin lance-c
9bd730add2ac70316c1d642b8459011e2dd92022, using the merged Lance #9602 revision68c12dfd7efe02f90ad2d7f3a239a7eb64884e57. This includes parallel-path partition-preparation timing. The upstream callback regression checks timer presence and nanosecond units while accepting valid zero durations.The Foyer wrapper caches immutable ObjectMeta and Attributes independently of HTTP response extensions, avoiding repeated HEAD requests on warm range reads. It does not replay transport extensions from cached metadata. The HTTP regression checks HEAD/GET counts, returned bytes/ranges/metadata, fresh dataset scopes, and explicit HEAD bypass.
Generate the retained Foyer patch from source commit
24c7ca4bcb9422c113b0d3e07e4efe1173b0bc9frelative to the pinned lance-c baseline. The header records both revisions and the regeneration command. The source is submitted as zhangstar333/lance-c#3 against the branch behind lance-format/lance-c#73, on top of merged source-alignment PR #2. Doris consumes the exact reviewed source revision while that supplementary PR awaits merge.Scope metadata, size, and block keys to a random namespace for each live underlying object-store instance. The wrapping API omits a complete stable backend identity; identical bucket/path names alone cannot distinguish S3-compatible endpoints. Weak identity records preserve sharing for the same live store without retaining it, while new stores and process restarts start cold. A fresh Dataset open that creates a new store consequently needs to warm its own cache. This deliberately reduces reuse across instances to prevent returning another origin's data; it does not claim unchanged cache-hit rates or production latency.
Avoid inserting an unchanged size entry into the WriteOnInsertion hybrid cache: look up both tiers first and populate only absent or invalid size records. Regression tests cover real HTTP endpoints sharing bucket/path/ETag, batched data and NotFound isolation, replaced origins after disk recovery, weak-reference lifetime, and actual disk-write bytes. Five warm range reads wrote 40 KB before the fix and zero bytes after it in the regression.
Fingerprint the patch in the downloader so existing source trees refresh after updates, including legacy empty markers. Identical patches reuse cached sources. Check platform definitions under nounset with simulated Darwin x86_64/arm64, and make the optional ADBC source guard safe when unset on master. Downloader lifecycle and platform handling remain downstream in Doris.
Validation: 461 regular Rust tests passed on the final source; Rust formatting and diff checks passed. GNU patch and git apply checks passed, and all 87 tracked source files match the recorded source commit. Downloader tests passed on master, branch-4.1, and the downstream hotfix branch for fresh extraction, idempotence, re-extraction, generic markers, legacy/mismatched Foyer markers, and patch failure. Shell syntax and simulated macOS initialization passed. All three opt-in native consumer tests passed: C calls, C++ calls, and static OSS HTTP transport. Full Doris builds and BE integration execution remain in PR CI.