feature(fts): add the search phase to the full-text benchmark - #15719
Draft
m-szymon wants to merge 12 commits into
Draft
feature(fts): add the search phase to the full-text benchmark#15719m-szymon wants to merge 12 commits into
m-szymon wants to merge 12 commits into
Conversation
2 tasks
Collaborator
✅ Test Summary: PASSED✅ Precommit: PASSED
✅ Tests: PASSED
|
m-szymon
changed the title
2 tasks
|
@m-szymon new branch |
Contributor
|
@claude review |
|
Claude finished @dimakr's task in 6m 23s —— View job Code Review: feature(fts): add the search phase to the full-text benchmark
SummaryThe PR is well-structured and implements a clean search performance flow. The framework additions ( Issues1.
|
m-szymon
force-pushed
the
pr/fts-search
branch
from
August 14, 2026 07:25
9a9bca2 to
0a4e0dd
Compare
…ient 'get_index_status' raises on the 404 vector-store returns for an index it has not discovered yet, which makes "not there yet" indistinguishable from a real error at the call site. 'get_index_status_or_none' returns None for that one case and re-raises everything else, so a caller can poll an index across the discovery gap in either direction -- waiting for a fresh index to appear, or for a dropped one to disappear again. 'wait_for_index_absent' is that second direction, which every caller recreating an index needs: vector-store forgets a dropped index asynchronously, so without the wait a 'CREATE CUSTOM INDEX' can race a drop ScyllaDB already considers done. A failed request counts as "still there", since mistaking a network hiccup for a completed drop would let the next build start too early. 'wait_for_ready' grows a 'required_statuses' argument. Its default still accepts BOOTSTRAPPING, which is what cluster readiness means here -- the service answers and is catching up -- but a caller that measures index build time or query latency needs the node actually serving first, and can now ask for that.
Vector-store logs both ends of an index's initial table scan ("starting/finished
full scan on <keyspace>.<index>") with a microsecond tracing timestamp. That scan
is the index build, so those two lines are the most direct and most precise
measurement of it available -- latte's clock has whole-second resolution, and
vector-store's index-status endpoint serves a snapshot refreshed on a ~1s ticker,
so polling either observes each edge late.
'wait_for_index_build_seconds' reads them off the node's log once the build is
over, retrying for a bounded window because the lines reach the runner
asynchronously, and returning None rather than raising when they never arrive:
the index is queryable either way, so a missing measurement should not fail a
run. Two log shapes are handled, since 'BaseNode.system_log' resolves to the raw
tracing line on docker and to the same line behind a log-shipper prefix on aws.
'index_key' folds the case the way ScyllaDB folds unquoted identifiers, which is
how vector-store knows an index -- 'fts_idx_10M_20tok_0' is 'fts_idx_10m_20tok_0'
downstream, and matching without folding silently never matches.
Argus reporting lives here too: 'index_build_columns' and
'send_index_build_result' report build time, how much was indexed and the
throughput that follows from the two. The count column is named by the caller in
its own vocabulary (documents, vectors), and the table itself is declared by the
test that owns it, so its name and description stay next to that test.
None of this is specific to full-text search: it applies to any index
vector-store serves, because the measurement comes from vector-store's own log
rather than from whatever issued the DDL.
…tore The two helpers of the previous commits are unit tested against mocks and captured log lines, which leaves exactly the parts that can rot unchecked: whether a missing index really answers 404, whether a dropped one is really forgotten, and whether vector-store still writes its "starting/finished full scan" lines in the shape the parser expects and still reaches the runner's copy of the log. Two integration tests answer those against a live ScyllaDB and vector-store, using the docker fixtures the existing vector-store tests already use. Only the deterministic states are asserted -- before an index exists and after it is dropped -- rather than racing vector-store's discovery window, which would be flaky. They run against a vector index, because that is what the released images support and it keeps the tests runnable anywhere the other vector-store integration tests are. Neither helper knows which kind of index it is looking at: the full-text tests call exactly these two, and this is also the evidence for that claim.
'fts.rn' drives the BM25 workload: the schema and index DDL, the document load, the index build (which waits for the index to become queryable and reports the build time and the indexing throughput as latte custom metrics) and the search phase. 'metrics.rn' holds the IR accuracy metrics -- recall, precision, NDCG and friends -- the search phase reports against the qrels. Both are copies of the scripts in scylladb/vector-store's latte/full-text-search/. They have to live here because 'LatteStressThread' resolves '.rn' paths inside the SCT tree and copies the containing directory into the loader container, so a script cannot be loaded from another repo. Keep them a mirror: changes belong upstream first, and the custom metrics stay declared even where SCT reads none of them. That is also why the search function and metrics.rn arrive before anything calls them -- trimming the scripts to what SCT uses today would only create a divergence to undo.
The orchestration a search benchmark needs is the same whichever index is under test, because vector-store serves both a full-text and a vector index and latte drives both: a YAML plan names datasets, a dataset is a sequence of steps, and a step loads more shards on top of the previous ones and rebuilds the index -- so one dataset yields results at several corpus sizes. Index build time comes from vector-store's log and every row is streamed to Argus as it is produced. What differs between workloads is the rune script, the vocabulary results are reported in, and the names they are reported under. All of it is declared in a 'SearchWorkload' that a subclass points 'WORKLOAD' at, leaving the subclass with only its Argus table and its 'test_*' entry point. 'LatteScriptParams' is part of that descriptor because the rune scripts are mirrored from scylladb/vector-store rather than owned here, and each keeps its own vocabulary -- the full-text one talks about documents. Mapping those names per workload means adding a workload never requires renaming a parameter in a script that lives in another repository. Getting a shard into the loader container uses the only route the stress framework offers out of the box: 'LatteStressThread.build_stress_cmd' copies every top-level file of the rune script's directory in, so the shard is copied there for the run that loads it and removed afterwards. That is enough for a corpus small enough to live in the repo and no further -- it routes the data through the SCT source tree and ships it into every latte invocation of the run -- so two commits from here it is replaced by a copy straight into the container. Keeping it this way first is deliberate: the framework change that replaces it is easier to judge against code that shows what it costs. The query phase is deliberately absent too. Running a step's query sets and reporting their latency needs the Argus latency-table extras and the hdr-tag workload detection fix, so it lands in its own PR on top of this one. What is here is the half that stands alone: corpus load, index build, and build time and indexing throughput per step. Every phase is a single latte command -- one schema change, one shard, one index build -- so each runs on one loader: 'run_latte_thread' otherwise fans its thread out to every loader in the set, and a two-loader cluster would load each shard twice and report a record count, and the indexing throughput derived from it, off by that factor. Dataset names have to be distinct within a plan, and the flow says so rather than finding out later: an index is named after its dataset and step and its build time is read from the first matching 'full scan' pair in vector-store's log, so a repeated name would re-report the first dataset's build times against the second one's record counts. A dataset whose local directory does not exist is reported as that, because the corpora are generated rather than tracked -- a fresh clone that skipped the generator would otherwise fail on an open() of a shard inside a directory the flow had just created empty. The first consumer is the next commit, which adds the full-text search test as a descriptor plus an entry point.
Adds 'fts_test.FtsSearchTest.test_fts_search': load documents into ScyllaDB with latte, build a 'fulltext_index' on them, and report how long that build took and at what throughput to Argus. The test itself is a 'SearchWorkload' descriptor and an entry point, because the orchestration is the shared search flow of the previous commit. What is declared here is what makes the benchmark full-text: fts.rn, the '-P' names that script chose, "docs" as the unit results are counted in, and the Argus names the history is kept under -- the "FTS Index Build Time" table and the 'fts_idx_*' index names. What to run is a YAML plan, not a pile of SCT params: 'search_test_config' names it and has no default, because which datasets and shards to load is the definition of the test rather than something to fall back on. The param is the shared flow's rather than this test's -- a vector-search test names its own plan through the same one -- and it takes a path relative to the SCT root, like every other file a test case points at. 'local_config.yaml' is the tiny plan the docker run uses, and it exercises every branch of the step loop -- a single shard, a shard range, an unsharded corpus and a rebuild that loads nothing. 'generate_local_dataset.py' produces the two corpora that plan names, so a docker-backend run needs no S3 access. It lives with the plan rather than with the mirrored rune scripts because the two are one unit: the plan names 'local_tiny' and 'local_smoke', the generator writes exactly those, and the docs tell you to run it once. What it writes is gitignored because it is generated, not because it is temporary -- a run reads the corpora in place and leaves them alone. 'fts-search-test-docker.yaml' runs the whole flow on the docker backend against those corpora, which is how the orchestration is verified without an AWS cluster. docs/fts-search-test.md is the operator guide: how to run it, what to check afterwards, and the plan format. It stays a runbook -- the reasoning behind the measurement path is here rather than in it. The query phase follows in its own PR, adding a latency row per query configuration. The entry point stays as it is when it lands. The unit tests check the descriptor against the two things it names and cannot control: fts.rn, which is mirrored from scylladb/vector-store, and the Argus table. A rename in either would otherwise surface as a latte run silently ignoring every '-P' it was given, or as a new empty history in Argus.
Until now the search flow got a shard into the loader container by copying it next
to the rune script, because 'LatteStressThread.build_stress_cmd' ships every
top-level file of that directory and nothing else offered a way in. Three things
that cost:
- the corpus travelled through the SCT source tree, so a real shard would write
hundreds of megabytes into a git working copy;
- 'build_stress_cmd' ships *every* top-level file of that directory, so the
staged shard went into the container of every latte invocation of the run --
schema, index build, index drop -- not just the load that needed it;
- a shard had to exist in the tree for the whole run, which rules out fetching
one, loading it and deleting it before the next. That is the only way a corpus
too big for the runner's disk can be loaded at all, and what the S3 support
later builds on.
'LatteStressThread' grows an 'extra_files_to_stage' list of '(local, remote)' pairs
and copies them in 'build_stress_cmd'. That is the only point at which it can: the
'RemoteDocker' command runner is created and destroyed per latte invocation, so
nothing staged earlier would survive to the run, and nothing needs cleaning up
afterwards because the container is the cleanup context.
It belongs on the thread rather than on a subclass in the test. The distinction the
thread already draws is what a script needs on every invocation -- shipped with the
script's directory -- against what only this invocation needs, which had no answer
until now; that is a gap in the stress thread, not something specific to a search
benchmark. 'ClusterTester.run_latte_thread' passes the list through, so a caller
keeps the helper's timeout resolution and its loader fan-out. The load phase in
particular hands latte a cycle count as '-d', which the helper rewrites into a
duration when '--stress-duration' is set, so it has to keep passing an explicit
'duration' to stay out of that branch: easy to miss if a caller reimplemented this,
and silently wrong when missed.
Datasets land under the workload's 'remote_root' inside the container, which is a
new descriptor field.
A unit test can only assert that 'send_files' was called for what it was given.
Whether the file lands where the rune script looks for it depends on the container's
working directory, the '-P' names the script declares and the path the flow
interpolates -- so the integration test stages a corpus, loads it against a live
ScyllaDB and reads the rows back over CQL. Its negative half asserts that an
unstaged corpus loads nothing, which is what keeps the positive half honest.
The per-dataset cycle -- create the schema, load a shard, build the index, read how long that took, drop it, load the next shard and rebuild -- has until now been verifiable only by running the whole test on the docker backend and reading the result tables afterwards. That takes three minutes, a hand-driven hydra invocation, and a person to interpret it. This runs the same cycle as a test, in about fifty seconds, and asserts what a run reports: one build row per step with cumulative record counts and a positive build time, the rows actually in ScyllaDB, and the last index gone once the dataset is done. It runs the flow's own methods rather than a re-implementation of them. Two seams are replaced: '_run_latte', which would otherwise need ClusterTester's loader provisioning, and '_report_build_metrics', so the rows can be asserted instead of submitted to Argus. Everything between those -- the step loop, the '-P' mapping, the index naming, reading the build time out of vector-store's log, waiting for the drop to be noticed -- is the code that ships. It needs a ScyllaDB with a full-text index and a vector-store that serves one, and neither is released yet, so it skips unless the images are present locally. The check is a collection-time 'skipif' rather than a 'skip' in the body, because the fixture that starts the container would otherwise try to pull a missing image during setup, and that is an error rather than a skip. Both images are overridable by environment variable, so this keeps working as the released ones catch up.
… table Two opt-in parameters on 'latency_calculator_decorator', both threaded into 'send_result_to_argus' and both inert for every existing caller: 'extra_columns' appends 'ColumnMetadata' to the result table's schema, and the decorated function may then return an 'extra_values' dict written once per row alongside the usual latency cells. This is for per-row metadata that belongs next to the numbers rather than folded into an ever-longer row label -- the FTS test uses it to report the query configuration and an example query. The columns are appended to a copy, since 'Meta.Columns' is a class attribute shared by every test that reports a latency table. 'error_thresholds' overrides the 'latency_decorator_error_thresholds' test param for one call, for a caller whose expected latency is a property of what it is running rather than of the test as a whole -- again FTS, where the expectation comes from the dataset plan and differs per query set. 'extra_values' is only emitted for a result with a single HDR tag, the same branch that writes 'duration' and 'start time': with several tags there is one row per tag and no per-tag value to write.
'_HdrRangeHistogramBuilder' infers the workload from the hdr tag name and had
no keyword matching a search operation. For a latte 'search' function that is
not a cosmetic mislabel, it is fatal: 'get_latte_operation_type()' has no
'search' verb either, so it classifies 'latte run -f search' as 'user', which
means the 'stress_operation in ("WRITE", "READ")' fallback does not apply and
'_get_workload_type_by_hdr_tag()' raises ValueError on the 'fn--search' tag.
That raise lands in the streaming 'LatteHDRExporter' thread, which does not
guard 'split_line()', and 'FileFollowerThread.__exit__' re-raises it through
'future.result()' outside the 'try' that 'LatteStressThread._run_stress' wraps
around the command -- so the latte invocation fails rather than just losing a
Grafana label. Only the 'latency_calculator_decorator' path is safe, because
there 'stress_operation' is the caller's 'workload_type'.
Add 'search' next to the other read verbs.
m-szymon
force-pushed
the
pr/fts-search
branch
from
August 14, 2026 07:26
0a4e0dd to
71900c7
Compare
Completes the flow: after a step has loaded its shards and built its index, run the query sets the plan lists against that index and report their latency to Argus. The two framework changes this needs are the previous two commits -- the Argus latency-table extras and the hdr-tag workload detection. Every query entry must resolve an 'expected_p99_read_ms', on the entry itself or the dataset's 'defaults'. That single value selects the Argus table, becomes its P99 validation rule and is stated in its description, so the expectation lives in the plan rather than half in a label and half in an SCT default nobody set. It is deliberately not a column: it is a property of the table, identical on every row. What does vary per row is reported as columns -- limit, concurrency, rate and an example query -- which is why row labels carry only dataset, record count, step ordinal and query set. The step ordinal is there for the same reason the build row already carries one: the record count does not identify a step, because a step with an empty 'shards' list loads nothing and so repeats its predecessor's count. Without it, a query set run again on a rebuild would land on the row the earlier step reported under. It also lines a query row up with its build row. That leaves collisions within one step: two entries for the same set differing only in parameters that are now columns would land on one row coordinate, and Argus would keep whichever value was submitted last. 'row_labels_for_step' suffixes a colliding label with the query configuration, and additionally numbers the entries that are identical in every parameter. The suffix names every parameter rather than only those that differ within the collision group: the shorter form depends on the whole group, so adding one entry varying a parameter the others agreed on would rewrite the label of every one of them and lose their Argus history. As written it depends on nothing but the entry, so reordering never renames anything and adding an entry only affects rows sharing its label. The plan's query entries are resolved once, up front, rather than at the point each query runs. Every check here can raise -- an unknown parameter type, a duration latte cannot parse, a missing expected latency -- and by the time a query runs, its step has already loaded its shards and built its index, which on a real corpus is tens of minutes spent to report a typo. Nothing in the pass touches the cluster or the dataset files. limit, concurrency, rate and duration are checked rather than merely read, because all four are interpolated into a latte command line -- the same reason '_checked_name' exists for the plan's names. A duration must be latte's time form specifically: 'get_timeout_from_stress_cmd' parses no other, and one it cannot read silently gives the search phase the whole 'test_duration' as its timeout. The workload descriptor grows the four rune parameters and the two Argus names the query phase needs. Its fields are required, so fts_test.py's instance is updated here rather than in the next commit -- without it the module would not import.
Turns on the query phase for the full-text test: the docker plan now names query sets, so a run reports a latency row per query configuration alongside the index build rows. 'local_config.yaml' exercises the parts of the reporting that are easy to get wrong, and covers both ways two query entries can land on one Argus row. 'local_tiny' asks for a rate-limited query set on its first step, and on its second repeats one query configuration verbatim -- the two entries resolve to the same set, table and parameters within a single step, so they must come out with ' run scylladb#1' and ' run scylladb#2' appended to their configuration suffix. 'local_smoke' covers the other way: it carries qrels, which turns on the rune script's relevance metrics, then a step that neither loads nor queries, then a second no-load rebuild repeating the first step's query set. Nothing is loaded between the three, so all of them report the same document count and only the 'step #N' component of the row label keeps the last one off the first one's row. The test case documents why 'use_hdrhistogram: true' is required now: 'latency_calculator_decorator' returns early without it, so the latency tables would silently not appear. docs/fts-search-test.md gains what a plan author needs -- that every query set has to state an expected P99 and what that value selects, and how repeated query configurations are told apart -- plus the latency tables in the post-run checklist. It stays a runbook: the reasoning behind the label scheme is in this message, not in it. It also loses the trailing blank line it was added with, which 'end-of-file-fixer' rejects. The line predates this branch, but the hook runs on every file a PR touches, so it would fail CI here.
m-szymon
force-pushed
the
pr/fts-search
branch
from
August 14, 2026 08:10
71900c7 to
adffc11
Compare
Contributor
Author
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
4 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.
Add the search half of the full-text benchmark. After a step has loaded its
shards and built its index, run the query sets the plan lists against that index
and report their latency to Argus. Index build time and throughput are #15697.
With this the docker-backend test is complete. AWS runs follow.
Stacked on #15697.
Four commits. The two framework changes come first, because the query phase
cannot run without either.
'latency_calculator_decorator' gets two opt-in parameters. 'extra_columns' lets a
caller add columns to the latency table and fill them per row, which is how the
query configuration and an example query are reported next to the numbers instead
of being packed into the row label. 'error_thresholds' overrides the test-wide
'latency_decorator_error_thresholds' for one call, because here the expected
latency comes from the plan and differs per query set. Both are inert for every
existing caller.
The hdr-tag fix is one word, and it is not cosmetic. latte tags its histograms
after the rune function, so this test produces 'fn--search'.
'_get_workload_type_by_hdr_tag' has no keyword for it, 'get_latte_operation_type'
has no 'search' verb either, so the WRITE/READ fallback does not apply and the
lookup raises. The raise happens in the streaming exporter thread and surfaces
outside the try that wraps the command, so the latte invocation fails. Adding
'search' to the read verbs is the whole fix.
Then the query phase in the shared flow, and last the full-text plan and
descriptor that turn it on.
Every query set states an expected P99, on the entry or the dataset's 'defaults'.
That one value picks the Argus table, becomes its P99 validation rule and is
written into its description. It is not a column, because it is the same on every
row of the table it selects.
Row labels carry dataset, record count, step number and query set. Everything that
varies per row is a column. Two entries in one step can therefore collide on a
single row, and Argus would keep whichever was submitted last, so a colliding
label is suffixed with the query configuration, and entries identical in every
parameter are numbered as well. The suffix names every parameter, not only the
ones that differ, so that adding an entry cannot rename rows that already have
Argus history.
Follow-up: #15697
Closes: VECTOR-784
Testing
PR pre-checks (self review)
backportlabelsReminders
sdcm/sct_config.py)unit-test/folder)