perf: avoid redundant prefilters for complete ANN segments - #89
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.
### 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
### 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
yanghua
left a comment
There was a problem hiding this comment.
Please run the C/C++ static-link tests for this dependency update.
This changes the exact OpenDAL version and moves the complete Lance dependency graph from 11.x to 13.x, both of which are linked into the exported staticlib. However, the PR description explicitly says the three opt-in C/C++ compilation tests were not run. Rust tests cannot detect missing native symbols, static initialization, or HTTP transport registration failures in an actual C/C++ consumer.
Please run cargo test --test compile_and_run_test -- --ignored on the supported target(s), or add the equivalent CI result, before merging.
| lance_scanner_set_fragment_ids( | ||
| scanner, | ||
| fragments.as_ptr(), | ||
| fragments.len(), | ||
| ) | ||
| }, | ||
| 0 | ||
| ); | ||
| assert_eq!( | ||
| unsafe { lance_scanner_set_index_segments(scanner, uuid.as_ptr(), 1) }, | ||
| 0 | ||
| ); | ||
| assert_eq!(unsafe { lance_scanner_set_prefilter(scanner, true) }, 0); | ||
| let query = [0.0_f32; 8]; | ||
| assert_eq!( | ||
| unsafe { | ||
| lance_scanner_nearest( | ||
| scanner, | ||
| c_str("embedding").as_ptr(), | ||
| query.as_ptr().cast(), |
There was a problem hiding this comment.
Avoid requiring every stage timer to be non-zero.
The test is intended to verify that dynamic metrics cross the C callback with the nanosecond unit, but value > 0 additionally assumes every stage consumes at least one observable clock tick. In particular, index_cpu_queue_wait_time can legitimately be zero when there is no queue contention, and short cached stages can also round to zero on some platforms.
Please assert that each metric is present with TimeNanoseconds; only require a positive value for a deliberately controlled operation, or aggregate repeated executions before checking positivity.
There was a problem hiding this comment.
Fixed in abba808. The callback regression now checks that each named metric is present with TimeNanoseconds and accepts zero durations. Exact-result and prefilter materialization assertions remain intact. All 443 regular tests, all three opt-in native tests, Clippy, and formatting passed on the merged Lance revision.
|
Addressed review #89 (review) on abba808. The dependency now pins the merged Lance #9602 revision, 68c12dfd7efe02f90ad2d7f3a239a7eb64884e57. On Linux x86_64 with Rust 1.98.1,
|
LuciferYang
left a comment
There was a problem hiding this comment.
Three non-blocking observations from reviewing the dependency bump and the two new tests. All minor: two are about test-assertion strength, one about the pin target. The dependency pinning itself checks out (all crates on one rev, no stale rev left in the lock, and the segment-compat validation is still present upstream). Details inline.
| "prefilter_input_rows", | ||
| "prefilter_row_ids", | ||
| "prefilter_build_time", | ||
| "prefilter_load_time", |
There was a problem hiding this comment.
prefilter_build_time and prefilter_load_time are only ever asserted == 0 here, and the sum() runs over filter(name == ...), so a typo or an upstream rename makes the filter match nothing, the sum stays 0, and the assert passes vacuously. These two timers effectively aren't tested. The sibling counters prefilter_input_rows/prefilter_row_ids get a positive == 32 assertion in the full-snapshot test that pins their names; these two timers have none.
In a branch that does materialize (filtered, or !segmented), assert that the two metrics are present (.any(|(name, _, _)| name == ...)). That pins the names without relying on a specific timer value.
There was a problem hiding this comment.
Fixed in 9bd730a. Materializing cases now require both prefilter_build_time and prefilter_load_time to be present with TimeNanoseconds, without requiring positive durations. A temporary mutation dropping prefilter_build_time from the callback failed with the expected missing-metric assertion; after removing the mutation, all 443 regular tests, Clippy, and formatting passed.
| "ANNSubIndexExec_elapsed_compute", | ||
| "index_open_time", | ||
| "index_partition_load_time", | ||
| "index_partition_prepare_time", |
There was a problem hiding this comment.
Requiring index_cpu_queue_wait_time > 0 across all 16 combinations looks brittle. With small test data on an idle machine the CPU-dispatch permit is usually available immediately and nothing queues, so this wait can legitimately be 0; on a faster CI box the assert could fail intermittently. index_partition_load_time has the same risk once the cache is warm.
The timers that wrap real work (search, distance_topk, result_materialize) are fine to keep at > 0. For the pure wait/load timers, asserting the metric is present rather than > 0 removes a cross-machine flake source. This depends on Lance's timing semantics, so it's worth confirming whether the timer can be 0 before relying on it.
There was a problem hiding this comment.
This was already addressed in abba808: the ANN timing loop checks presence and TimeNanoseconds only, including queue/load timers. It no longer requires positive values.
| lance-table = { git = "https://github.com/lance-format/lance.git", rev = "db211492fc5cd9da5642d7234d9682de9be77c19" } | ||
| lance-datafusion = { git = "https://github.com/lance-format/lance.git", rev = "db211492fc5cd9da5642d7234d9682de9be77c19", features = ["substrait"] } | ||
| # Keep the dependency on the segment-prefilter fix and its float-consistent PQ prerequisite. | ||
| lance = { git = "https://github.com/lance-format/lance.git", rev = "68c12dfd7efe02f90ad2d7f3a239a7eb64884e57", features = ["substrait"] } |
There was a problem hiding this comment.
350351e is the pre-squash branch head of the now-merged PR lance-format/lance#9602 (Gabriel39:dev/ann-search-profile); it isn't on lance main (diverged: ahead 3 / behind 4). All three fixes it carries (#9537/#9599/#9602) are already on main, so pinning to a main ancestor would give the squashed, reviewed state and keep the dependency on main. Durability isn't the concern here: refs/pull/9602/head keeps 350351e fetchable even if the source branch is deleted. This is just tidiness.
Minor: the description says "including the merged lance-format/lance#9602", but 350351e is the pre-squash head, and the squash commit 68c12df isn't an ancestor of it. Pinning to a main commit would make that line accurate too.
There was a problem hiding this comment.
This was already addressed in abba808. Cargo.toml and Cargo.lock now consistently pin 68c12dfd7efe02f90ad2d7f3a239a7eb64884e57, the merged #9602 commit on Lance main; the PR description was updated accordingly.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The prefilter timing review point is addressed: materializing cases now require both prefilter build and load metrics with the nanosecond kind, so an absent metric cannot pass a zero-sum assertion. The complete, unfiltered path still checks for no row-ID allowlist, and the 16-case regression retains exact-result and loader-count checks. The focused test passes on this head. Zero-duration stages remain valid; these overlapping timings are diagnostics rather than additive latency.
The pin remains the merged Lance ANN timing revision, carrying the segment-prefilter fix and screened 4-bit PQ scorer without a C ABI change. The previous revision's 361 C API tests and native C/C++ and static-consumer checks remain applicable because this commit changes only the regression. The merged pin's additional FTS and opt-in row-lineage paths have dedicated upstream regressions.
…68613) Unfiltered ANN searches scoped to complete index segments can spend time materializing redundant row-ID allowlists. Update lance-c to an immutable upstream revision containing the merged Lance development-branch fix lance-format/lance#9599 and ANN diagnostics from lance-format/lance#9602, through lance-format/lance-c#89. Keep all master changes in `thirdparty/`. Remove the superseded local lance-c patch chain and retain the Foyer cache integration while its interface remains outside upstream main. The upstream revision also carries the merged lance-format/lance#9537 PQ scoring change and ANN stage timing diagnostics; no Doris FE or BE code changes are included here. The prefilter fast path requires complete visible coverage of every selected segment and no predicate. Partial coverage and predicates retain row-ID prefiltering, while deletions and unindexed fallback remain effective. Pin lance-c `9bd730add2ac70316c1d642b8459011e2dd92022`, using the merged Lance #9602 revision `68c12dfd7efe02f90ad2d7f3a239a7eb64884e57`. 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 `24c7ca4bcb9422c113b0d3e07e4efe1173b0bc9f` relative 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.
…nch-4.1) (#68615) 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.md` documents 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 revision `68c12dfd7efe02f90ad2d7f3a239a7eb64884e57`. 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 `24c7ca4bcb9422c113b0d3e07e4efe1173b0bc9f` relative 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.
Unfiltered ANN searches scoped to complete index segments can build redundant row-ID allowlists. Pin all Lance crates to the merged development-branch revision
68c12dfd7efe02f90ad2d7f3a239a7eb64884e57, containing lance-format/lance#9599, the PQ scoring fix in lance-format/lance#9537, and ANN stage timings in lance-format/lance#9602. Align the direct OpenDAL dependency with Lance.The complete-segment fast path preserves predicate, partial-coverage, deletion, stable-row-ID, and unindexed-tail behavior. The C API regression checks exact results and prefilter materialization counters across 16 combinations. The PQ regression compares unfiltered and all-row-filtered results for L2, Cosine, and Dot.
Verify that stage timings cross the existing dynamic statistics callback with the nanosecond unit. A reported zero duration is valid for short or uncontended stages, so the callback test checks metric presence and type without requiring every value to be positive. The C ABI is unchanged. Timers accumulate concurrent work and may overlap; they are not additive query latency.
The materializing cases also require both prefilter build/load timers to be present with the nanosecond unit. A temporary mutation removing the build timer failed as expected; the final test passed across all 16 combinations. All 443 regular tests, Clippy, and formatting were rerun for this test-only follow-up.
Validation on Linux x86_64 with Rust 1.98.1:
cargo test --locked: all 443 tests passed, including 361 C API tests.cargo test --locked --test compile_and_run_test -- --ignored: all three opt-in tests passed. These compile and execute real C and C++ API consumers against the shared library, plus a C consumer against the static archive. The static test verifies that both ordinary and shared-runtime OSS opens reach a local HTTP server from fresh processes, without relying on whole-archive linking.cargo clippy --locked --all-targets -- -D warningsandcargo fmt --all --checkpassed.The native result above is local Linux validation; macOS remains covered by the existing PR CI job. No production-data latency measurement is claimed.