Skip to content

feat(fts): record scorer_build_ms for the exec-local BM25 scorer fallback #7807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jackye1995 merged 2 commits into from
Jul 15, 2026
Merged

feat(fts): record scorer_build_ms for the exec-local BM25 scorer fallback #7807

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bda225b
feat(fts): record scorer_build_ms for the exec-local BM25 scorer fall…
LuQQiu Jul 15, 2026
8562bf5
Merge branch 'main' into lu/fts_metrics
LuQQiu Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions rust/lance/src/io/exec/fts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use datafusion::error::{DataFusionError, Result as DataFusionResult};
use datafusion::execution::SendableRecordBatchStream;
use datafusion::physical_plan::empty::EmptyExec;
use datafusion::physical_plan::execution_plan::{Boundedness, EmissionType};
use datafusion::physical_plan::metrics::{ExecutionPlanMetricsSet, MetricsSet};
use datafusion::physical_plan::metrics::{ExecutionPlanMetricsSet, Gauge, MetricsSet};
use datafusion::physical_plan::repartition::RepartitionExec;
use datafusion::physical_plan::stream::RecordBatchStreamAdapter;
use datafusion::physical_plan::union::UnionExec;
Expand Down Expand Up @@ -166,6 +166,9 @@ pub struct FtsIndexMetrics {
and_candidates_pruned_before_return: Count,
and_full_scores: Count,
freqs_collected: Count,
/// Wall time (ms) of the exec-local `build_global_bm25_scorer`
/// fallback; zero when a preset base scorer was injected.
scorer_build_ms: Gauge,
baseline_metrics: BaselineMetrics,
}

Expand All @@ -179,13 +182,18 @@ impl FtsIndexMetrics {
.new_count(AND_CANDIDATES_PRUNED_BEFORE_RETURN_METRIC, partition),
and_full_scores: metrics.new_count(AND_FULL_SCORES_METRIC, partition),
freqs_collected: metrics.new_count(FREQS_COLLECTED_METRIC, partition),
scorer_build_ms: metrics.new_gauge("scorer_build_ms", partition),

@coderabbitai coderabbitai Bot Jul 15, 2026

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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target file around the referenced lines
sed -n '160,210p' rust/lance/src/io/exec/fts.rs

echo
echo "---- search for metric registrations ----"
rg -n "new_gauge|MetricBuilder|MetricCategory|scorer_build_ms" rust/lance/src/io/exec/fts.rs rust/lance/src -S

echo
echo "---- locate DataFusion dependency versions ----"
rg -n 'datafusion|datafusion-physical-plan' Cargo.toml Cargo.lock rust -S | head -n 80

Repository: lance-format/lance

Length of output: 8368

🌐 Web query:

DataFusion MetricBuilder with_category MetricCategory::Timing new_gauge uncategorized EXPLAIN ANALYZE timing filter Gauge custom metric

💡 Result:

In Apache DataFusion, the MetricBuilder is used to construct metrics, such as counters, gauges, and timers, within execution plans [1][2]. When creating a metric, you can assign it a semantic category using the.with_category method, which accepts a MetricCategory enum [3][4]. The MetricCategory enum classifies metrics to allow for selective filtering during EXPLAIN ANALYZE output [4][5]. The available categories are: Rows: Metrics tracking row counts [4]. Bytes: Metrics tracking data volume [4]. Timing: Metrics measuring wall-clock time, such as elapsed_compute [4]. Uncategorized: The default category for custom Count or Gauge metrics that are not explicitly assigned a specific category [4][5]. When implementing a custom gauge, you can use MetricBuilder to initialize it and assign it a category [3][6]. For example, if you wanted to explicitly categorize a custom timing metric, you would use MetricCategory::Timing [4]. If no category is specified during construction, the metric defaults to Uncategorized [4][5]. The EXPLAIN ANALYZE command uses these categories to filter which metrics are displayed in the query plan output [4][7]. Users can control this via configuration (e.g., setting datafusion.explain.analyze_categories), which determines whether metrics assigned to categories like Timing or Uncategorized are shown [4][5]. This is particularly useful for excluding non-deterministic timing metrics in regression tests [4][5]. Example usage pattern: MetricBuilder::new(&metrics).gauge("my_custom_metric", partition).with_category(MetricCategory::Timing) // Categorizing as Timing.build;

Citations:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the relevant DataFusion metric API usage from the local dependency cache if present via Cargo.lock only.
# No code execution; just inspect lockfile for exact version.
python3 - <<'PY'
from pathlib import Path
lock = Path('Cargo.lock')
if lock.exists():
    text = lock.read_text()
    for pkg in ['datafusion', 'datafusion-physical-plan']:
        print(f'[{pkg}]')
        for line in text.splitlines():
            if line.startswith(f'name = "{pkg}"'):
                print(line)
PY

Repository: lance-format/lance

Length of output: 250

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "---- timing/category usage in exec metrics ----"
rg -n "with_category\\(MetricCategory::Timing\\)|MetricCategory::Timing|MetricBuilder::new\\(metrics\\)" rust/lance/src/io/exec rust/lance/src -S

echo
echo "---- read neighboring metric setup in utils.rs for style/reference ----"
sed -n '240,380p' rust/lance/src/io/exec/utils.rs

Repository: lance-format/lance

Length of output: 6115

Classify scorer_build_ms as timing.
new_gauge("scorer_build_ms", partition) leaves this wall-time metric uncategorized, so timing-filtered EXPLAIN ANALYZE output can omit it. Use MetricBuilder with MetricCategory::Timing.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/io/exec/fts.rs` at line 185, Update the metric initialization
for scorer_build_ms to use MetricBuilder and explicitly assign
MetricCategory::Timing, ensuring it remains visible in timing-filtered EXPLAIN
ANALYZE output. Preserve the existing metric name and partition association.

baseline_metrics: BaselineMetrics::new(metrics, partition),
}
}

pub fn record_parts_searched(&self, num_parts: usize) {
self.partitions_searched.add(num_parts);
}

pub fn record_scorer_build(&self, elapsed: std::time::Duration) {
self.scorer_build_ms.set(elapsed.as_millis() as usize);
}
Comment on lines +194 to +196

@coderabbitai coderabbitai Bot Jul 15, 2026

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.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Document the new public metric API.

record_scorer_build is a new pub method but has no rustdoc describing milliseconds, per-partition scope, or when the value remains zero. Add documentation with a usage example or link to the relevant metrics API.

As per coding guidelines, document all public APIs with examples and links to relevant structs and methods.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/io/exec/fts.rs` around lines 194 - 196, Add rustdoc to the
public record_scorer_build method describing that it records scorer-build time
in milliseconds for the current partition and remains zero until recorded.
Include a concise usage example or link to the relevant metrics API, following
the documentation style used by nearby public metrics methods.

Source: Coding guidelines

}

impl MetricsCollector for FtsIndexMetrics {
Expand Down Expand Up @@ -521,11 +529,16 @@ impl ExecutionPlan for MatchQueryExec {
let tokens = collect_query_tokens(&query.terms, &mut tokenizer);
let base_scorer = match preset_base_scorer {
Some(scorer) => scorer,
None => Arc::new(
build_global_bm25_scorer(&indices, &tokens, &params)
.boxed()
.await?,
),
None => {
let scorer_start = std::time::Instant::now();
let scorer = Arc::new(
build_global_bm25_scorer(&indices, &tokens, &params)
.boxed()
.await?,
);
metrics.record_scorer_build(scorer_start.elapsed());
scorer
}
Comment on lines +532 to +541

@coderabbitai coderabbitai Bot Jul 15, 2026

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.

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect relevant portions of the file and nearby tests/usage.
git ls-files rust/lance/src/io/exec/fts.rs rust/lance/src | sed -n '1,20p'
echo "--- outline fts.rs ---"
ast-grep outline rust/lance/src/io/exec/fts.rs --view expanded || true
echo "--- snippets around target lines ---"
sed -n '500,560p' rust/lance/src/io/exec/fts.rs
echo "--- more snippets around other cited lines ---"
sed -n '1060,1120p' rust/lance/src/io/exec/fts.rs
echo "--- and around third cited lines ---"
sed -n '1360,1425p' rust/lance/src/io/exec/fts.rs
echo "--- tests / scorer metrics references ---"
rg -n "record_scorer_build|build_global_bm25_scorer|FlatMatchQueryExec|scorer" rust/lance/src/io/exec/fts.rs rust/lance/tests rust/lance/src -g '!target' -g '!dist' || true

Repository: lance-format/lance

Length of output: 42474

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '2320,2485p' rust/lance/src/io/exec/fts.rs
echo "---"
sed -n '2485,2635p' rust/lance/src/io/exec/fts.rs
echo "--- search for no-segment / fallback coverage ---"
rg -n "no segments|without segments|preset scorer|base_scorer|with_base_scorer|build_scorer|FlatMatchQueryExec::new|FlatMatchQueryExec::new_with_segments|MatchQueryExec::new_with_segments" rust/lance/src/io/exec/fts.rs

Repository: lance-format/lance

Length of output: 15785

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '2170,2355p' rust/lance/src/io/exec/fts.rs
echo "---"
rg -n "scorer_build_ms|record_scorer_build|ExecutionPlanMetricsSet|baseline_metrics" rust/lance/src/io/exec/fts.rs

Repository: lance-format/lance

Length of output: 10241

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "FlatMatchQueryExec|PhraseQueryExec|with_base_scorer|scorer_build_ms|no segments|preset scorer|base_scorer" rust/lance/src/io/exec/fts.rs

Repository: lance-format/lance

Length of output: 4158

Centralize scorer timing and add coverage for the override paths.

  • MatchQueryExec, FlatMatchQueryExec, and PhraseQueryExec repeat the same Instant::now → build_global_bm25_scorer → record_scorer_build block; extract a helper so the metric stays consistent.
  • Add tests for the injected-scorer branch (scorer_build_ms == 0) and the FlatMatchQueryExec no-segments branch; the current suite only exercises MatchQueryExec’s override path.
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/io/exec/fts.rs` around lines 532 - 541, The scorer
construction and timing logic is duplicated across MatchQueryExec,
FlatMatchQueryExec, and PhraseQueryExec; extract a shared helper that builds the
global BM25 scorer, records scorer-build timing, and is reused by each execution
path. Add coverage for injected scorers with scorer_build_ms == 0 and for
FlatMatchQueryExec when no segments are available, alongside the existing
MatchQueryExec override test.

Source: Coding guidelines

};

pre_filter.wait_for_ready().await?;
Expand Down Expand Up @@ -1066,13 +1079,16 @@ impl ExecutionPlan for FlatMatchQueryExec {
Some(scorer) => (*scorer).clone(),
None => {
let query_tokens = collect_query_tokens(&query.terms, &mut tokenizer);
build_global_bm25_scorer(
let scorer_start = std::time::Instant::now();
let scorer = build_global_bm25_scorer(
&indices,
&query_tokens,
&FtsSearchParams::new(),
)
.boxed()
.await?
.await?;
metrics.record_scorer_build(scorer_start.elapsed());
scorer
}
};
(tokenizer, Some(base_scorer))
Expand Down Expand Up @@ -1379,11 +1395,16 @@ impl ExecutionPlan for PhraseQueryExec {
let tokens = collect_query_tokens(&query.terms, &mut tokenizer);
let base_scorer = match preset_base_scorer {
Some(scorer) => scorer,
None => Arc::new(
build_global_bm25_scorer(&indices, &tokens, &params)
.boxed()
.await?,
),
None => {
let scorer_start = std::time::Instant::now();
let scorer = Arc::new(
build_global_bm25_scorer(&indices, &tokens, &params)
.boxed()
.await?,
);
metrics.record_scorer_build(scorer_start.elapsed());
scorer
}
};

pre_filter.wait_for_ready().await?;
Expand Down
Loading