feat(fts): record scorer_build_ms for the exec-local BM25 scorer fallback#7807
Conversation
…back When no preset base scorer is injected, Match/Boolean/Phrase FTS execs build one from every segment they search (build_global_bm25_scorer) — per-segment corpus stats plus per-term doc-frequency reads whose cost was folded invisibly into the node's elapsed time. Record it as a scorer_build_ms gauge on FtsIndexMetrics; zero when a preset scorer is supplied. Co-Authored-By: Claude Fable 5 <[email protected]>
📝 WalkthroughWalkthroughChangesFTS scorer metrics
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@rust/lance/src/io/exec/fts.rs`:
- Around line 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.
- 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.
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2aa39966-b5f2-4d76-bcf3-577f51337755
📒 Files selected for processing (1)
rust/lance/src/io/exec/fts.rs
| .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), |
There was a problem hiding this comment.
🎯 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 80Repository: 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:
- 1: https://docs.rs/datafusion/latest/datafusion/physical_expr_common/metrics/struct.MetricBuilder.html
- 2: https://docs.rs/datafusion-physical-plan/latest/datafusion_physical_plan/metrics/index.html
- 3: https://docs.rs/datafusion/latest/datafusion/physical_plan/struct.Metric.html
- 4: https://docs.rs/datafusion-physical-plan/latest/datafusion_physical_plan/metrics/enum.MetricCategory.html
- 5: https://docs.rs/datafusion-common/latest/datafusion_common/format/enum.MetricCategory.html
- 6: https://github.com/apache/datafusion/blob/main/datafusion/physical-plan/src/aggregates/row_hash.rs
- 7: https://docs.rs/datafusion-physical-plan/latest/src/datafusion_physical_plan/analyze.rs.html
🏁 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)
PYRepository: 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.rsRepository: 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.
| pub fn record_scorer_build(&self, elapsed: std::time::Duration) { | ||
| self.scorer_build_ms.set(elapsed.as_millis() as usize); | ||
| } |
There was a problem hiding this comment.
📐 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
| None => { | ||
| let scorer_start = std::time::Instant::now(); | ||
| let scorer = Arc::new( | ||
| build_global_bm25_scorer(&indices, &tokens, ¶ms) | ||
| .boxed() | ||
| .await?, | ||
| ); | ||
| metrics.record_scorer_build(scorer_start.elapsed()); | ||
| scorer | ||
| } |
There was a problem hiding this comment.
📐 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' || trueRepository: 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.rsRepository: 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.rsRepository: 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.rsRepository: lance-format/lance
Length of output: 4158
Centralize scorer timing and add coverage for the override paths.
MatchQueryExec,FlatMatchQueryExec, andPhraseQueryExecrepeat the sameInstant::now→build_global_bm25_scorer→record_scorer_buildblock; extract a helper so the metric stays consistent.- Add tests for the injected-scorer branch (
scorer_build_ms == 0) and theFlatMatchQueryExecno-segments branch; the current suite only exercisesMatchQueryExec’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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
CI note: the only failing job ( 🤖 Generated with Claude Code |
…back (#7807) ## Summary When no preset base scorer is injected, the FTS exec nodes (`MatchQueryExec` / `BooleanQueryExec` / `PhraseQueryExec`) build one via `build_global_bm25_scorer` over every segment they search — per-segment corpus stats plus per-term doc-frequency reads. That cost was folded invisibly into the node's `elapsed`, which makes it impossible to tell "search was slow" from "scorer construction was slow" in EXPLAIN ANALYZE. Adds a `scorer_build_ms` gauge to `FtsIndexMetrics` timing exactly that fallback at all three call sites. It reads 0 when a caller supplies a preset scorer (e.g. a distributed engine shipping corpus-wide stats via `with_base_scorer`), so the gauge also doubles as a visible marker of which scoring path a node took. ## Test plan - `cargo check -p lance` / `cargo fmt` clean - Verified end-to-end in a distributed setup (100M-row dataset, 4 index segments over 4 workers): with no preset scorer each worker's `MatchQuery` line reports its own `scorer_build_ms` (non-zero on cold caches, ~0 warm); with a preset scorer the gauge stays 0 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added performance metrics to track the time required to build BM25 scorers during full-text search. * **Monitoring** * Search execution metrics now report scorer construction duration when a fallback scorer is created. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Fable 5 <[email protected]> (cherry picked from commit 3ece36f)
2 participants
Summary
When no preset base scorer is injected, the FTS exec nodes (
MatchQueryExec/BooleanQueryExec/PhraseQueryExec) build one viabuild_global_bm25_scorerover every segment they search — per-segment corpus stats plus per-term doc-frequency reads. That cost was folded invisibly into the node'selapsed, which makes it impossible to tell "search was slow" from "scorer construction was slow" in EXPLAIN ANALYZE.Adds a
scorer_build_msgauge toFtsIndexMetricstiming exactly that fallback at all three call sites. It reads 0 when a caller supplies a preset scorer (e.g. a distributed engine shipping corpus-wide stats viawith_base_scorer), so the gauge also doubles as a visible marker of which scoring path a node took.Test plan
cargo check -p lance/cargo fmtcleanMatchQueryline reports its ownscorer_build_ms(non-zero on cold caches, ~0 warm); with a preset scorer the gauge stays 0🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Monitoring