perf(lexical): add a doc_values opt-out for stored-field DocValues duplication - #1115
Merged
Merged
Conversation
…misses InvertedIndexReader::has_doc_values is an index-wide any(...) across segments, not a per-document guarantee. TopFieldCollector::get_field_value and FacetCollector::collect_doc both treated a get_doc_value miss under has_dv == true as "the value is Null" / "no contribution" instead of falling back to the stored document. Unreachable via the "no segment has the column at all" case (already fixed in #1053), but real whenever segments disagree on whether they have the column -- which mixed old/new segments, or a doc_values: false field, will do routinely. Fixing this is a prerequisite for the doc_values opt-out flag (#1047). Also adds the missing document_fields override on PerSegmentReaderView, which was falling through to the trait default (document() + full clone). Refs #1047
…can use it alloc_bounds's checked_capacity/checked_len bound header-declared sizes against the true file length before allocating -- useful for any segment loader parsing unverified on-disk headers, not just vector segments. Moves it from vector::index (pub(crate)) to util (pub(crate)) and generalizes the "vector segment is corrupted" wording to "segment is corrupted" so the upcoming DocValuesReader bounds check (#1047) can reuse it without a vector-specific error message. No behavior change; existing tests only assert on the "corrupted" substring. Refs #1047
…hecking DocValuesReader::load used to deserialize every field's doc_id -> value payload into memory on first access, even when a query only touched one field. It now reads just the header and each field's directory entry (name, on-disk offset, payload length), skipping past payloads via seek; a field's map is deserialized and cached on its first get_value()/materialize() call. has_field/field_names are pure directory lookups that never touch a payload. Mirrors BKDReader's storage + path pattern (re-open per access) rather than holding a handle across the segment's lifetime. Also, as part of the same pass: - Bound every header-declared size (num_fields, name length, payload length) against the file's true byte length via crate::util::alloc_bounds, so a flipped header byte surfaces a clean "corrupted" error instead of an OOM abort or an out-of-bounds read. - DocValuesWriter.fields: HashMap -> BTreeMap, and each field's values_vec is now sorted by doc_id before serializing, so identical content always produces byte-identical .dv files regardless of insertion order. - Removed the unused DocValuesReader::get_field (zero callers). - get_value's signature changed from Option<&FieldValue> to Result<Option<FieldValue>> since materialization is now fallible I/O; updated its one caller (InvertedIndexReader::get_doc_value). Rewrote lexical_field_sort_test.rs::doc_values_load_once_per_segment, which pinned a literal "2 opens" tied to the old eager whole-file load; it now asserts the real invariant -- a repeated identical search adds zero further .dv opens -- since the exact open count changed shape (directory open + one payload open per touched field, per segment). No behavior change: same values, same .dv format, only when/how the payload is read changes. Refs #1047
Adds doc_values: bool (default true) to the seven field-option structs that can have a DocValues column -- TextOption, IntegerOption, FloatOption, BooleanOption, DateTimeOption, GeoOption, Geo3dOption. Not added to BytesOption: is_doc_values_candidate already excludes Bytes/Vector from DocValues unconditionally (#1052/#1053), so a flag there would always be dead. Effective rule (documented on each field): a DocValues column is written only when stored && doc_values are both true; doc_values: true with stored: false is silently ignored, the same precedent as indexed: false + term_vectors: true. This commit only adds the field and its accessors -- it does not yet gate the write path (that lands in the next commit). Every other exhaustive struct literal of these seven types across the workspace (laurus tests, the five bindings' schema.rs, laurus-cli's create wizard, laurus-server's proto<->domain conversion) is updated mechanically with doc_values: true to preserve exact prior behavior; none of them expose the new flag yet. Refs #1047
…ld paths
Core write-path wiring (Phase 3):
- InvertedIndexConfig::store_doc_values / LexicalIndexConfigBuilder::
store_doc_values: index-wide default, mirrors store_term_vectors.
- InvertedIndexWriterConfig::{field_doc_values, store_doc_values} and
stores_doc_values(field_name), a 3-level resolution (override >
field's own schema setting > index-wide default) identical in shape
to stores_term_positions (#1083).
- Both DocValues feed sites (upsert_analyzed_document,
rebuild_in_memory_index) now gate on
`is_doc_values_candidate(value) && stores_doc_values(field_name)`,
not just the type check.
- FieldOption::doc_values(&self) -> Option<bool> centralizes the 7-arm
match (None only for Bytes) so this and the merge path below don't
each duplicate it.
- InvertedIndex::writer() passes config.store_doc_values through.
Merge / field-rebuild wiring (Phase 4), deliberately NOT mirroring how
#1083 handled term_vectors: a discarded term position is unrecoverable,
so #1083 could only detect what a source segment already had. A
DocValues column can always be regenerated from stored_fields, so here
the CURRENT schema wins over whatever is on disk:
- MergeConfig::{field_doc_values, default_doc_values}: the owning
index's current per-field settings and index-wide default, populated
by InvertedIndex::merge_segment_set / rebuild_field via the new
current_field_doc_values() helper.
- reconstruct_segment / reconstruct_segment_with_field_override thread
a doc_values_by_field map alongside the existing positions_by_field:
schema-declared fields are pre-seeded (so detection's
.entry().or_insert_with() never overrides them); anything left
undeclared falls back to detecting the source segment's actual
column. rebuild_field_across_segments gained a target_doc_values
parameter, seeded the same way target_term_vectors already is.
- The replay InvertedIndexWriterConfig gets the resolved
field_doc_values map and store_doc_values default in both merge
paths.
Tests: stores_doc_values resolution order, a doc_values: false field
excluded from a real segment's DocValues (unit + on-disk), a large
field's .dv payload measurably shrinking, three merge-path tests
(independent per-field state across a merge, current schema winning
over a stale on-disk column, and a value-less field not being
misdetected as opted out). All new assertions were RED-proven by
reverting the relevant line and confirming the expected failure.
Refs #1047
Adds classify_doc_values(old, new, old_stored), folded via .max(...) into every lexical arm of classify_change except Bytes (which has no doc_values flag). Reuses classify_indexed_only's exact logic: turning DocValues on needs the column regenerated from stored_fields (Reindex when old_stored, Destructive otherwise, mirroring #1083's term_vectors); turning it off is MetadataOnly, since InvertedIndexWriterConfig::stores_doc_values gates every future write and a stale on-disk column is simply never read once the schema says false. Engine::update_field itself needs no changes: its Reindex/Destructive dispatch already calls InvertedIndex::rebuild_field generically, which was updated in the previous commit to derive target_doc_values from the field's new option. Extends classify_change_table with doc_values cases for Text (both directions, plus the stored:false destructive variant) and one representative non-Text case each for Integer and Boolean, proving the fold applies uniformly across arms. Refs #1047
Adds optional bool doc_values to the 7 proto field-option messages that carry the flag (not BytesOption, which has none). Tri-state for the same reason as term_vectors (#1083): a plain bool cannot distinguish "the client omitted this" from "the client explicitly set it to false", and the engine default is true. - convert/schema.rs: field_option_to_proto / field_option_from_proto carry doc_values through both directions, unset defaulting to true. - gateway/convert.rs: json_to_proto_field_option reads an optional doc_values key; proto_field_option_to_json's six non-Text arms (previously fixed-shape json! literals) are rewritten to conditionally-mutated objects, matching the Text arm's existing pattern, so doc_values is only emitted when explicitly set. Tests: proto round-trip (explicit true/false, and unset defaulting to true) for Text and Integer; JSON round-trip with the same tri-state contract, mirroring the existing base_weight tests. Refs #1047
…I wizard Adds a doc_values parameter (default true) to the seven add_*_field schema-builder methods across laurus-python, laurus-ruby, laurus-nodejs, laurus-wasm, and laurus-php, mirroring each binding's existing term_vectors parameter: - Python: keyword-only pyo3 signature (doc_values=true), inserted before analyzer on add_text_field. - Node.js / WASM: positional Option<bool>, inserted before the trailing analyzer param on add_text_field (same precedent term_vectors already set there). - PHP: #[php(defaults(...))] entry plus a plain bool parameter. - Ruby: extended each add_*_field's get_kwargs type tuple and name list by one Option<bool> slot; add_text_field's tuple grows to 5, still well under magnus's 9-element ScanArgsOpt limit. add_text_field's extra parameter on Python pushed it to 8 arguments, tripping clippy::too_many_arguments; added the same #[allow(...)] the vector field builders already carry for the same reason. laurus-cli's create wizard gets a "Doc values?" Confirm prompt (default true) in both prompt_text_option and prompt_indexed_stored_option. Tests: new laurus-python/tests/test_doc_values.py and laurus-ruby/test/test_doc_values.rb -- doc_values: false keeps a stored field retrievable, doesn't affect searchability, and every field option that carries the flag accepts it. Neither binding exposes field-sorted search or faceting yet, so these can't observe the DocValues column itself disappearing on disk; that's covered at the Rust level (Phases 3-4). Full workspace check/clippy/fmt clean; laurus-python's 107 pytest cases and laurus-ruby's 81 minitest cases all pass. Refs #1047
Documents the doc_values: bool schema option (default true, Issue #1047) across both the English and Japanese docs trees: - concepts/schema_and_fields.md: added to the TextOption builder example and option table, plus a shared note explaining it applies to every lexical field option except BytesOption. - laurus/faceting.md: corrected the "every stored field is written to DocValues" statement, which the opt-out makes inaccurate -- it's now "every stored field not excluded by type or by doc_values: false". - laurus-cli/schema_format.md: added to all 7 carrying field types' TOML snippets and option tables, plus a new "Common option: doc_values" section spelling out the effective rule (stored && doc_values) and BytesOption's exclusion. - laurus-server/grpc_api.md: added to the FieldOption oneof summary table, plus a note on the tri-state (optional bool) wire contract, mirroring term_vectors. - All 5 bindings' api_reference.md (laurus-python, laurus-ruby, laurus-nodejs, laurus-wasm, laurus-php): added doc_values to all 7 add_*_field signatures. Along the way, fixed a pre-existing wrong default in laurus-php's docs (addTextField's $termVectors was documented as defaulting to false; the actual default is true). laurus-server/http_gateway.md, concepts/indexing/lexical_indexing.md, and laurus/api_reference.md were left unchanged: the first has no option table (only bare JSON examples), the second never mentioned term_vectors either (segment-format overview, not a field-option reference), and the third documents SchemaBuilder methods by option struct name only, without enumerating struct fields. `mdbook build` clean for both docs/ and docs/ja/. Refs #1047
A second TextOption struct literal in a doc comment (the FieldOption enum's own example, distinct from the Field struct's example fixed in an earlier commit) was missed when doc_values was added to the struct, failing `cargo test -p laurus --doc` under --features embeddings-all. Refs #1047
46 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a per-field
doc_values: boolschema option (defaulttrue) so a field's value can be excluded from DocValues (the column-oriented store sort/facet/aggregation read from) without losing stored-field retrieval — closing the write-amplification gap described in #1047. Along the way, fixes a latent mixed-segment correctness bug the flag would otherwise have surfaced constantly, and makesDocValuesReaderload lazily per field instead of materializing every field on first touch.What changed, by phase
Phase 1 — mixed-segment fallback bug fix (prerequisite).
InvertedIndexReader::has_doc_valuesis an index-wideany(...)across segments, not a per-document guarantee.TopFieldCollector::get_field_valueandFacetCollector::collect_docboth collapsed aget_doc_valuemiss underhas_dv == truetoNull/ "no contribution" instead of falling back to the stored document — unreachable via the existing "no segment has the column at all" case (#1053), but real whenever segments disagree (mixed old/new segments, or adoc_values: falsefield). Fixed both, with unit and on-disk integration tests (.dvfile deleted from one of two segments), each RED-proven.Phase 2 — lazy per-field DocValues loading + robustness (no behavior change).
DocValuesReader::loadused to deserialize every field's payload on first access. It now reads only the header and each field's directory entry, materializing a field'sdoc_id -> valuemap lazily and caching it. Added header-declared-size bounds checking (reusingchecked_capacity/checked_len, moved fromvector::index::alloc_boundstoutil::alloc_bounds) and made.dvbyte output deterministic (HashMap->BTreeMap, sortedvalues_vec).Phase 3 — the
doc_valuesflag itself. Added toTextOption,IntegerOption,FloatOption,BooleanOption,DateTimeOption,GeoOption,Geo3dOption(notBytesOption— DocValues never carriesBytes/Vectorvalues regardless). Effective rule: a column is written only whenstored && doc_valuesare both true. Wired throughInvertedIndexConfig/LexicalIndexConfig(index-wide default) andInvertedIndexWriterConfig::stores_doc_values(3-level resolution mirroring#1083'sstores_term_positions).Phase 4 — merge / field-rebuild path. Deliberately not mirroring how #1083 handled
term_vectors: a discarded term position is unrecoverable, so that flag is detection-only. A DocValues column can always be regenerated fromstored_fields, so here the current schema wins over whatever a source segment has on disk (MergeConfig::field_doc_values), falling back to per-segment detection only for schema-undeclared fields. Covered by three new merge-path tests (independent per-field state, schema-priority winning over a stale on-disk column, and a value-less field not being misdetected as opted out).Phase 5 — schema-change classification.
classify_doc_values(reusesclassify_indexed_only's exact off->on/on->off logic) folded intoclassify_change's 7 lexical arms.Phase 6 — server surface.
optional bool doc_valueson the 7 proto field-option messages (tri-state, same reasoning asterm_vectors), wired through bothconvert/schema.rsandgateway/convert.rs(JSON).Phase 7 — bindings + CLI.
doc_valuesparameter added to all 7add_*_fieldmethods across Python, Ruby, Node.js, WASM, and PHP, plus a wizard prompt inlaurus-cli. New integration tests inlaurus-python/laurus-ruby(neither binding exposes field-sorted search or faceting yet, so they cover what's observable at that layer: stored-field retrieval and searchability are unaffected).Phase 8 — docs (EN + JA).
schema_and_fields.md,faceting.md(corrected a statement the opt-out makes inaccurate),schema_format.md,grpc_api.md, and all 5 bindings'api_reference.md. Also fixed a pre-existing wrong default inlaurus-php's docs ($termVectorswas documented as defaulting tofalse; it'strue).Phase 9 — verification. Full workspace test suite,
cargo checkon every binding, manual backward-compat check (an existing schema with nodoc_valueskey loads, commits, and searches identically, with the flag resolving totrue). Filed #1114 for an unrelated pre-existing gap found during investigation:DocumentParserignoresindexed/storedschema settings entirely (does not undermine this PR — the production ingestion path,InvertedIndexWriter::analyze_document, does consult the schema).Test plan
cargo test --workspace --features embeddings-all— 106 test binaries, all greencargo fmt --all -- --check— cleancargo clippy --workspace --all-targets --features embeddings-all -- -D warnings— cleancargo checkon every binding crate +laurus-wasm --target wasm32-unknown-unknown --testslaurus-python(107 pytest cases) andlaurus-ruby(81 minitest cases) built viamaturin develop/rake compileand run directlymdbook buildclean for bothdocs/anddocs/ja/doc_valueskey behaves exactly as before (loads, indexes, searches, retrieves)Closes #1047
Refs #547, #555, #548