perf: avoid cloning every fragment descriptor in take and FTS planning - #8102
Merged
Merged
Conversation
do_take_rows cloned all fragment descriptors via get_fragments() and then kept only the addressed ones; FTS match/phrase planning deep-copied the full fragment list per query via fragments().to_vec(). Both costs are O(dataset fragments) per call and dominate take/plan cost on datasets with many fragments (~1ms per call at 3150 fragments, capping a single-node take benchmark at ~1000 qps). Construct handles only for addressed fragments in do_take_rows (get_existing_fragments_from_ids preserves the skip-missing semantics of the previous filter_map), and borrow the fragment slice in plan_match_query/plan_phrase_query, materializing only for the no-index flat fallback.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
wkalt
approved these changes
Jul 30, 2026
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 2, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 5, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 6, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 10, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 10, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 12, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 14, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 14, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
sbrunk
added a commit
to sbrunk/lance
that referenced
this pull request
Aug 16, 2026
The composable scorer stack (lance-format#8092, lance-format#8093, lance-format#8094, lance-format#8131) added `compound.rs`, a single-pass `ComposableScorer` that runs a whole compound tree against one column's index with `Match`/`Phrase` leaves scored by a `MemBM25Scorer`. It matches `FtsQuery` exhaustively, so the `CombinedFields` variant has to be accounted for in multiple places. BM25F blends `tf'`, `dl'` and `docFreq'` across columns before scoring, so it cannot be a leaf that draws its postings from a single index; making it one needs a multi-index leaf protocol and block-max bounds out of the MAXSCORE scan. Gate it out instead, leaving those trees on the union/sort plan `plan_combined_fields_query` already builds: - `supports_compound_scorer` rejects any tree containing a `CombinedFields` node, at the top level or nested. - The two `compound.rs` entry points return `Error::not_supported` rather than `unreachable!()`, so a future planner change fails loudly instead of silently misscoring. - `collect_all_fts_columns` reports the real target columns, `contains_phrase_query` is false (BM25F needs no positions), `validate_fts_query_contract` has nothing to add (the weights are private and every constructor, `Deserialize` included, goes through `try_with_boosts`), and `count_fts_leaves` counts the per-column posting scans its partition estimate is approximating. `test_nested_combined_fields_limit_propagation` covers the gate: it nests a combined_fields node under MUST, SHOULD and BoostQuery, so a leak surfaces the `not_supported` error. Also apply lance-format#8102 to `plan_combined_fields_query`, which still deep-copied the fragment descriptor list on every plan. Borrow the slice like `plan_match_query` now does and materialize only the uncovered subset the flat plan takes ownership of.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two hot paths deep-copy the entire fragment descriptor list on every call, costing O(dataset fragments) regardless of how many fragments are actually touched:
do_take_rowscalleddataset.get_fragments()— cloning everyFragmentdescriptor (includingDataFilepath strings) — and then kept only the fragments addressed by the take.plan_match_query/plan_phrase_querymaterializedself.dataset.fragments().to_vec()per query, even though the list is only read.On a 126M-row dataset with 3,150 fragments this costs ~1 ms per call (~300 ns per descriptor clone+drop). Under a take-heavy workload both FTS plan build and the take pay it on every request, which showed up as a hard ~1,000 qps per-process ceiling with the machine >95% idle: thread dumps showed active threads dominated by
Fragment::clone/drop_in_place<DataFile>underScanner::plan_ftsanddo_take_rows.Fix
do_take_rows: constructFileFragmenthandles only for the addressed fragment ids viaget_existing_fragments_from_ids(same skip-missing semantics as the previousfilter_map).plan_match_query/plan_phrase_query: borrow the fragment slice (&[Fragment]); materialize only in the no-index flat fallback.