diff --git a/Makefile b/Makefile index 112f0263..b7a8773c 100644 --- a/Makefile +++ b/Makefile @@ -568,7 +568,7 @@ help: @echo " make integration-test-mongodb - MongoDB Atlas Local 8.0.4" @echo " make integration-test-valkey - Valkey Bundle (latest)" @echo " make integration-test-dragonfly - Dragonfly" - @echo " make integration-test-kividb - KiviDB" + @echo " make integration-test-kividb - KiviDB 1.1.2" @echo " make integration-test-chroma - Chroma" @echo " make integration-test-vertex - Vertex AI (cloud-only, needs GCP creds)" @echo "" diff --git a/README.md b/README.md index ba4af664..b8963a2c 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ Which shipped dataset/engine pairs this stops today: | `random-geo-radius-100-angular-filters`, `random-geo-radius-2048-angular-filters` | turbopuffer, chroma | neither filter DSL has a geo operator, an attribute-vs-attribute comparison, or any arithmetic, so an exact radius has no representation at all (#223). A bounding box is a *widening*, not a substitute. | | the same two | dragonfly, valkey | neither `configure()` declares a GEO field, so the shared RediSearch geo clause targeted a field that does not exist. Previously it was emitted anyway (#223) — Dragonfly's is a builder limitation (it rejects `$param` placeholders and integer literals, not geo itself), Valkey's is an engine one (no GEO field type). | | the same two | kividb, vertex | already aborted before #219 | -| `arxiv-titles-384-angular-filters` | kividb | multi-valued `labels` (see the KiviDB note) | +| every dataset that declares a TAG field (12 shipped, `arxiv-titles-384-angular-filters` among them) | kividb (server older than 1.1.2, or reporting no parseable `kividb_version`) | TAG declarations and TAG value encoding are tested on KiviDB 1.1.2 only (KiviDB note, row 10) | Two operational consequences worth knowing: @@ -74,19 +74,63 @@ Two operational consequences worth knowing: \*\*\*\*\*\* **KiviDB note:** [KiviDB](https://kividb.io) is a Redis-wire-compatible (RESP2) data store that implements a RediSearch-compatible `FT.*` subset (`FT.CREATE`/`FT.SEARCH`/`FT.INFO`/`FT.DROPINDEX`, `VECTOR` HNSW/FLAT, `*=>[KNN k @field $blob AS score]`). Supports **vector KNN + metadata filtering**: TAG/NUMERIC/TEXT datatypes, `match`/`match_any`/`range` (inclusive **and** exclusive bounds), and AND/OR/nested boolean, all verified live against filtered ground truth. KiviDB genuinely **pre-filters** (measured recall ~1.0 down to ~2% filter selectivity), so its filtered numbers are meaningful. KiviDB's `FT.CREATE` supports only the **float32** vector type. -The filter path is **not** the shared RediSearch builder used by Redis/Valkey/Dragonfly — KiviDB's `FT.SEARCH` diverges from RediSearch in ways that would otherwise produce silently wrong recall (issue #205), so the engine has its own emitter (`kividb_filter` in `kividb.rs`, where every divergence is documented with the measurement behind it). The headline one: **KiviDB does not substitute `$param` placeholders in a filter expression, in any query form** — a plain `FT.SEARCH` fails identically to a hybrid one, so this is not a prefilter-specific quirk — and all filter values are therefore inlined as literals. It also does not accept RediSearch backslash escaping, RediSearch's `(` exclusive-range marker, or `-` negation, and it reads a spaced intra-brace TAG-OR (`@f:{a | b}`) as match-all. +The filter path is **not** the shared RediSearch builder used by Redis/Valkey/Dragonfly. The early KiviDB release this engine was first written against diverged from RediSearch's `FT.SEARCH` in ways that would otherwise have produced silently wrong recall (issue #205), so the engine got its own emitter (`kividb_filter` in `kividb.rs`, where every divergence is documented with the measurement behind it). The headline one was that `$param` placeholders were not substituted in a filter expression, in any query form, so all filter values are inlined as literals. That release also rejected RediSearch backslash escaping, RediSearch's `(` exclusive-range marker and `-` negation, and read a spaced intra-brace TAG-OR (`@f:{a | b}`) as match-all. + +**Re-measured on KiviDB 1.1.2.** Every divergence probe was re-run against `kividb/kividb:v1.1.2-full`, the image CI runs, with Redis Stack 7.4.7 (`redis-stack-server:7.4.0-v8`) and Redis 8.8.0 answering the same queries on the same fixture. The expected counts are computed from the fixture, not taken from any server. KiviDB 1.1.2 returned the expected count on every probe, as both Redis servers did, except for GEO: + +| divergence (as numbered in `kividb_filter`) | probes | KiviDB 1.1.2 | +|---|---|---| +| 1. `$param` in a filter | TAG, NUMERIC and TEXT, plain and hybrid (`=>[KNN …]`) | substituted | +| 2. backslash-escaped TAG value | `@t:{hello\ world}` | correct | +| 3. `\|` `@` `(` `)` in a TAG value | escaped, alone and inside a group | correct | +| 4. TAG-OR | `@t:{red \| blue}` and `@t:{red\|blue}` | correct | +| 5. exclusive bounds | `[-inf (5]`, `[(5 +inf]` | correct | +| 6. `-` negation | alone, and with a positive clause | correct | +| 7. multi-term TEXT | `@body:(quick brown)` | correct | +| 8. GEO field type | `FT.CREATE … loc GEO` | **absent**: `GEO fields are not supported` | +| 9. grouping | `((X))`, `A (B)` | correct | +| 10. TAG declared without `SEPARATOR` (bare) | `@s:{Smith}` and `@s:{John}` against a stored `Smith, John`; `@t:{b}` against a stored `a;b;c` under `SEPARATOR ;` | splits on `,`, the RediSearch default; `SEPARATOR` accepted and split on | + +`FT.INFO` on 1.1.2 reports `num_docs`, `percent_indexed` and an `attributes` section in which a bare TAG shows `SEPARATOR ,`, as RediSearch does, and `HELLO 3` (RESP3) succeeds. + +Row 10 is the one that decides how this tool declares TAG fields. A bare TAG indexes a stored `Smith, John` as `Smith` + `John`, so `@f:{Smith}` also matches it, exactly as on Redis, which is why `redis.rs` declares every TAG `SEPARATOR ;`. The KiviDB engine therefore reads `kividb_version` from `INFO server` and declares **every** TAG field (keyword, uuid, bool, and `labels`) `SEPARATOR ;`. It does so on KiviDB **1.1.2**, the current stable release and the one CI runs, and newer; a pre-release is gated on its core version (`1.1.2-rc1` counts as 1.1.2). An older server, or one whose version cannot be read, gets no TAG declaration at all: a dataset that declares a TAG field is refused, naming what `INFO server` reported. Vector-only and numeric/text-only datasets still run. That is a support boundary, not the point where KiviDB's behaviour changed. Older images were measured during #375: 1.0.2, 1.0.3, 1.0.4-rc3 and 1.0.4 reject `SEPARATOR`, keep a bare TAG atomic and match only raw query values, while 1.0.5-rc1, 1.0.5 and 1.0.6-rc4 already behave like 1.1.2. 1.0.7 to 1.1.1 were not measured (1.1.1 was withdrawn). Only 1.1.2 is tested, so nothing older is declared on. + +Row 10 matters for shipped data. Streaming every shipped dataset with a TAG-typed field (h-and-m, arxiv, the three random-keyword sets, `random-768-100-tenants`, MS MARCO 100K and 1M; `synthetic-filter-32` is generated locally, and `random-768-25-tenants` and MS MARCO 10M/100M have no download link) shows that h-and-m has a comma in **99 query filter values** (`section_name`, `garment_group_name`, e.g. `Womens Nightwear, Socks & Tigh`) and ~17,000 corpus values; with bare TAGs those filters would silently under-match. Under `SEPARATOR ;`, a `;` inside a value is split instead, as on Redis/Valkey/Dragonfly. No shipped query filter value contains one. In the corpus only 12 arxiv `submitter` values (HTML entities such as `ł`) and 10 MS MARCO `url`s do. Neither direction is hit: none of arxiv's 2,928 distinct `submitter` query values equals a stored `;`-value or any of its 20 fragments, compared case-insensitively and trimmed as a TAG matches, and MS MARCO has no filtered vector queries (#377). `search()` also reads the index's TAG declarations back from `FT.INFO` and refuses a reused (`--skip-upload`) index whose TAG fields the server would split on `,`, such as one built bare by an older binary. + +**Row 2 decides how the filter emitter writes a TAG value.** The emitter's literal, minimally parenthesised *structure* is unchanged and correct on 1.1.2. The TAG *value* is not sent raw: 1.1.2 honours RediSearch backslash escapes, so a raw value is read differently. Measured on 1.1.2, a raw `a\b` matched the stored `ab`, a raw `J\"org Hennig` matched `J"org Hennig`, a raw `x|y` matched `x` and `y`, and a raw `}` or a leading `"` is a syntax error. Every TAG value is therefore escaped by the RediSearch rule, a backslash before every ASCII punctuation character and space (`kividb_caps::escape_tag_value`). On 1.1.2 that was measured exact for all 32 ASCII punctuation characters plus space, at the start, middle and end of a value, bare, inside a group and AND'd; non-ASCII characters are exact unescaped. This matters for shipped data. Of the TAG-bearing datasets listed above, arxiv `submitter` is the only exposed field: + +- **77 of its 10,000 queries** use a value containing `\` (69 distinct values, e.g. `J\"org Hennig`), 14 of them also with `{`/`}` (e.g. `Rasmus Bj{\o}rk`), and 4 use parentheses (`Francisco (Paco) Guinea`, `Chun Ning (Jeanie) Lau`). +- No other shipped query value contains `\ { } ( ) | @ ;`. + +Sent raw (measured on 1.0.5, which handles escapes the same way as 1.1.2), the backslash values returned 0 documents and the brace values failed per query. Escaped, they return the exact corpus counts (`J\"org Hennig` gives 31, `Ulf-G. Mei{\ss}ner` gives 108). + +Some values have no exact encoding, and each is a **hard error** when the filter is built, never a silent miss (all measured on 1.1.2): + +- leading or trailing whitespace, which the query parser trims (`@t:{\ sp}` matched the stored `sp`, ` sp` and `sp `); +- `;`, the declared separator, so the stored value was split and nothing matches it whole; +- a trailing `*`, read as a prefix wildcard even when escaped (`@t:{zz\*}` matched 73 documents); +- a leading `$`, read as a query parameter even when escaped (`No such parameter`). + +No shipped query value hits any of these. CI runs the KiviDB suite against `kividb/kividb:v1.1.2-full` (`make integration-test-kividb`). It includes four live TAG fixtures: + +- a comma in a keyword value, which fails if a TAG is declared bare; +- backslash, brace and quote values, each next to the decoy a raw value would match instead; +- `(`, `)`, `|` and `@` values, each next to its decoys; +- an exact-count fixture: values containing `\`, `{`, `}`, `"`, `,`, `(`, `)` and `|`, each stored on a different number of documents, queried on the engine's own index with the literal the engine sends. Every value must return exactly its own documents, bare and inside a group. + +**TAG matching is case-insensitive** (measured on 1.1.2: `@t:{mixed}` matches a stored `MiXeD`), as it is in `redis.rs`'s `vector` layout, while the shipped ground truth matches exactly. So a filter value that differs from another stored value only in case over-matches, on KiviDB and on Redis. Measured by a reviewer on 1.0.5, which folds case the same way, that affects 31 of h-and-m's 10,000 queries (e.g. `department_name` `Jersey fancy`: 3,148 documents vs 1,397 exact) and 30 of arxiv's. The cross-engine question is tracked in #377. Stored whitespace is trimmed too, so `@t:{sp}` also matches a stored ` sp `. **Not supported on KiviDB**, and therefore a hard **error** rather than a silently dropped condition (following `vertex.rs`; a dropped clause would run kNN over the whole corpus and publish a recall for a filter that was never applied): -- **GEO** — KiviDB's index schema has no GEO field type at all, so the field is never indexed (unlike Dragonfly, which has a GEO type but rejects this tool's `$param` geo bounds at the query-parser level). -- **Multi-valued `labels` arrays** — a KiviDB TAG value is *atomic*, never split on a separator (`FT.CREATE` rejects the `SEPARATOR` modifier, and `@f:{b}` does not match a stored `a;b;c`), so `match_any` over a labels field could only ever match the whole joined string. -- **Multi-term or non-alphanumeric full-text terms** — KiviDB's TEXT matcher handles a single alphanumeric term per `@field:(...)` clause and returns zero documents otherwise. -- **TAG filter values containing `|`, `@`, `(` or `)`** — inexpressible with no escaping mechanism. `|` is always the TAG-OR separator (under-match), `@` degrades the whole query to match-all (over-match), and `(`/`)` are parsed as grouping as soon as the clause sits inside a group, which every non-trivial filter does (`@t:{zz(zz} @u:{blue}` returns 0; `(@t:{zz)zz})` returns the whole corpus). This is a **query-side** limit only: a *stored* value containing one of these is inert — measured, a stored `a|b` is not split, and a corpus seeded with such values returns identical counts to a clean one for every clause shape the builder emits — so uploads are never rejected for it. That matters in practice: h-and-m stores a paren in `prod_name` on 3,900 documents and benchmarks correctly, because none of the 586 distinct filter values in its shipped queries contains one. +- **GEO** — KiviDB's index schema has no GEO field type at all (1.1.2: `FT.CREATE` answers `GEO fields are not supported`), so the field is never indexed (unlike Dragonfly, which has a GEO type but rejects this tool's `$param` geo bounds at the query-parser level). +- **TAG fields of any kind, on a server older than 1.1.2 or one that reports no parseable `kividb_version`** — see row 10 above. On 1.1.2 a multi-valued `labels` field is declared `TAG SEPARATOR ;` exactly as `redis.rs` does, and `match_any` has contains-any semantics (recall 1.000 on the multi-valued fixture). As on Redis, a `;` inside a single label (or inside any keyword value) is read as the separator, so that value is split. +- **Multi-term or non-alphanumeric full-text terms** — the TEXT matcher of the release this engine was written against handled a single alphanumeric term per `@field:(...)` clause and returned zero documents otherwise; kept as a conservative refusal (#376). +- **TAG filter values with no exact encoding**: leading or trailing whitespace, `;`, a trailing `*` or a leading `$` (see the escaping paragraph above). This is a **query-side** limit only. A *stored* value of one of these shapes does not perturb other queries (measured on 1.1.2: the stored `zz*`, `$zz` and `zz*zz` leave `@t:{zz}` at exactly its own documents), so uploads are never rejected for it. The two corpus-side effects that do reach other queries, case folding and trimmed whitespace, are described above. -The first two are decidable from the dataset schema, so KiviDB rejects them in the **configure** phase — before the index is created and before the corpus is read — leaving no partial upload, no orphan result file and no populated keyspace behind. +The first two are decidable from the dataset schema and the server version, so KiviDB rejects them in the **configure** phase — before the index is created and before the corpus is read — leaving no partial upload, no orphan result file and no populated keyspace behind. GEO needs no server at all and is rejected before `configure()` even connects. -> **Which shipped datasets this excludes.** **3 of the 61** datasets in `datasets/datasets.json` cannot run on KiviDB at all and now abort instead of producing a result file: `random-geo-radius-100-angular-filters` and `random-geo-radius-2048-angular-filters` (geo), and `arxiv-titles-384-angular-filters` (multi-valued `labels` — the only shipped dataset that declares a `labels` field). Before this was fixed these three *did* run and published a recall near 0.0, which was visibly wrong; they now publish nothing at all, and no result file records the rejection, so `--plot` simply shows a gap for KiviDB on those datasets. If you are sweeping KiviDB across everything, either exclude the three from `--datasets` (`arxiv-titles-384-angular-no-filters` is the pure-KNN twin of the arxiv one) or pass `--exit-on-error false` so the rest of the sweep continues. Every other dataset — including all filtered ones — is unaffected. +> **Which shipped datasets this excludes.** On **KiviDB 1.1.2, 2 of the 62** datasets in `datasets/datasets.json` cannot run: the two geo ones, `random-geo-radius-100-angular-filters` and `random-geo-radius-2048-angular-filters`. They abort instead of producing a result file. `arxiv-titles-384-angular-filters` runs and is filtered on 1.1.2: `labels` is split on `;`, and none of its 2,928 distinct `submitter` query values hits a hard error above. The escaped encoding was verified on that data on 1.0.5, which handles escapes as 1.1.2 does: with all 2,138,591 `submitter` values loaded, every escaped query value returned exactly the count expected under case-insensitive TAG matching (25 of them differ from the exact-case count only through that case folding). On an older server, or one that reports no parseable version, every dataset that declares a TAG field (12 shipped, arxiv-filters among them) aborts as well. Before this was fixed such datasets *did* run and published a recall near 0.0, which was visibly wrong; they now publish nothing at all. No result file records the rejection, so `--plot` simply shows a gap for KiviDB on those datasets. If you are sweeping KiviDB across everything, either exclude them from `--datasets` (`arxiv-titles-384-angular-no-filters` is the pure-KNN twin of the arxiv one) or pass `--exit-on-error false` so the rest of the sweep continues. Every other dataset runs, including all the other filtered ones, with the case-insensitive TAG matching described above. -One real protocol difference worth knowing: KiviDB's `FT.INFO` does not expose RediSearch's `num_docs`/`percent_indexed` — it reports HNSW graph state directly instead (`hnsw_live_count`, `hnsw_compaction_in_progress`), because it builds each vector's HNSW entry synchronously inside the `HSET` that stores it (no async backfill phase exists to report progress on); `wait_for_indexing` polls those fields instead and returns immediately as a result. RESP2 only (no RESP3 opt-in). Set the host port with `KIVIDB_PORT` (default `6380` — KiviDB's own default listen port, **not** Redis's 6379). +One protocol detail worth knowing: the early KiviDB release this engine was written against did not expose RediSearch's `num_docs`/`percent_indexed` in `FT.INFO`; it reported HNSW graph state directly instead (`hnsw_live_count`, `hnsw_compaction_in_progress`), because KiviDB builds each vector's HNSW entry synchronously inside the `HSET` that stores it (no async backfill phase exists to report progress on). `wait_for_indexing` therefore judges an HNSW index by `hnsw_live_count` (falling back to `num_docs`) and a FLAT index, which has no graph, by `num_docs` (falling back to `hnsw_live_count`; see `indexing_wait::kividb_ready`), and returns on the first poll in practice. KiviDB 1.1.2 reports `num_docs`/`percent_indexed` and speaks RESP3; the engine stays on RESP2 (no RESP3 opt-in), which every KiviDB release speaks. Set the host port with `KIVIDB_PORT` (default `6380` — KiviDB's own default listen port, **not** Redis's 6379). \*\*\*\*\*\*\*\* **MongoDB note — which knobs MongoDB actually honours:** MongoDB Vector Search exposes **build-time** HNSW tuning and **no query-time `ef`**. diff --git a/src/bin/vector_db_benchmark/engine/filter_guard.rs b/src/bin/vector_db_benchmark/engine/filter_guard.rs index 4f1195e9..056aa21f 100644 --- a/src/bin/vector_db_benchmark/engine/filter_guard.rs +++ b/src/bin/vector_db_benchmark/engine/filter_guard.rs @@ -426,7 +426,11 @@ fn engines() -> Vec<(&'static str, EngineResolver)> { // "produced nothing" before resolving, and so does this. ("kividb", |c, _| { through("KiviDB", c, |v| { - let rendered = super::kividb::kividb_filter::parse_conditions(v)?; + // A server TAG fields are supported on (KiviDB >= 1.1.2, the + // CI pin). What this matrix checks — a filter produced, with + // every leaf kept — does not depend on how a TAG value is + // encoded; the encoding is pinned in kividb_filter's own tests. + let rendered = super::kividb::kividb_filter::parse_conditions(v, true)?; Ok((rendered != super::kividb::kividb_filter::MATCH_ALL).then_some(rendered)) }) }), diff --git a/src/bin/vector_db_benchmark/engine/kividb.rs b/src/bin/vector_db_benchmark/engine/kividb.rs index f73d1601..4cdec44f 100644 --- a/src/bin/vector_db_benchmark/engine/kividb.rs +++ b/src/bin/vector_db_benchmark/engine/kividb.rs @@ -39,13 +39,15 @@ //! //! # RESP2 only //! -//! KiviDB does not implement RESP3 — there is no protocol opt-in here (unlike -//! dragonfly.rs's `DRAGONFLY_PROTOCOL=resp3`); every connection stays RESP2. +//! The KiviDB release this engine was first written against did not implement +//! RESP3; 1.1.2 does (`HELLO 3` succeeds, measured). There is no protocol +//! opt-in here (unlike dragonfly.rs's `DRAGONFLY_PROTOCOL=resp3`); every +//! connection stays RESP2, which every KiviDB release speaks. //! -//! # FT.INFO dialect: no `num_docs` / `percent_indexed` +//! # FT.INFO dialect: no `num_docs` / `percent_indexed` (KiviDB 1.0.2) //! -//! This is the detail that actually motivated this engine. KiviDB's `FT.INFO` -//! does not expose RediSearch's `num_docs` / `indexing` / `percent_indexed` +//! This is the detail that actually motivated this engine. KiviDB 1.0.2's +//! `FT.INFO` does not expose RediSearch's `num_docs` / `indexing` / `percent_indexed` //! fields at all — a benchmark client that polls for those (as the generic //! wait-for-indexing loops in this repo, and in the legacy `v0/` Python tool, //! do) sees `num_docs` default to 0 forever and stalls until its own timeout, @@ -55,7 +57,10 @@ //! tombstone compaction). `wait_for_indexing` below polls those fields //! instead. Because KiviDB builds each vector's HNSW entry synchronously //! inside the `HSET` that stores it, this in practice returns immediately — -//! there is no async backfill phase to wait out. +//! there is no async backfill phase to wait out. KiviDB 1.1.2 (and 1.0.5 +//! before it) also reports `num_docs` and `percent_indexed`; +//! `indexing_wait::kividb_ready` accepts either shape, so nothing here depends +//! on the version. use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; @@ -71,6 +76,9 @@ use crate::dataset::Dataset; use crate::engine::index_naming::{derive_index_name, derive_key_prefix}; use crate::engine::{CorpusCount, Engine, SearchResults, UploadStats}; use crate::metrics::compute_metrics; +use vector_db_benchmark::kividb_caps::{ + check_index_tag_separators, ft_info_tag_separators, KividbServer, TAG_MIN_VERSION_STR, +}; use vector_db_benchmark::parsers::{datetime_to_epoch_secs, doc_key_to_id, doc_key_to_id_opt}; use vector_db_benchmark::readers::metadata::{is_multivalued_keyword_field, MetadataItem}; use vector_db_benchmark::start_gate::WorkerPool; @@ -117,7 +125,7 @@ impl KividbEngine { pub fn new(engine_config: &EngineConfig, host: &str) -> Result { // KiviDB's own default listen port is 6380, NOT Redis's 6379 (its // banner reads `Listening on 0.0.0.0:6380`, and the official - // `quay.io/kividbio/kividb` image exposes 6380). Defaulting to 6379 + // `kividb/kividb` image exposes 6380). Defaulting to 6379 // meant an out-of-the-box run could not connect at all, or — worse — // silently benchmarked a real Redis that happened to be on 6379. let port: u16 = crate::effective_config::env_parsed("KIVIDB_PORT", 6380); @@ -222,7 +230,24 @@ impl KividbEngine { Ok(conn) } - fn create_index(&self, conn: &mut Connection, dataset: &Dataset) -> Result<(), String> { + /// What the connected server reports about itself (`kividb_version` from + /// `INFO server`), which decides how its TAG fields must be declared — see + /// `vector_db_benchmark::kividb_caps`. One round trip; every caller is + /// outside the timed windows. + fn server_identity(conn: &mut Connection) -> Result { + let info: String = redis::cmd("INFO") + .arg("server") + .query(conn) + .map_err(|e| format!("KiviDB: INFO server failed: {e}"))?; + Ok(KividbServer::from_info(&info)) + } + + fn create_index( + &self, + conn: &mut Connection, + dataset: &Dataset, + tags_supported: bool, + ) -> Result<(), String> { let distance = dataset.distance(); let vector_size = dataset.vector_size(); @@ -259,16 +284,12 @@ impl KividbEngine { // keyword/uuid/bool exact strings -> TAG; int/float/datetime (stored as // epoch) -> NUMERIC; full-text -> TEXT. // - // Two KiviDB-specific departures from the RediSearch schema (both - // verified live against the FT.CREATE parser): - // * NO `SEPARATOR` modifier. KiviDB's FT.CREATE rejects it outright - // ("unknown field type"), and a KiviDB TAG value is atomic — it is - // never split on any separator (a query `@f:{b}` does not match a - // stored `a;b;c`). So we declare a bare TAG (exact whole-string - // match). Scalar keyword/uuid/bool filtering is unaffected; a - // genuinely multi-valued `labels` array is caught and rejected at - // upload time rather than indexed into a filter that silently - // matches nothing (see `upload_batch_internal`). + // Two KiviDB-specific points (both verified live against the + // FT.CREATE parser): + // * Every TAG is declared redis.rs's `TAG SEPARATOR ;`, on a server + // the tool supports TAG fields on (KiviDB >= 1.1.2, see + // `tag_declaration` and `kividb_caps`); on any other server no + // TAG is declared at all. // * GEO is never declared: KiviDB's schema (`FieldType`) has no Geo // variant at all, unlike Dragonfly's parser-level rejection of // `$param` geo bounds — there is simply nothing to declare here. @@ -277,8 +298,15 @@ impl KividbEngine { if let Some(schema) = dataset.config.schema.as_ref().and_then(|s| s.as_object()) { for (field_name, field_type) in schema { match field_type.as_str().unwrap_or("") { - "keyword" | "uuid" | "bool" => { - cmd.arg(field_name).arg("TAG"); + t if is_tag_type(t) => { + // `validate_dataset_support` has already rejected every + // combination `tag_declaration` refuses; it refuses them + // again here so a caller that skips validation errors + // instead of declaring a TAG that matches the wrong docs. + cmd.arg(field_name); + for arg in tag_declaration(field_name, t, tags_supported)? { + cmd.arg(*arg); + } } "int" | "float" | "datetime" => { cmd.arg(field_name).arg("NUMERIC"); @@ -489,9 +517,30 @@ impl KividbEngine { /// /// This deliberately does NOT reuse redis.rs's `parse_conditions`. That builder /// is correct for RediSearch/ValkeySearch/Dragonfly and is left untouched; the -/// list below is what was measured against a real `quay.io/kividbio/kividb:v1.0.2-full` -/// server and is why KiviDB needs its own emitter. Every item was reproduced by -/// hand with `FT.SEARCH` against a seeded index before being encoded here. +/// list below is what was measured against the early KiviDB release this engine +/// was first written against, and is why KiviDB got its own emitter. Every item +/// was reproduced by hand with `FT.SEARCH` against a seeded index before being +/// encoded here. +/// +/// Re-measured on KiviDB 1.1.2 (README, "Re-measured on KiviDB 1.1.2"): items +/// 1-7 and 9 no longer reproduce, and every probe returns the same count as +/// RediSearch; 8 (no GEO field type) still does. The literal, +/// minimally-parenthesised STRUCTURE is kept because it is correct on 1.1.2 +/// too. The TAG VALUE inside the braces is not: 1.1.2 honours RediSearch's +/// backslash escapes, so a raw `\` is read as an escape (`@t:{a\b}` matches the +/// stored `ab`, not `a\b`), a raw `}` or leading `"` is a syntax error, and a +/// raw `|` is the TAG-OR separator. Every TAG value is therefore escaped +/// (`kividb_caps::escape_tag_value`: a backslash before every ASCII punctuation character +/// and space), which was measured exact on 1.1.2 for all 32 of them plus space, +/// bare, inside a group and AND'd. [`parse_conditions`] takes whether the +/// server is one TAG fields are supported on (KiviDB >= 1.1.2, see +/// `kividb_caps`) and refuses every TAG value anywhere else, since the escaped +/// form is only known to be right there. +/// +/// What a value cannot express is a hard error (`kividb_caps::tag_value_problem`), never a +/// silent miss. On the index side a bare TAG splits on `,` (as on RediSearch), +/// so `create_index` declares every TAG `SEPARATOR ;` (`tag_declaration`), and +/// `@f:{Smith\,\ John}` matches exactly. /// /// 1. **`$param` placeholders are NOT substituted in a filter expression, in /// ANY query form.** This is the root cause of issue #205. The failure is @@ -511,18 +560,19 @@ impl KividbEngine { /// wrapped OR arm degrades the same way — which is why fixing this one alone /// would not have fixed them. So this builder inlines every value as a /// LITERAL and binds no filter params at all. -/// 2. **No escaping.** KiviDB TAG values are matched raw. RediSearch-style -/// backslash escaping matches nothing (`@uid:{3f2a\-9c}` → 0 docs, while -/// `@uid:{3f2a-9c}` → the document). Spaces, `-`, `$`, `{`, `}`, `:`, `;`, -/// `.`, `/`, quotes and backslashes all round-trip verbatim, even inside a -/// compound clause. -/// 3. **Four characters are inexpressible in a TAG value** — see -/// [`tag_value_offender`], which carries the measurement. `|` is always read -/// as the TAG-OR separator (under-match), `@` degrades the whole query to -/// match-all (over-match), and `(`/`)` are parsed as grouping the moment the -/// clause sits inside a group (`@t:{zz(zz} @u:{blue}` → 0; -/// `(@t:{zz)zz})` → the whole corpus), which every non-trivial filter does. -/// All four are hard errors here rather than a silent wrong recall. +/// 2. **No escaping (on the early release only).** KiviDB 1.0.2 matched TAG +/// values raw: a RediSearch-style backslash escape matched nothing +/// (`@uid:{3f2a\-9c}` → 0 docs, while `@uid:{3f2a-9c}` → the document), and +/// spaces, `-`, `$`, `{`, `}`, `:`, `;`, `.`, `/`, quotes and backslashes all +/// round-tripped verbatim. From 1.0.5, and on 1.1.2, this is reversed: +/// escapes are honoured and the raw forms of `\`, `|`, `}` and a leading `"` +/// are wrong, so every TAG value is escaped (`kividb_caps::escape_tag_value`). +/// 3. **Four characters were inexpressible in a TAG value on the early +/// release**: `|` (always the TAG-OR separator), `@` (degraded the query to +/// match-all) and `(`/`)` (parsed as grouping inside a group), with no +/// escape to fall back on. Escaped, all four are exact on 1.1.2, bare, +/// grouped and AND'd. What is still inexpressible is listed in +/// `kividb_caps::tag_value_problem`. /// 4. **`|` spacing is inverted between the two positions it appears in.** /// INSIDE braces it must NOT be spaced: `@f:{a | b}` matches EVERY document /// (a parser divergence) while `@f:{a|b}` is correct. BETWEEN clauses it @@ -575,6 +625,7 @@ impl KividbEngine { /// selectivity, so the filtered numbers this engine reports are meaningful. pub(crate) mod kividb_filter { use serde_json::{Map, Value}; + use vector_db_benchmark::kividb_caps::tag_literal; use vector_db_benchmark::parsers::datetime_to_epoch_secs; /// Sentinel TAG/TEXT value used to express "matches nothing". @@ -715,14 +766,14 @@ pub(crate) mod kividb_filter { /// `QueryConditions::try_resolve_all` reject it. Keeping that check at the /// call site rather than here is what stops the sentinel laundering a /// dropped filter into an unfiltered run. - pub fn parse_conditions(conditions: &Value) -> Result { + pub fn parse_conditions(conditions: &Value, tags_supported: bool) -> Result { let Some(obj) = conditions.as_object() else { return Ok(MATCH_ALL.to_string()); }; if obj.is_empty() { return Ok(MATCH_ALL.to_string()); } - Ok(build_group(obj)? + Ok(build_group(obj, tags_supported)? .map(|node| node.render()) .unwrap_or_else(|| MATCH_ALL.to_string())) } @@ -730,13 +781,13 @@ pub(crate) mod kividb_filter { /// Build one boolean group (`{and:[...], or:[...]}`). Recursive with /// [`build_subfilters`], mirroring the shape of redis.rs's builder so the /// same dataset conditions parse identically. - fn build_group(obj: &Map) -> Result, String> { + fn build_group(obj: &Map, tags_supported: bool) -> Result, String> { let and_children = match obj.get("and").and_then(|v| v.as_array()) { - Some(entries) => build_subfilters(entries)?, + Some(entries) => build_subfilters(entries, tags_supported)?, None => Vec::new(), }; let or_children = match obj.get("or").and_then(|v| v.as_array()) { - Some(entries) => build_subfilters(entries)?, + Some(entries) => build_subfilters(entries, tags_supported)?, None => Vec::new(), }; @@ -764,7 +815,7 @@ pub(crate) mod kividb_filter { /// skipping one entry of an `and` array widens the prefilter and publishes a /// recall for a filter that was only partly applied. No shipped dataset /// produces either shape, so this changes no current run. - fn build_subfilters(entries: &[Value]) -> Result, String> { + fn build_subfilters(entries: &[Value], tags_supported: bool) -> Result, String> { let mut children = Vec::new(); for entry in entries { let Some(entry_obj) = entry.as_object() else { @@ -775,7 +826,7 @@ pub(crate) mod kividb_filter { )); }; if entry_obj.contains_key("and") || entry_obj.contains_key("or") { - if let Some(group) = build_group(entry_obj)? { + if let Some(group) = build_group(entry_obj, tags_supported)? { children.push(group); } continue; @@ -788,7 +839,12 @@ pub(crate) mod kividb_filter { )); }; for (condition_type, criteria) in filter_obj { - children.push(build_filter(field_name, condition_type, criteria)?); + children.push(build_filter( + field_name, + condition_type, + criteria, + tags_supported, + )?); } } } @@ -799,17 +855,22 @@ pub(crate) mod kividb_filter { /// `None` for anything it does not recognise — every unhandled shape here is /// an error, so a condition can never be dropped on the floor and leave a /// weaker (or empty) prefilter behind. - fn build_filter(field: &str, condition_type: &str, criteria: &Value) -> Result { + fn build_filter( + field: &str, + condition_type: &str, + criteria: &Value, + tags_supported: bool, + ) -> Result { match condition_type { "match" => { // match_any (IN-list) takes precedence over exact {value}, which // takes precedence over full-text {text} — same order as redis.rs. if let Some(any) = criteria.get("any").and_then(|v| v.as_array()) { - build_match_any(field, any) + build_match_any(field, any, tags_supported) } else if let Some(text) = criteria.get("text").and_then(|v| v.as_str()) { build_text(field, text) } else { - build_exact_match(field, criteria) + build_exact_match(field, criteria, tags_supported) } } "range" => build_range(field, criteria), @@ -826,7 +887,11 @@ pub(crate) mod kividb_filter { } /// Exact match: string/bool → `@f:{value}` (TAG), number → `@f:[v v]`. - fn build_exact_match(field: &str, criteria: &Value) -> Result { + fn build_exact_match( + field: &str, + criteria: &Value, + tags_supported: bool, + ) -> Result { let Some(value) = criteria.get("value") else { return Err(format!("KiviDB filter: empty match filter for `{field}`")); }; @@ -838,7 +903,7 @@ pub(crate) mod kividb_filter { return Ok(Node::Leaf(format!("@{field}:{{{token}}}"))); } if let Some(s) = value.as_str() { - return tag_clause(field, s); + return tag_clause(field, s, tags_supported); } if let Some(i) = value.as_i64() { return Ok(Node::Leaf(format!("@{field}:[{i} {i}]"))); @@ -860,7 +925,7 @@ pub(crate) mod kividb_filter { /// reads a *spaced* intra-brace `|` as "match everything", so the shared /// builder's form silently disables the filter. The separate-clause form was /// verified to return exactly the union. - fn build_match_any(field: &str, any: &[Value]) -> Result { + fn build_match_any(field: &str, any: &[Value], tags_supported: bool) -> Result { // An all-integer list filters a NUMERIC field, so OR single-value ranges. if !any.is_empty() && any.iter().all(|v| v.is_i64()) { let clauses: Vec = any @@ -884,8 +949,8 @@ pub(crate) mod kividb_filter { if s.is_empty() { continue; } - check_tag_value(field, s)?; - clauses.push(Node::Leaf(format!("@{field}:{{{s}}}"))); + let literal = tag_literal(field, s, tags_supported)?; + clauses.push(Node::Leaf(format!("@{field}:{{{literal}}}"))); } if clauses.is_empty() { @@ -913,7 +978,8 @@ pub(crate) mod kividb_filter { "KiviDB filter: multi-term full-text query {trimmed:?} on `{field}` is not \ supported — KiviDB's TEXT matcher handles a single term per `@field:(...)` \ clause and returns ZERO documents for a multi-term query (verified live on \ - v1.0.2), which would be reported as recall 0.0." + an early release; kept as a conservative refusal), which would be reported \ + as recall 0.0." )); } if !trimmed.chars().all(|c| c.is_alphanumeric()) { @@ -1032,93 +1098,12 @@ pub(crate) mod kividb_filter { } /// A TAG clause for a literal string value. - fn tag_clause(field: &str, value: &str) -> Result { + fn tag_clause(field: &str, value: &str, tags_supported: bool) -> Result { if value.is_empty() { return Ok(never_match_tag(field)); } - check_tag_value(field, value)?; - Ok(Node::Leaf(format!("@{field}:{{{value}}}"))) - } - - /// The first character in `value` that a KiviDB TAG value cannot represent, - /// if any. - /// - /// Measured on v1.0.2 by storing `zzzz` and querying it back for 32 - /// candidate characters. FOUR fail to round-trip, and the probe has to be - /// run in every clause POSITION this builder emits, not just as a lone bare - /// clause — the first version of this check tested bare clauses only and - /// consequently missed the parens: - /// * `|` — always parsed as the TAG-OR separator, so a clause carrying it - /// silently UNDER-matches (0 documents); - /// * `@` — starts a new field reference, degrading the whole query to - /// match-all, so the clause silently OVER-matches (every document); - /// * `(` and `)` — correct ONLY in a lone bare clause. Measured on a - /// 200-doc index (35 matching documents): - /// - /// | value | bare clause | inside a group | - /// | ------------ | ----------- | -------------- | - /// | `@t:{zz(zz}` | 1 (correct) | 0 (UNDER-match) | - /// | `@t:{zz)zz}` | 1 (correct) | 35 (MATCH-ALL) | - /// - /// Every non-trivial rendering puts a TAG clause inside a group — a sole - /// `match_any`, a two-clause AND, a `match_any` AND'd with a leaf, any - /// nested boolean — so a parenthesised TAG value is silently wrong almost - /// everywhere. `build_text` already rejects `)` for the same reason. - /// - /// There is no escaping mechanism to fall back on: a backslash escape (the - /// RediSearch answer) matches nothing on KiviDB. - /// - /// # Why this is checked on the QUERY side only - /// - /// The obvious worry is the corpus: if a stored value carrying one of these - /// characters were mis-indexed, no query-side check could see it. Measured - /// on v1.0.2, it is not — the whole failure lives in the query parser and a - /// stored occurrence is INERT: - /// - /// * A stored `a|b` is **not** split on the separator. With documents - /// `a|b`, `a` and `b` in one TAG field, `@t:{a}` returns exactly 1 (the - /// `a` document), not 2. TAG values really are atomic on storage, so the - /// feared over-match — a stored `a|b` answering an unrelated `@t:{a}` — - /// does not happen. The same holds for a stored `@` (`@t:{c}` returns 1 - /// with `c` and `c@d` both indexed). - /// * Two identical corpora, one seeded with `Bag (Small)` / `x|y` / `p@q` - /// and one entirely clean, return IDENTICAL counts for every clause shape - /// this builder emits (bare leaf, ` | ` disjunction, parenthesised - /// disjunct AND'd with a leaf, two parenthesised disjuncts). - /// - /// So a corpus value carrying one of these is simply unaddressable — the - /// only query that could match it is one this function rejects — and it - /// never perturbs any other query's result. Adding an upload-side check - /// would therefore buy no correctness and would hard-fail a dataset that - /// benchmarks correctly today. Scanned over the shipped h-and-m corpus and - /// its shipped queries (13 TAG-typed schema fields): - /// - /// | | `(` | `)` | `@` | `|` | - /// | --- | --- | --- | --- | --- | - /// | corpus field-values (105,100 docs) | 3,900 | 3,878 | 0 | 0 | - /// | distinct query filter values (586) | 0 | 0 | 0 | 0 | - /// - /// An upload-side check would abort h-and-m outright; the query-side check - /// it actually needs fires on nothing shipped. - pub fn tag_value_offender(value: &str) -> Option { - value.chars().find(|c| matches!(c, '|' | '@' | '(' | ')')) - } - - /// Reject a QUERY-side TAG value KiviDB cannot represent. See - /// [`tag_value_offender`] for the measurement and for the corpus-side twin. - fn check_tag_value(field: &str, value: &str) -> Result<(), String> { - if let Some(c) = tag_value_offender(value) { - return Err(format!( - "KiviDB filter: TAG value {value:?} for field `{field}` contains {c:?}, which \ - KiviDB's FT.SEARCH cannot express — `|` is always read as the TAG-OR separator \ - (silent under-match), `@` degrades the whole query to match-all (silent \ - over-match), and `(`/`)` are parsed as grouping the moment the clause sits \ - inside a group, which every non-trivial filter does (`(` under-matches, `)` \ - degrades to match-all). Backslash escaping matches nothing on KiviDB. Refusing \ - to emit a filter that would report a wrong recall." - )); - } - Ok(()) + let literal = tag_literal(field, value, tags_supported)?; + Ok(Node::Leaf(format!("@{field}:{{{literal}}}"))) } /// TAG clause that matches nothing. See [`NEVER_MATCH`]. @@ -1139,12 +1124,18 @@ pub(crate) mod kividb_filter { use super::*; use serde_json::json; + // Render for a server TAG fields are supported on (KiviDB >= 1.1.2), + // the only one a TAG value is ever emitted for. fn q(v: Value) -> String { - parse_conditions(&v).unwrap() + parse_conditions(&v, true).unwrap() } fn err(v: Value) -> String { - parse_conditions(&v).unwrap_err() + parse_conditions(&v, true).unwrap_err() + } + + fn exact(field: &str, v: &str) -> Value { + json!({"and": [{field: {"match": {"value": v}}}]}) } #[test] @@ -1157,32 +1148,82 @@ pub(crate) mod kividb_filter { // The absence of a filter is expressed as the `*` prefilter, which is // exactly what it means on the wire, so no caller can confuse it with // a filter that silently parsed away. - assert_eq!(parse_conditions(&json!({})).unwrap(), MATCH_ALL); - assert_eq!(parse_conditions(&json!(null)).unwrap(), MATCH_ALL); - assert_eq!(parse_conditions(&json!({"and": []})).unwrap(), MATCH_ALL); + for tags_supported in [false, true] { + assert_eq!( + parse_conditions(&json!({}), tags_supported).unwrap(), + MATCH_ALL + ); + assert_eq!( + parse_conditions(&json!(null), tags_supported).unwrap(), + MATCH_ALL + ); + assert_eq!( + parse_conditions(&json!({"and": []}), tags_supported).unwrap(), + MATCH_ALL + ); + } } #[test] - fn the_tag_offender_set_is_exactly_pipe_at_and_the_two_parens() { - // Pins the shared predicate both the query side and the corpus side - // consult, so the two can never drift apart (the corpus side would - // otherwise store a value the query side would have rejected). - for (v, expected) in [ - ("a|b", Some('|')), - ("a@b", Some('@')), - ("a(b", Some('(')), - ("a)b", Some(')')), - ("plain-value_1.2:3/4", None), + fn shipped_arxiv_values_render_escaped() { + // Exactly what was measured to return the corpus counts + // (`J\"org Hennig` → 31, `Ulf-G. Mei{\ss}ner` → 108; raw, 0 and a + // syntax error), and the parenthesised value that aborted every + // arxiv-filters run before the escaping. + for (v, want) in [ + (r#"J\"org Hennig"#, r#"@s:{J\\\"org\ Hennig}"#), + (r"Ulf-G. Mei{\ss}ner", r"@s:{Ulf\-G\.\ Mei\{\\ss\}ner}"), + ("Smith, John", r"@s:{Smith\,\ John}"), + ( + "Francisco (Paco) Guinea", + r"@s:{Francisco\ \(Paco\)\ Guinea}", + ), ] { - assert_eq!(tag_value_offender(v), expected, "{v}"); + assert_eq!(q(exact("s", v)), want, "{v}"); } - // Everything else was verified live to round-trip raw. - for c in "-_.:/\\+*?~^$#%&[]{}<>=!,;'\"` ".chars() { - assert_eq!( - tag_value_offender(&format!("zz{c}zz")), - None, - "{c:?} must round-trip through a KiviDB TAG value" - ); + } + + /// `tag_literal`'s refusals (pinned in `kividb_caps`) reach the caller + /// from both TAG paths, exact match and every `match_any` arm. + #[test] + fn tag_value_problems_abort_both_exact_and_match_any() { + for v in [ + exact("c", " sp"), + exact("c", "zz*"), + json!({"and": [{"c": {"match": {"any": ["ok", "sp "]}}}]}), + json!({"and": [{"c": {"match": {"any": ["ok", "$p"]}}}]}), + ] { + let e = err(v.clone()); + assert!(e.contains("cannot be expressed exactly"), "{v}: {e}"); + } + } + + #[test] + fn match_any_escapes_every_arm() { + let out = q(json!({"and": [{"c": {"match": {"any": ["a|b", "c@d", "x"]}}}]})); + assert_eq!(out, r"@c:{a\|b} | @c:{c\@d} | @c:{x}"); + } + + /// The escaped form is only known to be right on a server TAG fields + /// are supported on; anywhere else (older, or version unknown) a TAG + /// value is refused, whether from an exact match or a `match_any`. + #[test] + fn unsupported_server_refuses_any_tag_value_and_nothing_else() { + for v in [ + exact("c", "red"), + json!({"and": [{"c": {"match": {"any": ["a"]}}}]}), + ] { + let e = parse_conditions(&v, false).unwrap_err(); + assert!(e.contains("unknown or older than 1.1.2"), "{e}"); + } + // Numeric, bool and full-text clauses do not depend on it. + for v in [ + json!({"and": [{"n": {"range": {"gte": 1}}}]}), + json!({"and": [{"n": {"match": {"value": 3}}}]}), + json!({"and": [{"f": {"match": {"value": true}}}]}), + json!({"and": [{"t": {"match": {"text": "word"}}}]}), + ] { + assert!(parse_conditions(&v, false).is_ok(), "{v}"); } } @@ -1196,13 +1237,13 @@ pub(crate) mod kividb_filter { } #[test] - fn uuid_hyphens_are_not_escaped() { - // Regression guard for #205: RediSearch would escape the hyphens, - // and the escaped form matches NOTHING on KiviDB. + fn uuid_hyphens_are_escaped() { + // #205 found that the 1.0.2-era server matched an escaped hyphen + // against nothing. KiviDB 1.1.2 honours the escape (measured: `-` + // escaped is exact), and every TAG value is escaped the same way. let uuid = "550e8400-e29b-41d4-a716-446655440000"; let out = q(json!({"and": [{"uid": {"match": {"value": uuid}}}]})); - assert_eq!(out, format!("@uid:{{{uuid}}}")); - assert!(!out.contains('\\')); + assert_eq!(out, r"@uid:{550e8400\-e29b\-41d4\-a716\-446655440000}"); } #[test] @@ -1377,37 +1418,40 @@ pub(crate) mod kividb_filter { } #[test] - fn tag_values_with_pipe_at_or_parens_are_hard_errors() { - // `|` under-matches, `@` over-matches, and `(`/`)` are read as - // grouping the moment the clause sits inside a group (which every - // non-trivial rendering does) — all silently, so none may be - // emitted. The parens were missed by the first probe because it - // only tested a LONE BARE clause, the one position where they work. - for (v, needle) in [ - ("a|b", "'|'"), - ("a@b", "'@'"), - ("a(b", "'('"), - ("a)b", "')'"), - ("Bag (Small)", "'('"), + fn tag_values_with_pipe_at_or_parens_are_escaped() { + // Inexpressible on the 1.0.2-era server (`|` under-matched, `@` + // over-matched, `(`/`)` were read as grouping inside a group); + // exact on 1.1.2 once escaped, bare and inside a group. + for (v, want) in [ + ("a|b", r"@c:{a\|b}"), + ("a@b", r"@c:{a\@b}"), + ("a(b", r"@c:{a\(b}"), + ("a)b", r"@c:{a\)b}"), + ("Bag (Small)", r"@c:{Bag\ \(Small\)}"), ] { - let e = err(json!({"and": [{"c": {"match": {"value": v}}}]})); - assert!(e.contains(needle), "{v}: {e}"); + assert_eq!(q(exact("c", v)), want, "{v}"); } - let e = err(json!({"and": [{"c": {"match": {"any": ["ok", "a|b"]}}}]})); - assert!(e.contains("'|'"), "{e}"); - let e = err(json!({"and": [{"c": {"match": {"any": ["ok", "a)b"]}}}]})); - assert!(e.contains("')'"), "{e}"); + assert_eq!( + q(json!({"and": [{"c": {"match": {"any": ["ok", "a)b"]}}}]})), + r"@c:{ok} | @c:{a\)b}" + ); } #[test] - fn tag_values_with_other_specials_round_trip_unescaped() { - // Verified live: spaces, hyphens, braces, `$`, `:`, `;`, `.` and `/` - // all match raw, so they must NOT be rejected or escaped. - for v in [ - "new york", "co-op", "x{y}z", "$a", "a:b", "a;b", "a.b", "a/b", + fn tag_values_with_other_specials_are_escaped() { + // Spaces, hyphens, braces, `$` and `*` (not at the edges that + // `tag_value_problem` refuses), `:`, `.` and `/`. + for (v, want) in [ + ("new york", r"@c:{new\ york}"), + ("co-op", r"@c:{co\-op}"), + ("x{y}z", r"@c:{x\{y\}z}"), + ("a$b", r"@c:{a\$b}"), + ("a*b", r"@c:{a\*b}"), + ("a:b", r"@c:{a\:b}"), + ("a.b", r"@c:{a\.b}"), + ("a/b", r"@c:{a\/b}"), ] { - let out = q(json!({"and": [{"c": {"match": {"value": v}}}]})); - assert_eq!(out, format!("@c:{{{v}}}")); + assert_eq!(q(exact("c", v)), want, "{v}"); } } @@ -1602,10 +1646,10 @@ fn upload_batch_internal( s.clone() }; // Stored verbatim, with NO corpus-side counterpart to - // `kividb_filter::check_tag_value`. That asymmetry is - // deliberate and measured — see `tag_value_offender`: - // the `|`/`@`/`(`/`)` failure lives entirely in the - // QUERY parser, and a stored occurrence is inert. + // `kividb_caps::tag_value_problem`. That asymmetry is + // deliberate and measured — see its doc: the failures + // it refuses live in the QUERY parser, and a stored + // occurrence is inert. hset_cmd.arg(k.as_str()).arg(stored); } MetadataValue::Int(n) => { @@ -1615,14 +1659,15 @@ fn upload_batch_internal( hset_cmd.arg(k.as_str()).arg(f.to_string()); } MetadataValue::Labels(labels) => { - // A DECLARED multi-valued field never reaches this - // point: `validate_dataset_support` rejects the dataset - // in `configure()`, before the payload is even read, - // because a KiviDB TAG value is atomic and could only - // match the whole joined string. + // Stored `;`-joined, as redis.rs does. A DECLARED + // multi-valued field only reaches this point on a server + // that splits TAG values on `;` (KiviDB >= 1.1.2); + // `validate_dataset_support` rejects the dataset in + // `configure()` on any other server, before the payload is + // even read. // // An UNDECLARED labels field was never filterable on any - // engine, so it is still stored joined — the HSET + // engine, so it is stored joined either way — the HSET // succeeds and a pure-KNN run over such a dataset keeps // working, exactly as before. hset_cmd.arg(k.as_str()).arg(labels.join(";")); @@ -1643,12 +1688,95 @@ fn upload_batch_internal( Ok(()) } +/// The FT.CREATE arguments that follow a TAG-typed field's name (`keyword`, +/// `uuid`, `bool`). `tags_supported` is [`KividbServer::tags_supported`]. Pure, +/// so the declaration each server gets is unit-tested. +/// +/// * Supported (KiviDB >= 1.1.2): `TAG SEPARATOR ;` for EVERY TAG field, +/// exactly as redis.rs declares them. A multi-valued `labels` value is stored +/// `;`-joined and splits into its elements. A scalar must not be declared +/// bare: a bare TAG splits on RediSearch's default `,`, so a stored +/// `Smith, John` would index as `Smith` + `John` and `@f:{Smith}` would +/// match it (measured on 1.1.2, see `kividb_caps`). +/// * Otherwise (an older server, or one whose version is unknown): no +/// declaration is known to be right, so `Err`. +fn tag_declaration( + field_name: &str, + field_type: &str, + tags_supported: bool, +) -> Result<&'static [&'static str], String> { + if tags_supported { + Ok(&["TAG", "SEPARATOR", ";"]) + } else { + Err(format!( + "KiviDB: refusing to declare {field_type} field `{field_name}` as a TAG on a \ + server whose version is unknown or older than {TAG_MIN_VERSION_STR}" + )) + } +} + +/// Whether `create_index` declares a schema field of this type as a TAG. The ONE +/// definition: `create_index`, `validate_dataset_support` and `tag_field_names` +/// (which scopes the FT.INFO declaration check in `search()`) all use it, so a +/// TAG type added to one cannot silently drop out of the others' checks. +fn is_tag_type(field_type: &str) -> bool { + matches!(field_type, "keyword" | "uuid" | "bool") +} + +/// The schema fields `create_index` declares as TAG (see [`is_tag_type`]). +fn tag_field_names(dataset: &Dataset) -> Vec { + dataset + .config + .schema + .as_ref() + .and_then(|s| s.as_object()) + .map(|o| { + o.iter() + .filter(|(_, t)| t.as_str().is_some_and(is_tag_type)) + .map(|(k, _)| k.clone()) + .collect() + }) + .unwrap_or_default() +} + +/// Reject, with no server round trip, the dataset schema shapes NO KiviDB +/// version can index: a `geo` field (KiviDB's index schema has no GEO type). +/// +/// `configure()` runs this BEFORE it connects, so a geo dataset aborts +/// instantly even against an unreachable server; the version-dependent checks +/// in [`validate_dataset_support`] need the server and run after. +fn validate_schema_static(dataset: &Dataset) -> Result<(), String> { + let Some(schema) = dataset.config.schema.as_ref().and_then(|s| s.as_object()) else { + return Ok(()); + }; + let dataset_name = &dataset.config.name; + for (field_name, field_type) in schema { + if field_type.as_str() == Some("geo") { + return Err(format!( + "KiviDB cannot benchmark dataset `{dataset_name}`: its schema declares geo \ + field `{field_name}`, and KiviDB's index schema has no GEO field type at all \ + (its `FieldType` has no `Geo` variant, unlike Dragonfly, which has the type \ + but rejects this tool's `$param` geo bounds at the query parser). The field \ + can never be indexed, so a geo condition matches zero documents and the run \ + would publish recall 0.0 for a filter that was never applied. Per-run \ + remedies: drop this dataset from `--datasets` (the geo datasets are \ + `random-geo-radius-100-angular-filters` and \ + `random-geo-radius-2048-angular-filters`), or keep the sweep going past it \ + with `--exit-on-error false`. Do NOT edit the field out of \ + `datasets/datasets.json` — that file is shared, and qdrant, milvus, weaviate, \ + pgvector and elasticsearch all filter this field natively." + )); + } + } + Ok(()) +} + /// Reject — BEFORE anything is created, read or uploaded — the dataset schema -/// shapes KiviDB cannot index faithfully. +/// shapes KiviDB cannot index faithfully, given what `server` reports. /// -/// Both rejections are decidable from `dataset.config.schema` alone, so they -/// belong here and not deeper in the run. Doing them late is not merely slower, -/// it is actively harmful: +/// Every rejection is decidable from `dataset.config.schema` plus the server +/// version, so it belongs here and not deeper in the run. Doing it late is not +/// merely slower, it is actively harmful: /// /// * A geo condition can only be seen in `search()`, i.e. AFTER the whole corpus /// is uploaded (1M vectors for `random-geo-radius-100-angular-filters`) AND @@ -1659,53 +1787,51 @@ fn upload_batch_internal( /// * A multi-valued `labels` array can only be seen mid-`upload()`, i.e. after /// parsing a 3.6 GB payload file for `arxiv-titles-384-angular-filters`. /// -/// Failing in `configure()` makes both aborts instant and leaves nothing behind. +/// Failing in `configure()` makes every abort instant and leaves nothing behind. /// -/// Exactly three shipped datasets are affected: the two -/// `random-geo-radius-*-angular-filters` (geo) and -/// `arxiv-titles-384-angular-filters` (multi-valued `labels`). -fn validate_dataset_support(dataset: &Dataset) -> Result<(), String> { +/// Shipped datasets affected on KiviDB >= 1.1.2: the two +/// `random-geo-radius-*-angular-filters` (geo, every KiviDB version). On an +/// older server, or one whose version cannot be read, every dataset that +/// declares a TAG field is refused too (see [`tag_declaration`]), including +/// `arxiv-titles-384-angular-filters` and its multi-valued `labels`; +/// vector-only and numeric/text-only schemas still run. +fn validate_dataset_support(dataset: &Dataset, server: &KividbServer) -> Result<(), String> { + validate_schema_static(dataset)?; let Some(schema) = dataset.config.schema.as_ref().and_then(|s| s.as_object()) else { return Ok(()); }; let dataset_name = &dataset.config.name; + if server.tags_supported() { + return Ok(()); + } + let reported = server.describe(); for (field_name, field_type) in schema { - match field_type.as_str().unwrap_or("") { - "geo" => { - return Err(format!( - "KiviDB cannot benchmark dataset `{dataset_name}`: its schema declares geo \ - field `{field_name}`, and KiviDB's index schema has no GEO field type at all \ - (its `FieldType` has no `Geo` variant, unlike Dragonfly, which has the type \ - but rejects this tool's `$param` geo bounds at the query parser). The field \ - can never be indexed, so a geo condition matches zero documents and the run \ - would publish recall 0.0 for a filter that was never applied. Per-run \ - remedies: drop this dataset from `--datasets` (the geo datasets are \ - `random-geo-radius-100-angular-filters` and \ - `random-geo-radius-2048-angular-filters`), or keep the sweep going past it \ - with `--exit-on-error false`. Do NOT edit the field out of \ - `datasets/datasets.json` — that file is shared, and qdrant, milvus, weaviate, \ - pgvector and elasticsearch all filter this field natively." - )); - } - // Only `keyword` reaches `create_index`'s TAG arm, which is where the - // atomic-TAG limitation below bites. - "keyword" if is_multivalued_keyword_field(field_name) => { - return Err(format!( - "KiviDB cannot benchmark dataset `{dataset_name}`: its schema declares the \ - multi-valued keyword field `{field_name}`, and a KiviDB TAG value is ATOMIC \ - — `FT.CREATE` rejects the `SEPARATOR` modifier outright and `@{field_name}:\ - {{b}}` does not match a stored `a;b;c`. A `match_any` over `{field_name}` \ - could therefore only ever match the whole joined string, i.e. zero documents, \ - and the run would publish recall 0.0 for a filter that was never applied. \ - Per-run remedies: drop this dataset from `--datasets` and use its unfiltered \ - twin `arxiv-titles-384-angular-no-filters` for a pure-KNN KiviDB number, or \ - keep the sweep going past it with `--exit-on-error false`. Do NOT edit the \ - field out of `datasets/datasets.json` — that file is shared, and qdrant, \ - milvus, weaviate, pgvector and elasticsearch all filter this field natively." - )); - } - _ => {} + let field_type = field_type.as_str().unwrap_or(""); + if !is_tag_type(field_type) { + continue; } + let multivalued = if field_type == "keyword" && is_multivalued_keyword_field(field_name) { + format!( + " (`{field_name}` is multi-valued: a `match_any` over it needs the TAG to split \ + on `;`; for a pure-KNN KiviDB number use the unfiltered twin \ + `arxiv-titles-384-angular-no-filters`)" + ) + } else { + String::new() + }; + return Err(format!( + "KiviDB cannot benchmark dataset `{dataset_name}` on this server: {reported}. This \ + tool declares TAG fields (here the {field_type} field `{field_name}`{multivalued}) \ + as `TAG SEPARATOR ;`, and escapes TAG filter values, on KiviDB >= \ + {TAG_MIN_VERSION_STR} only: that is the release CI tests. Older releases differ \ + (before 1.0.5 `SEPARATOR` is rejected and an escaped value matches nothing), and \ + an unreadable version could be either, so guessing risks publishing a recall for \ + a filter that matched the wrong documents. Per-run remedies: run against KiviDB \ + {TAG_MIN_VERSION_STR} or newer, whose `INFO server` reports `kividb_version` as \ + `major.minor.patch[-pre][+build]`; drop this dataset from `--datasets`; or keep \ + the sweep going past it with `--exit-on-error false`. Do NOT edit the field out \ + of `datasets/datasets.json` — that file is shared." + )); } Ok(()) } @@ -2000,20 +2126,24 @@ impl Engine for KividbEngine { } fn configure(&mut self, dataset: &Dataset) -> Result<(), String> { - // FIRST, before a connection is opened or an index dropped/created: a - // dataset whose schema KiviDB cannot index faithfully aborts here rather - // than after the corpus has been uploaded (geo) or parsed (labels). See - // `validate_dataset_support`. - validate_dataset_support(dataset)?; - + // FIRST, before an index is dropped/created: a dataset whose schema + // KiviDB cannot index faithfully aborts here rather than after the corpus + // has been uploaded (geo) or parsed (labels). See + // `validate_dataset_support`. Geo needs no server, so it is checked + // before connecting; how TAG fields may be declared depends on the + // server version, so that check follows the connection. + validate_schema_static(dataset)?; let mut conn = self.get_connection()?; + let server = Self::server_identity(&mut conn)?; + validate_dataset_support(dataset, &server)?; + println!("KiviDB server: {}", server.describe()); println!( "Using algorithm {} with config {{'M': {}, 'EF_CONSTRUCTION': {}}}", self.config.algorithm, self.config.m, self.config.ef_construction ); - self.create_index(&mut conn, dataset)?; + self.create_index(&mut conn, dataset, server.tags_supported())?; // Best-effort; see check_commandstats note in upload()/search() below. self.commandstats_baseline = redis_utils::reset_commandstats(&mut conn)?; self.commandstats_primed = true; @@ -2120,16 +2250,41 @@ impl Engine for KividbEngine { // Idempotent no-op once primed; outside every timed window. self.prime_commandstats_if_needed()?; - // Repeated for the `--skip-upload` path, which reaches `search()` - // without ever calling `configure()`. Schema-only, so it costs nothing. - validate_dataset_support(dataset)?; - - // Index-existence guard: on the --skip-upload path a missing or - // mismatched index would otherwise write a silent recall-0.0 result file. - { + // Schema validation, repeated for the `--skip-upload` path, which + // reaches `search()` without ever calling `configure()` (one INFO round + // trip); the index-existence guard: on that path a missing or + // mismatched index would otherwise write a silent recall-0.0 result + // file; and the index's TAG declarations as the SERVER reports them + // (FT.INFO), so a reused index built bare (a bare TAG + // splits on `,`) is refused rather than measured. All run before the + // queries are read, outside every timed window. Whether the server is + // one TAG fields are supported on also gates the prefilters' TAG + // values below (escaped there, refused anywhere else). + let tags_supported = { let mut conn = self.get_connection()?; + let server = Self::server_identity(&mut conn)?; + validate_dataset_support(dataset, &server)?; redis_utils::ensure_index_exists(&mut conn, &self.config.index_name)?; - } + let tag_fields = tag_field_names(dataset); + if !tag_fields.is_empty() { + let info: redis::Value = redis::cmd("FT.INFO") + .arg(&self.config.index_name) + .query(&mut conn) + .map_err(|e| { + format!("KiviDB: FT.INFO {} failed: {e}", self.config.index_name) + })?; + let separators = ft_info_tag_separators(&info) + .map_err(|e| format!("KiviDB: index `{}`: {e}", self.config.index_name))?; + check_index_tag_separators( + &self.config.index_name, + separators.as_ref(), + &tag_fields, + server.tags_supported(), + &server.describe(), + )?; + } + server.tags_supported() + }; let ef = params .search_params @@ -2157,7 +2312,7 @@ impl Engine for KividbEngine { // only a genuinely absent condition reaches `MATCH_ALL` here. let prefilters: Vec = conditions .try_resolve_all("KiviDB", |v| { - let rendered = kividb_filter::parse_conditions(v)?; + let rendered = kividb_filter::parse_conditions(v, tags_supported)?; Ok((rendered != kividb_filter::MATCH_ALL).then_some(rendered)) })? .into_iter() @@ -2406,6 +2561,171 @@ mod tests { Value::BulkString(s.as_bytes().to_vec()) } + fn dataset_with_schema(schema: serde_json::Value) -> Dataset { + Dataset::new(crate::config::DatasetConfig { + name: "schema-test".to_string(), + dataset_type: None, + path: serde_json::Value::Null, + distance: Some("cosine".to_string()), + vector_size: Some(8), + vector_count: None, + link: None, + schema: Some(schema), + description: None, + }) + } + + fn server(version: Option<&str>) -> KividbServer { + let info = match version { + Some(v) => format!("# Server\r\nredis_version:8.10.0\r\nkividb_version:{v}\r\n"), + None => "# Server\r\nredis_version:8.10.0\r\n".to_string(), + }; + KividbServer::from_info(&info) + } + + #[test] + fn multivalued_labels_rejected_below_the_minimum_version() { + let ds = dataset_with_schema(serde_json::json!({"labels": "keyword"})); + for v in [Some("1.1.0"), Some("1.1.1"), Some("1.1.1-rc3")] { + let err = validate_dataset_support(&ds, &server(v)).unwrap_err(); + assert!(err.contains("keyword field `labels`"), "{err}"); + assert!(err.contains("multi-valued"), "{err}"); + assert!(err.contains("1.1.2"), "remedy must name the version: {err}"); + } + for v in ["1.1.2", "1.1.2-rc1", "1.1.3", "1.2.0-rc2"] { + validate_dataset_support(&ds, &server(Some(v))) + .unwrap_or_else(|e| panic!("{v} indexes `labels` as TAG SEPARATOR ;: {e}")); + } + } + + /// #326 review, optional item 1: the hard error says what the server + /// reported, so an absent field, an unparsable value and a genuinely old + /// version are told apart from the message alone. + #[test] + fn rejection_names_what_the_server_reported() { + let ds = dataset_with_schema(serde_json::json!({"labels": "keyword"})); + let err = validate_dataset_support(&ds, &server(Some("1.1.1"))).unwrap_err(); + assert!( + err.contains("`kividb_version:1.1.1`, older than 1.1.2"), + "{err}" + ); + let err = validate_dataset_support(&ds, &server(Some("v1.1.2"))).unwrap_err(); + assert!( + err.contains("`kividb_version:v1.1.2`, which is not a"), + "{err}" + ); + let err = validate_dataset_support(&ds, &server(None)).unwrap_err(); + assert!(err.contains("no `kividb_version` field"), "{err}"); + } + + /// An unknown or unsupported version has no TAG declaration known to be + /// right, so EVERY TAG-typed field is refused, not only `labels`. Schemas + /// without one still run. + #[test] + fn unsupported_version_refuses_every_tag_field_and_nothing_else() { + for field_type in ["keyword", "uuid", "bool"] { + let ds = dataset_with_schema(serde_json::json!({"f": field_type})); + for unknown in [None, Some("garbage"), Some("1.1.1"), Some("1.1.0")] { + let err = validate_dataset_support(&ds, &server(unknown)).unwrap_err(); + assert!( + err.contains(&format!("{field_type} field `f`")), + "{field_type}: {err}" + ); + } + } + let ds = dataset_with_schema(serde_json::json!({"n": "int", "x": "float", "t": "text"})); + validate_dataset_support(&ds, &server(None)).expect("no TAG field, nothing to guess"); + validate_dataset_support(&ds, &server(Some("1.1.1"))).expect("no TAG field"); + let ds = dataset_with_schema(serde_json::Value::Null); + validate_dataset_support(&ds, &server(None)).expect("vector-only dataset"); + validate_dataset_support(&ds, &server(Some("1.1.0"))).expect("vector-only dataset"); + } + + #[test] + fn geo_is_rejected_on_every_server_version() { + let ds = dataset_with_schema(serde_json::json!({"location": "geo"})); + validate_schema_static(&ds).unwrap_err(); + for v in [None, Some("1.1.1"), Some("1.1.2"), Some("1.2.0")] { + let err = validate_dataset_support(&ds, &server(v)).unwrap_err(); + assert!(err.contains("geo"), "{err}"); + } + } + + #[test] + fn scalar_keyword_is_accepted_on_every_supported_server_version() { + let ds = dataset_with_schema(serde_json::json!({"color": "keyword", "n": "int"})); + for v in ["1.1.2-rc1", "1.1.2", "1.1.10", "1.2.0-rc2", "2.0.0"] { + validate_dataset_support(&ds, &server(Some(v))).expect("scalar fields are fine"); + } + } + + /// The declaration each server gets. A bare TAG splits on `,` (measured on + /// 1.1.2: `@s:{Smith}` matches a stored `Smith, John`), so a SCALAR field + /// must be `SEPARATOR ;` too, exactly as redis.rs declares it; an + /// unsupported server gets no declaration at all. + #[test] + fn tag_declaration_follows_server_support() { + for (field, ty) in [ + ("color", "keyword"), + ("id", "uuid"), + ("flag", "bool"), + ("labels", "keyword"), + ] { + assert_eq!( + tag_declaration(field, ty, true).unwrap(), + &["TAG", "SEPARATOR", ";"], + "{ty} on a supported server" + ); + assert!( + tag_declaration(field, ty, false).is_err(), + "{ty} on an unsupported server" + ); + } + } + + /// The FT.INFO declaration check in `search()` is scoped by + /// `tag_field_names`; it must cover exactly the fields `create_index` + /// declares as TAG (#375 review pass 2), or a stale bare index on a field + /// it misses would pass `--skip-upload`. + #[test] + fn tag_field_names_are_exactly_the_tag_typed_fields() { + let ds = dataset_with_schema(serde_json::json!({ + "k": "keyword", "u": "uuid", "b": "bool", "labels": "keyword", + "i": "int", "f": "float", "d": "datetime", "t": "text", "g": "geo" + })); + let mut got = tag_field_names(&ds); + got.sort(); + assert_eq!(got, ["b", "k", "labels", "u"]); + } + + /// #326 review, optional item 4: geo is rejected before `configure()` + /// connects, so it is reported as geo even when the server is unreachable + /// (port 1: nothing listens there), not as a connection error. + #[test] + fn configure_rejects_geo_before_connecting() { + let mut engine = KividbEngine { + name: "kividb-geo-unreachable".to_string(), + host: "127.0.0.1".to_string(), + port: 1, + config: KividbEngineConfig { + m: 16, + ef_construction: 128, + data_type: "FLOAT32".to_string(), + algorithm: "hnsw".to_string(), + batch_size: 1, + parallel: 1, + index_name: "idx:geo".to_string(), + key_prefix: "geo:".to_string(), + }, + search_params: Vec::new(), + commandstats_baseline: None, + commandstats_primed: false, + }; + let ds = dataset_with_schema(serde_json::json!({"location": "geo"})); + let err = engine.configure(&ds).unwrap_err(); + assert!(err.contains("geo field `location`"), "{err}"); + } + #[test] fn encode_query_vector_is_float32_le_bytes() { let v = vec![1.0f32, -2.5, 3.25]; diff --git a/src/indexing_wait.rs b/src/indexing_wait.rs index 9e3c0c5b..e4f93685 100644 --- a/src/indexing_wait.rs +++ b/src/indexing_wait.rs @@ -203,7 +203,7 @@ impl FtInfoProgress { } } -fn value_text(v: &redis::Value) -> Option { +pub(crate) fn value_text(v: &redis::Value) -> Option { match v { redis::Value::BulkString(b) => Some(String::from_utf8_lossy(b).into_owned()), redis::Value::SimpleString(s) => Some(s.clone()), @@ -239,7 +239,7 @@ pub fn parse_ft_info(info: &redis::Value) -> Result { } /// Top-level (field, value) pairs of an `FT.INFO` reply, RESP2 or RESP3. -fn ft_info_pairs(info: &redis::Value) -> Result, String> { +pub(crate) fn ft_info_pairs(info: &redis::Value) -> Result, String> { match info { redis::Value::Array(a) => Ok(a .chunks(2) @@ -354,8 +354,9 @@ pub fn kividb_ready(algorithm: &str, p: &KividbProgress, expected: u64) -> Resul Ok(match p.num_docs.or(p.hnsw_live_count) { Some(n) => n >= expected, // Verified against KiviDB source (kividbio/kividb src/commands/search.rs, - // v1.1.0 and main: `hnsw_*` only for HNSW fields) and CI on the pinned - // quay.io/kividbio/kividb:v1.0.2-full image (#361, d682fb6). + // v1.1.0 and main: `hnsw_*` only for HNSW fields) and in CI (#361, + // d682fb6). KiviDB 1.1.2 reports `num_docs` (measured), so this arm is + // only reached by a server that reports neither. None => true, }) } else { diff --git a/src/kividb_caps.rs b/src/kividb_caps.rs new file mode 100644 index 00000000..fac44f9d --- /dev/null +++ b/src/kividb_caps.rs @@ -0,0 +1,767 @@ +//! Server capabilities of a KiviDB instance that change what the `kividb` +//! engine may safely do, decided from the version the server reports. +//! +//! Lives in the library (not `src/bin/`) so the version gate is unit-tested: +//! a gate that silently opens on a server the tool does not support could +//! declare a TAG field that matches the wrong documents and publish a recall +//! for a filter that was never applied (the failure `validate_dataset_support` +//! exists to prevent). A gate that silently stays shut on a NEWER server is +//! wrong the other way round: it refuses datasets the server handles. + +use std::collections::BTreeMap; + +/// The `major.minor.patch` core of a KiviDB version. +pub type VersionCore = (u32, u32, u32); + +/// A parsed `kividb_version`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct KividbVersion { + pub core: VersionCore, + /// Whether the reported value carried a SemVer pre-release suffix + /// (`1.1.2-rc1`). Recorded, but NOT used to order versions: see + /// [`TAG_MIN_VERSION`]. + pub prerelease: bool, +} + +/// The oldest KiviDB release this tool declares TAG fields on: 1.1.2, the +/// current stable release, which is what CI runs (`make +/// integration-test-kividb`). +/// +/// On it TAG fields follow RediSearch semantics: `FT.CREATE` accepts +/// `SEPARATOR` and splits stored values on it, and a bare `TAG` (no +/// `SEPARATOR`) splits on RediSearch's default `,`. The engine therefore +/// declares every TAG `SEPARATOR ;`, exactly as redis.rs does. The query side +/// honours RediSearch backslash escapes, so the filter emitter escapes every +/// TAG value (`kividb_filter::escape_tag_value` in the engine). +/// +/// An older server, or one whose version cannot be read, is not supported for +/// TAG fields: every dataset that declares one is refused (vector-only and +/// numeric/text-only datasets still run). This is a SUPPORT boundary, not the +/// point where the behaviour changed. Measured during #375: 1.0.2, 1.0.3, +/// 1.0.4-rc3 and 1.0.4 reject `SEPARATOR`, store a bare TAG as one atomic token +/// and match only raw (unescaped) query values; 1.0.5-rc1, 1.0.5 and 1.0.6-rc4 +/// already split on `SEPARATOR` and honour escapes as 1.1.2 does. 1.0.7 to +/// 1.1.1 were not measured (1.1.1 was withdrawn). CI runs 1.1.2 only, so +/// nothing older is declared on, rather than relying on an encoding no test +/// exercises. +/// +/// A pre-release is gated on its core version (`1.1.2-rc1` counts as 1.1.2, +/// unlike strict SemVer precedence): a release candidate carries the behaviour +/// of the release it leads to. +pub const TAG_MIN_VERSION: VersionCore = (1, 1, 2); + +/// `TAG_MIN_VERSION` as text, for messages. +pub const TAG_MIN_VERSION_STR: &str = "1.1.2"; + +/// What a KiviDB server reported about itself in `INFO server`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct KividbServer { + /// The raw `kividb_version` value, if the field was present. + pub reported: Option, + /// `reported`, parsed. `None` when absent or not a recognised version. + pub version: Option, +} + +impl KividbServer { + /// Read the server's identity out of an `INFO` (or `INFO server`) reply. + pub fn from_info(info: &str) -> Self { + let reported = raw_kividb_version(info).map(str::to_string); + let version = reported.as_deref().and_then(parse_version); + Self { reported, version } + } + + /// Whether the engine may declare TAG fields on this server (`TAG + /// SEPARATOR ;`): its version is known and at least [`TAG_MIN_VERSION`]. + pub fn tags_supported(&self) -> bool { + self.version.is_some_and(|v| v.core >= TAG_MIN_VERSION) + } + + /// One clause naming what the server reported, for error messages, so + /// "why is my server rejected?" is answerable from the message alone. + pub fn describe(&self) -> String { + match (&self.reported, &self.version) { + (None, _) => "`INFO server` reported no `kividb_version` field".to_string(), + (Some(raw), None) => format!( + "`INFO server` reported `kividb_version:{raw}`, which is not a \ + `major.minor.patch[-pre][+build]` version" + ), + (Some(raw), Some(v)) if v.core < TAG_MIN_VERSION => format!( + "`INFO server` reported `kividb_version:{raw}`, older than \ + {TAG_MIN_VERSION_STR}, the oldest KiviDB this tool supports TAG fields on" + ), + (Some(raw), Some(_)) => format!("`INFO server` reported `kividb_version:{raw}`"), + } + } +} + +/// The `SEPARATOR` each TAG attribute of an index reports in `FT.INFO`, by +/// attribute name (`None`: a TAG attribute that reports no `SEPARATOR`). +/// +/// KiviDB lists `attributes` as flat `identifier, …, attribute, …, type, TAG, +/// SEPARATOR, …` arrays and reports a bare TAG as `SEPARATOR ,` (measured on +/// 1.1.2). An `FT.INFO` reply with no `attributes` section at all is +/// `Ok(None)`. +pub fn ft_info_tag_separators( + info: &redis::Value, +) -> Result>>, String> { + use crate::indexing_wait::{ft_info_pairs, value_text}; + let Some((_, attributes)) = ft_info_pairs(info)? + .into_iter() + .find(|(k, _)| k == "attributes") + else { + return Ok(None); + }; + let redis::Value::Array(attributes) = attributes else { + return Err(format!( + "FT.INFO `attributes` is not an array: {attributes:?}" + )); + }; + let mut out = BTreeMap::new(); + for attr in attributes { + let mut name = None; + let mut is_tag = false; + let mut separator = None; + for (k, v) in ft_info_pairs(attr)? { + match k.as_str() { + "attribute" => name = value_text(v), + "identifier" if name.is_none() => name = value_text(v), + "type" => is_tag = value_text(v).as_deref() == Some("TAG"), + "SEPARATOR" => separator = value_text(v), + _ => {} + } + } + if is_tag { + let name = + name.ok_or_else(|| format!("FT.INFO TAG attribute without a name: {attr:?}"))?; + out.insert(name, separator); + } + } + Ok(Some(out)) +} + +/// Check that an EXISTING index declares every TAG field in `tag_fields` +/// `SEPARATOR ;`, as `configure()` would have built it. +/// +/// This is what makes `--skip-upload` safe to reuse an index built elsewhere: +/// an index built bare (by a binary that declared scalar TAGs bare, or by +/// hand) reports `SEPARATOR ,`, splits every stored `Smith, John`, and would +/// publish a recall for filters that matched the wrong documents. +/// `tags_supported` is [`KividbServer::tags_supported`]: on a server where it +/// is false no TAG declaration is known to be right, so a TAG-bearing index +/// cannot be verified at all. +/// +/// `reported` is [`KividbServer::describe`], so every refusal names the version +/// the server reported. +pub fn check_index_tag_separators( + index: &str, + separators: Option<&BTreeMap>>, + tag_fields: &[String], + tags_supported: bool, + reported: &str, +) -> Result<(), String> { + if tag_fields.is_empty() { + return Ok(()); + } + if !tags_supported { + return Err(format!( + "KiviDB: cannot verify the TAG declarations of index `{index}`: {reported}, and \ + this tool declares TAG fields only on KiviDB >= {TAG_MIN_VERSION_STR}" + )); + } + let rebuild = format!( + "({reported}.) Re-run without `--skip-upload` so `configure()` rebuilds the index \ + with `TAG SEPARATOR ;`." + ); + let Some(separators) = separators else { + return Err(format!( + "KiviDB: `FT.INFO {index}` reports no `attributes`, so the TAG declarations of \ + the index cannot be verified. {rebuild}" + )); + }; + for field in tag_fields { + match separators.get(field) { + Some(Some(sep)) if sep == ";" => {} + Some(sep) => { + let consequence = match sep.as_deref() { + Some(",") => "That splits a stored value such as `Smith, John` into \ + `Smith` + `John`, so a filter on the whole value \ + under-matches and a filter on a fragment over-matches." + .to_string(), + Some(other) => format!( + "That splits every stored value on {other:?}, so a filter on a whole \ + value containing it under-matches and a filter on a fragment \ + over-matches." + ), + None => "The server reports no SEPARATOR for it, so how it tokenises \ + stored values is unknown." + .to_string(), + }; + let sep = sep + .as_deref() + .map_or("none".to_string(), |s| format!("{s:?}")); + return Err(format!( + "KiviDB: index `{index}` declares TAG field `{field}` with SEPARATOR \ + {sep}, not \";\". {consequence} {rebuild}" + )); + } + None => { + return Err(format!( + "KiviDB: index `{index}` has no TAG field `{field}` (FT.INFO attributes: \ + {:?}). {rebuild}", + separators.keys().collect::>() + )); + } + } + } + Ok(()) +} + +// ── TAG query values ───────────────────────────────────────────────────── +// +// How the `kividb` engine's filter builder writes a string between a TAG +// clause's braces. Here rather than in the engine so the live suite +// (`tests/integration_kividb.rs`) can query the engine-built index with +// exactly the clause the engine sends and assert exact document counts. + +/// What goes between a TAG clause's braces for `value`: the value escaped +/// with [`escape_tag_value`], or an error if it cannot be matched exactly. +/// +/// `tags_supported` is [`KividbServer::tags_supported`] (KiviDB >= 1.1.2). +/// The escaped form is only known to be right there; the release before +/// 1.0.5 matched escaped values against nothing. So on any other server a +/// TAG value is an error. The engine already refuses a dataset whose schema +/// declares a TAG field on such a server; this also covers a string +/// condition on a field the schema does not declare as a TAG. +pub fn tag_literal(field: &str, value: &str, tags_supported: bool) -> Result { + if !tags_supported { + return Err(format!( + "KiviDB filter: cannot encode TAG value {value:?} for field `{field}`: the \ + server's `kividb_version` is unknown or older than {TAG_MIN_VERSION_STR}, the \ + oldest KiviDB on which this tool's (escaped) TAG encoding is tested." + )); + } + if let Some(problem) = tag_value_problem(value) { + return Err(format!( + "KiviDB filter: TAG value {value:?} for field `{field}` cannot be expressed \ + exactly on KiviDB: {problem}. Refusing to emit a filter that would report a \ + wrong recall." + )); + } + Ok(escape_tag_value(value)) +} + +/// RediSearch-style TAG escaping, as KiviDB 1.1.2 accepts it: a backslash +/// before every ASCII punctuation character and before space. +/// +/// Measured on `kividb_version:1.1.2` (a `TAG SEPARATOR ;` index): storing +/// `zzzz`, `zz` and `zz` for each of the 32 ASCII punctuation +/// characters plus space and querying the escaped value bare, inside a +/// parenthesised OR AND'd with a second clause, and AND'd after another +/// clause returned exactly the expected documents in all three positions, +/// except the shapes [`tag_value_problem`] refuses. Raw, 22 of 146 probe +/// values were wrong: a raw `\` is read as an escape (`a\b` matched `ab`, +/// `J\"org Hennig` matched `J"org Hennig`), a raw `|` is the TAG-OR +/// separator (`x|y` matched `x` and `y`), and a raw `}` or leading `"` is a +/// syntax error. KiviDB 1.0.5 measured the same way, which is how the +/// shipped arxiv `submitter` values were found: escaped they return their +/// exact corpus counts (`J\"org Hennig` → 31, 0 raw; `Ulf-G. Mei{\ss}ner` → +/// 108, a syntax error raw). +/// +/// Non-ASCII characters are passed through: non-ASCII punctuation and +/// fullwidth look-alikes (`’ – （ ） ｜ ＠ ＊ ＄`) and accented letters +/// matched exactly unescaped on 1.1.2. Escaping everything rather than a +/// minimal set is deliberate: it is the RediSearch rule, so every character +/// this emitter could meet in a value has one encoding with a measurement +/// behind it. +pub fn escape_tag_value(value: &str) -> String { + let mut out = String::with_capacity(value.len() * 2); + for c in value.chars() { + // MUTATION (throwaway): escaping reverted, values sent raw. + let _ = c; + out.push(c); + } + out +} + +/// Why `value` cannot be expressed exactly as a KiviDB TAG query value, or +/// `None` if it can. Measured on 1.1.2 (and on 1.0.5 before it), with the +/// value escaped by [`escape_tag_value`]: +/// +/// * **Leading or trailing whitespace.** The query side trims it, and so +/// does the index: `@t:{\ sp}` matched the stored `sp`, ` sp` and `sp `. +/// Rust's `char::is_whitespace` is used, which covers the tab, U+3000 +/// and U+2028 that were measured to be trimmed as well (on 1.0.5). +/// * **`;`**, the SEPARATOR every TAG field is declared with, so the stored +/// value was split into tokens and no query matches it whole +/// (`@t:{zz\;zz}` → 0). +/// * **A trailing `*`**, read as a prefix wildcard even when escaped: +/// `@t:{zz\*}` matched 73 documents (`zz`, `zzzz`, `zza`, `zz!zz`, …) and +/// a lone `\*` is a syntax error. A `*` elsewhere (`zz\*zz`, `\*zz`) is +/// exact. +/// * **A leading `$`**, read as a query parameter even when escaped: +/// `@t:{\$zz}` fails with `No such parameter`. That error is per query, +/// and `search()` drops a failed query with only a log line, so it is +/// refused here instead. A `$` elsewhere (`zz\$`, `zz\$zz`) is exact. +/// +/// No shipped query value hits any of these. Only the QUERY side is +/// checked: a stored value with one of these shapes does not perturb any +/// other query (on 1.1.2 the stored `zz*`, `$zz` and `zz*zz` leave +/// `@t:{zz}` at exactly its own documents). Two corpus-side effects that do +/// reach other queries are in the README, because they are shared with +/// redis.rs's `vector` layout: TAG matching is case-insensitive, and stored +/// whitespace is trimmed (so `@t:{sp}` also matches a stored ` sp `). +pub fn tag_value_problem(value: &str) -> Option { + if value.starts_with(char::is_whitespace) || value.ends_with(char::is_whitespace) { + Some( + "it has leading or trailing whitespace, which KiviDB trims from a TAG query \ + value (`@t:{\\ sp}` is looked up as `sp`)" + .to_string(), + ) + } else if value.contains(';') { + Some( + "it contains `;`, the SEPARATOR every TAG field is declared with, so the \ + stored value was split into tokens and no query can match it whole" + .to_string(), + ) + } else if value.ends_with('*') { + Some( + "it ends in `*`, which KiviDB reads as a prefix wildcard even when escaped \ + (`@t:{zz\\*}` also matches `zz`, `zzab`, ...)" + .to_string(), + ) + } else if value.starts_with('$') { + Some( + "it starts with `$`, which KiviDB reads as a query parameter even when \ + escaped (`@t:{\\$zz}` fails with `No such parameter`)" + .to_string(), + ) + } else { + None + } +} + +/// The raw `kividb_version` value in an `INFO` reply, trimmed. +fn raw_kividb_version(info: &str) -> Option<&str> { + info.lines() + .find_map(|line| line.trim().strip_prefix("kividb_version:")) + .map(str::trim) +} + +/// Parse `major.minor.patch[-prerelease][+build]` (SemVer shape). Anything else +/// (`1.1`, `v1.1.2`, `1.1.2.1`, `+1.1.2`) is `None`: an unrecognised version is +/// reported as such, never guessed at. +fn parse_version(value: &str) -> Option { + // A SemVer pre-release or build identifier: non-empty, `[0-9A-Za-z.-]`. + let valid_ident = |s: &str| { + !s.is_empty() + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-') + }; + // Build metadata never affects precedence, but must still be well-formed. + let value = match value.split_once('+') { + Some((v, build)) if valid_ident(build) => v, + Some(_) => return None, + None => value, + }; + let (core, prerelease) = match value.split_once('-') { + Some((core, pre)) if valid_ident(pre) => (core, true), + Some(_) => return None, + None => (value, false), + }; + // `u32::from_str` would accept a leading `+`, but none survives here: the + // build-metadata split above already cut the value at the first `+`. + let num = |s: &str| s.parse::().ok(); + let mut parts = core.split('.'); + let major = num(parts.next()?)?; + let minor = num(parts.next()?)?; + let patch = num(parts.next()?)?; + if parts.next().is_some() { + return None; + } + Some(KividbVersion { + core: (major, minor, patch), + prerelease, + }) +} + +/// Extract and parse `kividb_version` from an `INFO` (or `INFO server`) reply. +pub fn parse_kividb_version(info: &str) -> Option { + KividbServer::from_info(info).version +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The head of a real KiviDB 1.1.2 `INFO server` reply. + const INFO_112: &str = + "# Server\r\nredis_version:8.10.0\r\nkividb_version:1.1.2\r\ngcc_version:0.0.0\r\n"; + + fn info(v: &str) -> String { + format!("# Server\r\nkividb_version:{v}\r\n") + } + + fn supported(v: &str) -> bool { + KividbServer::from_info(&info(v)).tags_supported() + } + + #[test] + fn parses_the_field_out_of_a_real_info_reply() { + assert_eq!( + parse_kividb_version(INFO_112), + Some(KividbVersion { + core: (1, 1, 2), + prerelease: false + }) + ); + assert!(KividbServer::from_info(INFO_112).tags_supported()); + } + + #[test] + fn does_not_mistake_redis_version_for_kividb_version() { + // A plain Redis (or a KiviDB build without the field) reports only + // redis_version; that must read as "unknown", not as 8.10.0. + let s = KividbServer::from_info("redis_version:8.10.0\r\n"); + assert_eq!(s.version, None); + assert!(!s.tags_supported()); + } + + #[test] + fn rejects_malformed_versions() { + for v in [ + "1.1", + "1.1.2.1", + "x.y.z", + "", + "v1.1.2", + "+1.1.2", + "1.+1.2", + "1.1.2-", + "1.1.2-r c", + "1.1.2+", + "1.1.2+b d", + ] { + assert_eq!(parse_kividb_version(&info(v)), None, "{v:?} must not parse"); + assert!(!supported(v), "{v:?} must read as unknown, so unsupported"); + } + } + + #[test] + fn parses_prerelease_and_build_metadata() { + let v = parse_kividb_version(&info("1.2.0-rc2")).unwrap(); + assert_eq!(v.core, (1, 2, 0)); + assert!(v.prerelease); + let v = parse_kividb_version(&info("1.1.2+build.7")).unwrap(); + assert_eq!(v.core, (1, 1, 2)); + assert!(!v.prerelease); + } + + /// Both sides of the gate at `TAG_MIN_VERSION`, plus the numeric-ordering + /// traps. + #[test] + fn tags_are_supported_from_the_minimum_version() { + for (v, want) in [ + // Older than the current stable release: unsupported. + ("0.10.5", false), + ("0.9.9", false), + ("1.1.0-rc1", false), + ("1.1.0", false), + ("1.1.1-rc3", false), + ("1.1.1", false), + // The current stable release, its pre-releases (gated on their + // core version) and everything newer: supported. + ("1.1.2-rc1", true), + ("1.1.2", true), + ("1.1.3", true), + ("1.2.0-rc2", true), + ("1.2.0", true), + ("2.0.0", true), + // "1.1.10" < "1.1.2" as strings; as a version it is newer. + ("1.1.10", true), + ] { + assert_eq!(supported(v), want, "kividb_version:{v}"); + } + } + + fn bulk(s: &str) -> redis::Value { + redis::Value::BulkString(s.as_bytes().to_vec()) + } + + /// The RESP2 `FT.INFO` shape KiviDB 1.1.2 returns (measured), with one + /// TAG attribute per `(name, SEPARATOR)` and one NUMERIC. + fn ft_info_112(tags: &[(&str, &str)]) -> redis::Value { + let mut attrs: Vec = tags + .iter() + .map(|(n, sep)| { + redis::Value::Array( + [ + "identifier", + n, + "attribute", + n, + "type", + "TAG", + "SEPARATOR", + sep, + ] + .iter() + .map(|s| bulk(s)) + .collect(), + ) + }) + .collect(); + attrs.push(redis::Value::Array( + ["identifier", "n", "attribute", "n", "type", "NUMERIC"] + .iter() + .map(|s| bulk(s)) + .collect(), + )); + redis::Value::Array(vec![ + bulk("index_name"), + bulk("idx:x"), + bulk("attributes"), + redis::Value::Array(attrs), + bulk("num_docs"), + bulk("0"), + ]) + } + + /// An `FT.INFO` reply without an `attributes` section. + fn ft_info_without_attributes() -> redis::Value { + redis::Value::Array(vec![ + bulk("index_name"), + bulk("idx:x"), + bulk("num_fields"), + bulk("1"), + ]) + } + + fn check(info: &redis::Value, fields: &[&str], tags_supported: bool) -> Result<(), String> { + let seps = ft_info_tag_separators(info).unwrap(); + let fields: Vec = fields.iter().map(|s| s.to_string()).collect(); + check_index_tag_separators("idx:x", seps.as_ref(), &fields, tags_supported, REPORTED) + } + + const REPORTED: &str = "`INFO server` reported `kividb_version:1.1.3`"; + + #[test] + fn reads_tag_separators_from_ft_info() { + let seps = ft_info_tag_separators(&ft_info_112(&[("s", ","), ("t", ";")])) + .unwrap() + .unwrap(); + assert_eq!(seps.get("s"), Some(&Some(",".to_string()))); + assert_eq!(seps.get("t"), Some(&Some(";".to_string()))); + assert!(!seps.contains_key("n"), "NUMERIC is not a TAG"); + assert_eq!( + ft_info_tag_separators(&ft_info_without_attributes()).unwrap(), + None + ); + } + + /// #375 review, optional item 2: `--skip-upload` must refuse an index whose + /// TAG fields the server splits on `,` (built bare, e.g. by a binary that + /// declared scalar TAGs bare). + #[test] + fn existing_index_must_declare_semicolon() { + let good = ft_info_112(&[("s", ";"), ("labels", ";")]); + check(&good, &["s", "labels"], true).unwrap(); + let stale = ft_info_112(&[("s", ","), ("labels", ";")]); + let err = check(&stale, &["s", "labels"], true).unwrap_err(); + assert!( + err.contains("`s`") && err.contains("SEPARATOR \",\""), + "{err}" + ); + let err = check(&good, &["s", "missing"], true).unwrap_err(); + assert!(err.contains("no TAG field `missing`"), "{err}"); + let bare = ft_info_without_attributes(); + assert!(check(&bare, &["s"], true).is_err(), "unverifiable"); + // On an unsupported server nothing can be verified, not even a + // `SEPARATOR ;` index. + let err = check(&good, &["s"], false).unwrap_err(); + assert!(err.contains(TAG_MIN_VERSION_STR), "{err}"); + // No TAG field: nothing to check, whatever the server. + check(&bare, &[], false).unwrap(); + // Every refusal names what the server reported. + let stale_err = check(&stale, &["s", "labels"], true).unwrap_err(); + for e in [ + stale_err.clone(), + check(&bare, &["s"], true).unwrap_err(), + check(&good, &["s"], false).unwrap_err(), + ] { + assert!(e.contains("kividb_version:1.1.3"), "{e}"); + } + // The `Smith, John` story is told only for a `,` separator. + assert!(stale_err.contains("Smith, John"), "{stale_err}"); + let pipe = check(&ft_info_112(&[("s", "|")]), &["s"], true).unwrap_err(); + assert!(pipe.contains("\"|\"") && !pipe.contains("Smith"), "{pipe}"); + } + + /// A TAG attribute that reports no `SEPARATOR` at all (`Some(None)`) is + /// refused, and the two malformed `FT.INFO` shapes are errors rather than + /// an empty (vacuously passing) map. + #[test] + fn tag_attribute_without_separator_and_malformed_ft_info_are_refused() { + let attr = |pairs: &[&str]| redis::Value::Array(pairs.iter().map(|s| bulk(s)).collect()); + let info = |attrs: redis::Value| { + redis::Value::Array(vec![ + bulk("index_name"), + bulk("idx:x"), + bulk("attributes"), + attrs, + ]) + }; + let no_sep = info(redis::Value::Array(vec![attr(&[ + "identifier", + "s", + "attribute", + "s", + "type", + "TAG", + ])])); + assert_eq!( + ft_info_tag_separators(&no_sep).unwrap().unwrap().get("s"), + Some(&None) + ); + let e = check(&no_sep, &["s"], true).unwrap_err(); + assert!(e.contains("SEPARATOR none"), "{e}"); + + let not_array = info(bulk("oops")); + let e = ft_info_tag_separators(¬_array).unwrap_err(); + assert!(e.contains("not an array"), "{e}"); + + let nameless = info(redis::Value::Array(vec![attr(&["type", "TAG"])])); + let e = ft_info_tag_separators(&nameless).unwrap_err(); + assert!(e.contains("without a name"), "{e}"); + } + + #[test] + fn describe_names_what_the_server_reported() { + let absent = KividbServer::from_info("redis_version:8.10.0\r\n").describe(); + assert!(absent.contains("no `kividb_version`"), "{absent}"); + let garbage = KividbServer::from_info(&info("v1.1.2")).describe(); + assert!( + garbage.contains("`kividb_version:v1.1.2`") && garbage.contains("not a"), + "{garbage}" + ); + let old = KividbServer::from_info(&info("1.1.1")).describe(); + assert!( + old.contains("`kividb_version:1.1.1`") && old.contains("older than 1.1.2"), + "{old}" + ); + let current = KividbServer::from_info(&info("1.1.2")).describe(); + assert!(current.contains("`kividb_version:1.1.2`"), "{current}"); + assert!( + !current.contains("not a") && !current.contains("older"), + "{current}" + ); + } + #[test] + fn the_inexpressible_set_is_semicolon_trailing_star_leading_dollar() { + for (v, needle) in [ + ("a;b", "`;`"), + (";", "`;`"), + ("zz*", "prefix wildcard"), + ("a*b*", "prefix wildcard"), + ("*", "prefix wildcard"), + ("$p", "query parameter"), + ("$", "query parameter"), + ] { + let p = tag_value_problem(v).expect(v); + assert!(p.contains(needle), "{v}: {p}"); + let e = tag_literal("c", v, true).unwrap_err(); + assert!(e.contains(needle), "{v}: {e}"); + } + // Measured exact once escaped on 1.1.2: `*` / `$` anywhere else, + // and the four characters the early release could not express. + for v in [ + "a*b", + "*zz", + "a$b", + "zz$", + "a|b", + "a@b", + "Bag (Small)", + "x{y}z", + "a\\b", + ] { + assert_eq!(tag_value_problem(v), None, "{v}"); + } + for c in "-_.:/\\+*?~^$#%&[]{}<>=!,'\"`|@() ".chars() { + assert_eq!( + tag_value_problem(&format!("zz{c}zz")), + None, + "{c:?} must be expressible (escaped) on KiviDB 1.1.2" + ); + } + } + + #[test] + fn leading_or_trailing_whitespace_is_inexpressible() { + for v in [ + " sp", + "sp ", + " sp ", + "\tlead", + "trail\t", + "zz\u{3000}", + "\u{2028}zz", + ] { + let p = tag_value_problem(v).unwrap_or_else(|| panic!("{v:?}")); + assert!(p.contains("whitespace"), "{v:?}: {p}"); + } + for v in ["two spaces", "tab\there", "sp"] { + assert_eq!(tag_value_problem(v), None, "{v:?}"); + } + let e = tag_literal("c", " sp", true).unwrap_err(); + assert!(e.contains("whitespace"), "{e}"); + } + + #[test] + fn every_ascii_punctuation_character_and_space_is_escaped() { + // The measured set: all 32 ASCII punctuation characters plus space. + let specials: String = (0u8..128) + .map(char::from) + .filter(|c| c.is_ascii_punctuation() || *c == ' ') + .collect(); + assert_eq!(specials.len(), 33); + for c in specials.chars() { + assert_eq!( + escape_tag_value(&format!("a{c}b")), + format!("a\\{c}b"), + "{c:?}" + ); + } + // Alphanumerics, other ASCII whitespace inside a value, and every + // non-ASCII character (measured exact unescaped) pass through. + for v in [ + "abcXYZ019", + "tab\there", + "Jörg Müller", + "王小明", + "（）｜＠＊＄", + "nbsp\u{a0}x", + ] { + let want: String = v + .chars() + .flat_map(|c| (c == ' ').then_some('\\').into_iter().chain([c])) + .collect(); + assert_eq!(escape_tag_value(v), want, "{v}"); + } + } + + /// The escaped form is only known to be right on a server TAG fields are + /// supported on; anywhere else every TAG value is refused, even one that + /// needs no escaping. + #[test] + fn tag_literal_escapes_on_a_supported_server_and_refuses_elsewhere() { + assert_eq!(tag_literal("c", "red", true).unwrap(), "red"); + assert_eq!( + tag_literal("s", r"Rasmus Bj{\o}rk", true).unwrap(), + r"Rasmus\ Bj\{\\o\}rk" + ); + for v in ["red", r"a\b"] { + let e = tag_literal("c", v, false).unwrap_err(); + assert!(e.contains("unknown or older than 1.1.2"), "{e}"); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index bb89cc4c..3f6b56a2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,6 +11,7 @@ //! - `results_dump` - per-query result records and the `--dump-results` format //! - `compare_results` - scores one engine's dumps against a declared reference //! - `config` - Redis configuration from environment +//! - `kividb_caps` - version-gated KiviDB server capabilities //! - `redis_client` - Redis connection management pub mod build_info; @@ -21,6 +22,7 @@ pub mod fulltext; pub mod hybrid; pub mod indexing_wait; pub mod ingest; +pub mod kividb_caps; pub mod metrics; pub mod msmarco; pub mod opensearch_maintenance; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index eb056dd2..d3a934a6 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -662,6 +662,233 @@ fn write_filter_project_multi( } } +/// Scalar keyword values that CONTAIN A COMMA: field `author` (schema type +/// `keyword`) holds `Smith, John` (id % 3 == 0), `Smith` (== 1) or `John` +/// (== 2). Even queries filter `author == "Smith, John"`, odd ones +/// `author == "Smith"`. +/// +/// Guards the TAG declaration, not the filter syntax: an engine whose index +/// splits a scalar on `,` (a bare RediSearch-style TAG, whose default +/// separator is `,`, as is KiviDB's) indexes `Smith, John` as `Smith` + +/// `John`, so the even queries match nothing and the odd ones also match every +/// `Smith, John` document. Either way recall collapses. +pub fn write_comma_keyword_project( + dataset_name: &str, + engine_configs_json: &str, + dim: usize, +) -> FilterProject { + let author = |id: usize| ["Smith, John", "Smith", "John"][id % 3]; + let wanted = |q: usize| { + if q.is_multiple_of(2) { + "Smith, John" + } else { + "Smith" + } + }; + write_filter_project_multi( + dataset_name, + engine_configs_json, + dim, + GtMetric::L2, + N_QUERIES, + serde_json::json!({ "author": "keyword" }), + move |id| serde_json::json!({ "author": author(id) }), + move |q| serde_json::json!({ "and": [ { "author": { "match": { "value": wanted(q) } } } ] }), + move |q, id| author(id) == wanted(q), + ) +} + +/// Keyword values carrying the backslash / brace / comma / quote characters +/// that KiviDB 1.1.2 only matches exactly once escaped, each next to the decoy +/// a raw value would hit instead. Shaped on the shipped arxiv `submitter` +/// values (`J\"org Hennig`, `Rasmus Bj{\o}rk`). Sent raw (measured on 1.1.2), +/// `J\"org Hennig` matches the decoy `J"org Hennig`, `a\b` matches `ab`, and +/// `Bj{\o}rk` is a syntax error (a dropped query). +/// +/// Every query targets one of those values (one is a `match_any` over two of +/// them), so any wrong encoding fails at least one of the ten queries outright. +pub const TAG_ESCAPE_VALUES: [&str; 9] = [ + r#"J\"org Hennig"#, + r#"J"org Hennig"#, + r"Rasmus Bj{\o}rk", + "Rasmus Bjork", + "Smith, John", + "Smith", + r"a\b", + "ab", + r"C:\dir", +]; + +pub fn write_tag_escape_project( + dataset_name: &str, + engine_configs_json: &str, + dim: usize, +) -> FilterProject { + write_tag_values_project( + dataset_name, + engine_configs_json, + dim, + &TAG_ESCAPE_VALUES, + &[&[0], &[2], &[4], &[6], &[8], &[0], &[2], &[4], &[6, 8]], + ) +} + +/// Keyword values with `(`, `)`, `|` and `@`, next to the decoys a mis-parse +/// would hit. KiviDB 1.1.2 expresses all four exactly once escaped (raw, `x|y` +/// matches `x` and `y`). `Francisco (Paco) Guinea` is a shipped arxiv +/// `submitter` value. +pub const TAG_PAREN_PIPE_VALUES: [&str; 7] = [ + "Francisco (Paco) Guinea", + "Francisco", + "x|y", + "x", + "y", + "c@d", + "c", +]; + +pub fn write_tag_paren_pipe_project( + dataset_name: &str, + engine_configs_json: &str, + dim: usize, +) -> FilterProject { + write_tag_values_project( + dataset_name, + engine_configs_json, + dim, + &TAG_PAREN_PIPE_VALUES, + &[&[0], &[2], &[5], &[0], &[2], &[5], &[0], &[2, 5]], + ) +} + +/// Values for the exact-count fixture: one of each character the TAG encoding +/// has to handle (`\`, `{`, `}`, `"`, `,`, `(`, `)`, `|`), next to the decoy a +/// raw or mis-parsed value would hit. Value `i` is stored on exactly +/// [`tag_count_docs`]`(i)` documents, a DISTINCT count per value, so a clause +/// that matches a decoy instead can never return the right count by accident. +pub const TAG_COUNT_VALUES: [&str; 14] = [ + r#"J\"org Hennig"#, + r#"J"org Hennig"#, + r"Rasmus Bj{\o}rk", + "Rasmus Bjork", + r"a\b", + "ab", + "close}brace", + "Smith, John", + "Smith", + "Francisco (Paco) Guinea", + "Francisco", + "f(x)", + "x|y", + "x", +]; + +/// Stored on every document no [`TAG_COUNT_VALUES`] bucket covers. +pub const TAG_COUNT_FILLER: &str = "filler"; + +/// How many documents carry `TAG_COUNT_VALUES[i]`: 12, 14, …, 38 (350 of the +/// 400; the rest carry [`TAG_COUNT_FILLER`]). All >= `TOP`, so every value can +/// also be a recall query. +pub fn tag_count_docs(i: usize) -> usize { + 12 + 2 * i +} + +/// The [`TAG_COUNT_VALUES`] index document `id` carries, or `None` for filler. +pub fn tag_count_value_index(id: usize) -> Option { + let mut start = 0; + for i in 0..TAG_COUNT_VALUES.len() { + let end = start + tag_count_docs(i); + if id < end { + return Some(i); + } + start = end; + } + None +} + +/// The exact-count fixture: field `author` (keyword) per +/// [`tag_count_value_index`]; the ten recall queries cover `\`, `{ }`, `,`, +/// `( )` and `|` values, two of them as a `match_any`. +pub fn write_tag_count_project( + dataset_name: &str, + engine_configs_json: &str, + dim: usize, +) -> FilterProject { + const WANTED: [&[usize]; 10] = [ + &[0], + &[2], + &[4], + &[6], + &[7], + &[9], + &[11], + &[12], + &[0, 9], + &[2, 6], + ]; + let author = |id: usize| { + tag_count_value_index(id) + .map(|i| TAG_COUNT_VALUES[i]) + .unwrap_or(TAG_COUNT_FILLER) + }; + write_filter_project_multi( + dataset_name, + engine_configs_json, + dim, + GtMetric::L2, + N_QUERIES, + serde_json::json!({ "author": "keyword" }), + move |id| serde_json::json!({ "author": author(id) }), + move |q| { + let w = WANTED[q % WANTED.len()]; + let m = if w.len() == 1 { + serde_json::json!({ "value": TAG_COUNT_VALUES[w[0]] }) + } else { + serde_json::json!({ + "any": w.iter().map(|&i| TAG_COUNT_VALUES[i]).collect::>() + }) + }; + serde_json::json!({ "and": [ { "author": { "match": m } } ] }) + }, + move |q, id| { + tag_count_value_index(id).is_some_and(|i| WANTED[q % WANTED.len()].contains(&i)) + }, + ) +} + +/// Field `author` (keyword) cycles through `values`; query `q` filters on +/// `wanted[q % wanted.len()]` — an exact `match` for one index, a `match_any` +/// for several — with exact-string ground truth. +fn write_tag_values_project( + dataset_name: &str, + engine_configs_json: &str, + dim: usize, + values: &'static [&'static str], + wanted: &'static [&'static [usize]], +) -> FilterProject { + let author = move |id: usize| values[id % values.len()]; + let wanted_for = move |q: usize| wanted[q % wanted.len()]; + write_filter_project_multi( + dataset_name, + engine_configs_json, + dim, + GtMetric::L2, + N_QUERIES, + serde_json::json!({ "author": "keyword" }), + move |id| serde_json::json!({ "author": author(id) }), + move |q| { + let w = wanted_for(q); + let m = if w.len() == 1 { + serde_json::json!({ "value": values[w[0]] }) + } else { + serde_json::json!({ "any": w.iter().map(|&i| values[i]).collect::>() }) + }; + serde_json::json!({ "and": [ { "author": { "match": m } } ] }) + }, + move |q, id| wanted_for(q).contains(&(id % values.len())), + ) +} + /// bool filter: field `flag` (schema type `bool`), value `id % 2 == 0`. The /// query filters `flag == true`, so half the corpus matches. pub fn write_bool_project( @@ -1872,6 +2099,18 @@ pub fn run_binary_capture( dataset: &str, host: &str, envs: &[(&str, &str)], +) -> BinaryRun { + run_binary_capture_extra(root, engine, dataset, host, envs, &[]) +} + +/// [`run_binary_capture`] with extra CLI args (e.g. `--skip-upload`). +pub fn run_binary_capture_extra( + root: &Path, + engine: &str, + dataset: &str, + host: &str, + envs: &[(&str, &str)], + extra: &[&str], ) -> BinaryRun { let mut cmd = std::process::Command::new(binary_path()); cmd.args([ @@ -1884,6 +2123,7 @@ pub fn run_binary_capture( "--skip-if-exists", "false", ]) + .args(extra) .current_dir(root); for (k, v) in envs { cmd.env(k, v); @@ -2313,7 +2553,7 @@ fn display_id(id: &str) -> &str { /// redis:8.8.0 (`db0:keys=1,expires=0,avg_ttl=0,subexpiry=0`), valkey-bundle /// (`...,keys_with_volatile_items=0`), dragonfly df-v1.40.1 /// (`db0:keys=1,expires=0,hits=0,misses=0,hit_ratio=0.00,avg_ttl=-1`) and kividb -/// v1.0.2-full (`db0:keys=1,expires=0,avg_ttl=0`) — so parse by name. All four +/// 1.1.2 (`db0:keys=1,expires=0,avg_ttl=0`) — so parse by name. All four /// emit the `# Keyspace` header (measured). pub fn sum_keyspace_keys(info: &str) -> Option { if !info @@ -2357,7 +2597,7 @@ pub fn info_field(info: &str, field: &str) -> String { /// is not honoured for a different one that later took the port. /// /// `run_id` from `INFO server` where available (redis:8.8.0, valkey-bundle, -/// kividb v1.0.2-full), falling back to `master_replid` from `INFO replication` +/// kividb 1.1.2), falling back to `master_replid` from `INFO replication` /// (dragonfly df-v1.40.1 reports no `run_id`; all four checked live). Empty when /// neither is present — an empty identity never matches a prior claim. /// @@ -2426,7 +2666,7 @@ pub fn probe_server(host: &str, port: u16, wait: std::time::Duration) -> Probe { // still filling, so "0 keys" would be a lie. // // Best-effort by design: an engine whose `INFO persistence` errors, or that - // omits `loading` entirely (kividb v1.0.2-full does — measured), simply gets + // omits `loading` entirely (an early kividb release did; 1.1.2 reports it), simply gets // no loading check. Making a missing section fatal would refuse those engines // outright, and a server so locked down that INFO fails is caught two lines // below by `INFO keyspace` anyway. diff --git a/tests/docker-compose.test.yml b/tests/docker-compose.test.yml index 136f5efd..8b804019 100644 --- a/tests/docker-compose.test.yml +++ b/tests/docker-compose.test.yml @@ -186,17 +186,16 @@ services: retries: 10 kividb: - image: quay.io/kividbio/kividb:v1.0.2-full - # KiviDB listens on 6380 internally (not 6379). The image ships no - # redis-cli/valkey-cli, but it does include bash, so the healthcheck opens - # a raw TCP connection to the server port to confirm readiness. + # The current stable KiviDB release, `full` variant (Lua + TLS). Published + # to Docker Hub (`kividb/kividb`) and GHCR (`ghcr.io/kividbio/kividb`). + image: kividb/kividb:v1.1.2-full + # KiviDB listens on 6380 internally (not 6379). The image is distroless: + # no shell and no redis-cli, so there is nothing to run a healthcheck with. + # As with Chroma below, `up --wait` therefore waits only for the container + # to be running, and the suite's wait_for_kividb() polls PING (30s + # deadline) for actual readiness. ports: - "6386:6380" - healthcheck: - test: ["CMD", "bash", "-c", "exec 3<>/dev/tcp/127.0.0.1/6380"] - interval: 1s - timeout: 3s - retries: 15 chroma: image: chromadb/chroma:1.0.0 diff --git a/tests/harness_invariants.rs b/tests/harness_invariants.rs index fad51a42..ba4b6e7c 100644 --- a/tests/harness_invariants.rs +++ b/tests/harness_invariants.rs @@ -1315,7 +1315,7 @@ fn keyspace_sum_covers_every_database_on_all_four_engines() { 1, ), ( - "kividb v1.0.2-full", + "kividb 1.1.2", "# Keyspace\r\ndb0:keys=1,expires=0,avg_ttl=0\r\n", 1, ), diff --git a/tests/integration_kividb.rs b/tests/integration_kividb.rs index 0eb05e98..20268f51 100644 --- a/tests/integration_kividb.rs +++ b/tests/integration_kividb.rs @@ -30,6 +30,7 @@ use std::time::{Duration, Instant}; use rand::rngs::StdRng; use rand::{Rng, SeedableRng}; use redis::Connection; +use vector_db_benchmark::kividb_caps::{tag_literal, KividbServer}; mod common; @@ -566,17 +567,32 @@ fn test_binary_kividb_geo_is_rejected_not_silently_dropped() { ); } -/// Multi-valued `labels`: a KiviDB TAG value is atomic (never split on any -/// separator), so a `match_any` over a labels array can only ever match the -/// whole joined string — i.e. nothing. The run must abort rather than build an -/// index that silently answers every such query with zero documents — and abort -/// in `configure()`, before the payload file is parsed at all (3.6 GB for the -/// shipped `arxiv-titles-384-angular-filters`). +/// Assert the server under test is one the tool declares TAG fields on +/// (KiviDB >= 1.1.2, see `kividb_caps`), so a server that silently stops +/// reporting a usable `kividb_version` fails the suite by name instead of +/// failing every TAG test for a reason that is harder to read. +fn assert_server_supports_tags(conn: &mut Connection) { + let info: String = redis::cmd("INFO").arg("server").query(conn).unwrap(); + let server = KividbServer::from_info(&info); + assert!( + server.tags_supported(), + "{} — this suite needs a KiviDB the tool supports TAG fields on", + server.describe() + ); +} + +/// Multi-valued `labels`: the field is declared `TAG SEPARATOR ;` (as redis.rs +/// does) and a `match_any` over the array has contains-any semantics. The +/// fixture's ground truth is filtered on that semantics, so an engine that +/// indexes the joined string as one atomic TAG scores ~0 recall. (On a server +/// the tool does not support TAG fields on, the run is refused in +/// `configure()`: unit-tested in `kividb.rs`.) #[test] -fn test_binary_kividb_multivalued_labels_is_rejected_not_silently_mis_encoded() { +fn test_binary_kividb_multivalued_labels_match_any() { wait_for_kividb(); let mut conn = get_test_connection(); flush_db(&mut conn); + assert_server_supports_tags(&mut conn); let dim = 8; let configs = serde_json::json!([{ @@ -592,21 +608,328 @@ fn test_binary_kividb_multivalued_labels_is_rejected_not_silently_mis_encoded() common::GtMetric::L2, ); assert!( - !common::run_binary( + proj.matching_docs >= proj.top, + "fixture must have >= top matching docs (got {})", + proj.matching_docs + ); + let ok = common::run_binary( + &proj.root, + "kividb-labels", + "kividb-labels-test", + TEST_HOST, + &[("KIVIDB_PORT", test_port().to_string().as_str())], + ); + assert!(ok, "kividb multi-valued labels run failed"); + let recall = common::read_recall(&proj.root, "kividb-labels"); + println!("kividb match_any labels recall={:.3}", recall); + assert!( + recall >= 0.9, + "kividb multi-valued labels match_any recall {:.3} < 0.9", + recall + ); +} + +/// A SCALAR keyword value containing a comma must match itself, and only +/// itself. KiviDB splits a bare TAG on `,` (the RediSearch default; measured on +/// 1.1.2: `@s:{Smith}` matches a stored `Smith, John`), so every TAG must be +/// declared `SEPARATOR ;`. A bare declaration collapses recall here. +#[test] +fn test_binary_kividb_keyword_value_with_comma() { + { + wait_for_kividb(); + assert_server_supports_tags(&mut get_test_connection()); + } + run_kividb_filter_test( + "kividb-comma", + "kividb-comma-test", + common::write_comma_keyword_project, + ); +} + +/// Run a TAG-value fixture with `--fail-on-dropped-queries` (a value the +/// server rejects as a syntax error would otherwise be dropped with a log line +/// and leave the recall of the surviving queries looking fine) and require a +/// near-exact recall. +/// +/// Each fixture's ten queries all target a value that KiviDB 1.1.2 matches +/// exactly only once escaped, next to the decoy a raw value would match +/// instead, so a single mis-encoded value costs a whole query (recall <= 0.9). +fn run_kividb_tag_value_test( + name: &str, + dataset: &str, + build: impl Fn(&str, &str, usize) -> common::FilterProject, +) { + let dim = 8; + let configs = serde_json::json!([{ + "name": name, "engine": "kividb", "algorithm": "hnsw", + "collection_params": { "hnsw_config": { "M": 16, "EF_CONSTRUCTION": 128 } }, + "search_params": [{ "parallel": 1, "search_params": { "ef": 256 } }], + "upload_params": { "parallel": 1, "batch_size": 64 } + }]); + let proj = build(dataset, &configs.to_string(), dim); + assert!(proj.matching_docs >= proj.top); + let port = test_port().to_string(); + let run = common::run_binary_capture_extra( + &proj.root, + name, + dataset, + TEST_HOST, + &[("KIVIDB_PORT", port.as_str())], + &["--fail-on-dropped-queries"], + ); + assert!(run.ok, "kividb {name} run failed:\n{}", run.combined); + let recall = common::read_recall(&proj.root, name); + println!("kividb {name} recall={recall:.3}"); + assert!( + recall >= 0.95, + "kividb {name} recall {recall:.3} < 0.95: a TAG value was encoded wrongly (see \ + kividb_caps::tag_literal)" + ); +} + +/// Backslash, braces, a double quote and a comma in TAG filter values +/// (shipped arxiv shape), each escaped. +#[test] +fn test_binary_kividb_tag_values_with_backslash_and_braces() { + wait_for_kividb(); + let mut conn = get_test_connection(); + flush_db(&mut conn); + assert_server_supports_tags(&mut conn); + run_kividb_tag_value_test( + "kividb-tag-escape", + "kividb-tag-escape-test", + common::write_tag_escape_project, + ); +} + +/// `(`, `)`, `|` and `@` in TAG filter values, each escaped. (Before 1.0.5 +/// these four were inexpressible; on 1.1.2 they are exact.) +#[test] +fn test_binary_kividb_tag_values_with_parens_pipe_and_at() { + wait_for_kividb(); + let mut conn = get_test_connection(); + flush_db(&mut conn); + assert_server_supports_tags(&mut conn); + run_kividb_tag_value_test( + "kividb-tag-paren", + "kividb-tag-paren-test", + common::write_tag_paren_pipe_project, + ); +} + +/// The number of documents `FT.SEARCH` matches for `query`, and their ids. +fn ft_search_ids(conn: &mut Connection, index: &str, prefix: &str, query: &str) -> Vec { + let reply: Vec = redis::cmd("FT.SEARCH") + .arg(index) + .arg(query) + .arg("RETURN") + .arg(0) + .arg("LIMIT") + .arg(0) + .arg(1000) + .arg("DIALECT") + .arg(2) + .query(conn) + .unwrap_or_else(|e| panic!("FT.SEARCH {query:?} failed: {e}")); + let total = match &reply[0] { + redis::Value::Int(n) => *n as usize, + other => panic!("FT.SEARCH {query:?}: unexpected count {other:?}"), + }; + let mut ids: Vec = reply[1..] + .iter() + .map(|v| { + let key: String = redis::from_redis_value(v.clone()).unwrap(); + key.strip_prefix(prefix) + .and_then(|id| id.parse().ok()) + .unwrap_or_else(|| panic!("unexpected key {key:?}")) + }) + .collect(); + ids.sort_unstable(); + assert_eq!(ids.len(), total, "FT.SEARCH {query:?}: count vs ids"); + ids +} + +/// EXACT counts, on the index the engine built, for TAG values containing +/// `\`, `{`, `}`, `"`, `,`, `(`, `)` and `|`, queried with exactly the TAG +/// literal the engine sends (`kividb_caps::tag_literal`). Every value is +/// stored on a distinct number of documents (`common::tag_count_docs`) next to +/// the decoy a wrong encoding hits, so a mis-encoded value fails on its count +/// and on its document ids; one that is a syntax error fails the `FT.SEARCH`. +/// Each value is checked bare and in the grouped, AND'd shape the engine's +/// filters take. The engine run itself (`--fail-on-dropped-queries`) must also +/// reach full recall. +#[test] +fn test_binary_kividb_tag_values_exact_counts() { + wait_for_kividb(); + let mut conn = get_test_connection(); + flush_db(&mut conn); + let info: String = redis::cmd("INFO").arg("server").query(&mut conn).unwrap(); + let server = KividbServer::from_info(&info); + assert!(server.tags_supported(), "{}", server.describe()); + + let name = "kividb-tag-count"; + let dataset = "kividb-tag-count-test"; + let dim = 8; + let configs = serde_json::json!([{ + "name": name, "engine": "kividb", "algorithm": "hnsw", + "collection_params": { "hnsw_config": { "M": 16, "EF_CONSTRUCTION": 128 } }, + "search_params": [{ "parallel": 1, "search_params": { "ef": 256 } }], + "upload_params": { "parallel": 1, "batch_size": 64 } + }]); + let proj = common::write_tag_count_project(dataset, &configs.to_string(), dim); + let port = test_port().to_string(); + let run = common::run_binary_capture_extra( + &proj.root, + name, + dataset, + TEST_HOST, + &[("KIVIDB_PORT", port.as_str())], + &["--fail-on-dropped-queries", "--keep-data"], + ); + assert!(run.ok, "kividb {name} run failed:\n{}", run.combined); + let recall = common::read_recall(&proj.root, name); + println!("kividb {name} recall={recall:.3}"); + assert!(recall >= 0.95, "kividb {name} recall {recall:.3} < 0.95"); + + let index = format!("idx:{name}"); + let prefix = format!("{name}:"); + for (i, value) in common::TAG_COUNT_VALUES.iter().enumerate() { + let want: Vec = (0..common::N_DOCS) + .filter(|&id| common::tag_count_value_index(id) == Some(i)) + .collect(); + assert_eq!(want.len(), common::tag_count_docs(i)); + let lit = tag_literal("author", value, server.tags_supported()) + .unwrap_or_else(|e| panic!("{value:?} must be expressible on KiviDB 1.1.2: {e}")); + for query in [ + format!("@author:{{{lit}}}"), + format!("(@author:{{{lit}}} | @author:{{no\\ such\\ value}}) @author:{{{lit}}}"), + ] { + let got = ft_search_ids(&mut conn, &index, &prefix, &query); + println!( + "kividb exact count {value:?} {query:?}: {} of {}", + got.len(), + want.len() + ); + assert_eq!( + got, + want, + "{value:?} via {query:?}: matched {} documents, expected exactly {}", + got.len(), + want.len() + ); + } + } +} + +/// `--skip-upload` must not measure a reused index whose TAG fields the server +/// splits on `,`. On KiviDB a bare TAG reports `SEPARATOR ,`; an index +/// built that way (by a binary predating #375, which declared scalar TAGs bare, +/// or by hand) under-matches every comma-bearing value. Phase 1 uploads the +/// comma fixture and keeps it; the index is then rebuilt over the same keys, +/// first bare (must be refused, naming the field) and then `SEPARATOR ;` (must +/// run at full recall: the positive control that the check is not a blanket +/// refusal and that the rebuilt index really covers the kept corpus). +#[test] +fn test_kividb_skip_upload_refuses_a_stale_bare_tag_index() { + wait_for_kividb(); + let mut conn = get_test_connection(); + flush_db(&mut conn); + assert_server_supports_tags(&mut conn); + + let name = "kividb-stale"; + let configs = serde_json::json!([{ + "name": name, "engine": "kividb", "algorithm": "hnsw", + "collection_params": { "hnsw_config": { "M": 16, "EF_CONSTRUCTION": 128 } }, + "search_params": [{ "parallel": 1, "search_params": { "ef": 256 } }], + "upload_params": { "parallel": 1, "batch_size": 64 } + }]); + let dim = 8; + let proj = common::write_comma_keyword_project("kividb-stale-test", &configs.to_string(), dim); + let port = test_port().to_string(); + let envs = [("KIVIDB_PORT", port.as_str())]; + assert!( + common::run_binary_extra( &proj.root, - "kividb-labels", - "kividb-labels-test", + name, + "kividb-stale-test", TEST_HOST, - &[("KIVIDB_PORT", test_port().to_string().as_str())], + &envs, + &["--keep-data"] ), - "kividb must REJECT a multi-valued labels field, not index it as an atomic TAG" + "phase 1 (upload, keep data) failed" ); - assert_no_results_written(&proj.root); - assert_eq!( - redis::cmd("DBSIZE").query::(&mut conn).unwrap_or(-1), - 0, - "a multi-valued labels dataset must be rejected BEFORE the corpus is uploaded" + + let index = format!("idx:{name}"); + let rebuild = |conn: &mut Connection, tag: &[&str]| { + let _: () = redis::cmd("FT.DROPINDEX").arg(&index).query(conn).unwrap(); + let mut cmd = redis::cmd("FT.CREATE"); + cmd.arg(&index) + .arg("ON") + .arg("HASH") + .arg("PREFIX") + .arg("1") + .arg(format!("{name}:")) + .arg("SCHEMA") + .arg("vector") + .arg("VECTOR") + .arg("HNSW") + .arg(10) + .arg("TYPE") + .arg("FLOAT32") + .arg("DIM") + .arg(dim) + .arg("DISTANCE_METRIC") + .arg("L2") + .arg("M") + .arg(16) + .arg("EF_CONSTRUCTION") + .arg(128) + .arg("author"); + for a in tag { + cmd.arg(*a); + } + let _: () = cmd.query(conn).unwrap(); + }; + + rebuild(&mut conn, &["TAG"]); + delete_search_result_files(&proj.root); + let run = common::run_binary_capture_extra( + &proj.root, + name, + "kividb-stale-test", + TEST_HOST, + &envs, + &["--skip-upload", "--keep-data"], + ); + assert!(!run.ok, "a bare-TAG index must be refused"); + assert!( + run.combined + .contains("declares TAG field `author` with SEPARATOR \",\""), + "refused for the wrong reason:\n{}", + run.combined + ); + let searches: Vec<_> = fs::read_dir(proj.root.join("results")) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.contains("-search-")) + .collect(); + assert!(searches.is_empty(), "refused run wrote {searches:?}"); + + rebuild(&mut conn, &["TAG", "SEPARATOR", ";"]); + assert!( + common::run_binary_extra( + &proj.root, + name, + "kividb-stale-test", + TEST_HOST, + &envs, + &["--skip-upload"] + ), + "a SEPARATOR ; index over the kept corpus must be accepted" ); + let recall = common::read_recall(&proj.root, name); + assert!(recall >= 0.9, "rebuilt-index recall {recall:.3} < 0.9"); } /// A `"algorithm":"flat"` config must run end-to-end: EF_RUNTIME is HNSW-only,