fix(lexical): honor indexed/stored field options in DocumentParser - #1119
Merged
Merged
Conversation
… behind accessors The 8-arm match deriving (indexed, stored) from a field's FieldOption was duplicated in InvertedIndexWriter::analyze_document (both flags) and Engine::update_field's rebuild path (indexed only). Adding a third copy for DocumentParser (#1114) crossed the threshold to factor it out, mirroring the FieldOption::doc_values() precedent from #1047: - FieldOption::indexed(&self) -> bool: Bytes always false (BytesOption has no indexed flag of its own -- a binary payload has no term representation to index). - FieldOption::stored(&self) -> bool: Bytes reads its own stored flag. Pure refactor: both call sites now delegate to the accessors, with no behavior change (verified by the existing test suites). Adds a direct unit test covering all 8 variants, including Bytes's asymmetric indexed()/stored() behavior. Refs #1114
DocumentParser::parse ignored the schema entirely, unconditionally indexing and storing every field regardless of its FieldOption. The production ingestion path (InvertedIndexWriter::analyze_document) already gates on (indexed, stored); DocumentParser did not, even though its own doc comment on InvertedIndexWriter::add_analyzed_document recommends parsing through it for "explicit control". Non-breaking: DocumentParser::new(analyzer) is unchanged and still means schema-less (index everything), matching every existing caller (3 doctests, 3 unit tests) with zero changes. A new with_fields(fields) consuming builder attaches the same HashMap<String, FieldOption> shape InvertedIndexWriterConfig::fields already uses, switching the parser into schema-aware mode. A new private field_flags(field_name) mirrors analyze_document's exact resolution: a declared field follows its own option; an internal (_-prefixed) field or any field while schema-less defaults to index-and-store; a field absent from an otherwise non-empty schema is skipped entirely. parse() itself keeps its existing per-type term/point generation code untouched -- only restructured to gate on (should_index, should_store) before running it, with every arm's `stored_fields.insert` collapsed into one call at the end of the loop (each arm's reconstructed value was already equivalent to `field.clone()`). Deliberately does not delegate to InvertedIndexWriter::analyze_field_value: the two implementations diverge in more than gating (term frequency/position grouping, and DateTime's indexed representation), and delegating would change DocumentParser's output shape, risking a behavior change for existing schema-less callers. Adds 7 tests covering indexed: false (terms + lengths excluded), stored: false, point-value gating, Bytes.stored, schema-less passthrough, schema-undeclared-field skipping, and `_`-prefixed internal fields bypassing the schema. All 5 correctness-sensitive assertions RED-proven (temporarily short-circuited field_flags to always return schema-less and confirmed the exact expected failures). Refs #1114
Adds a "Schema Awareness" module section and a third doctest showing with_fields() excluding an indexed: false field from field_terms while keeping it in stored_fields (Issue #1114). Updates the struct doc and add_analyzed_document's example (previously schema-less) to use with_fields(config.fields.clone()), since that doc actively recommends the DocumentParser::parse() -> add_analyzed_document() pattern for "explicit control" and should demonstrate the gate that makes that control real. Refs #1114
31 tasks
9 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
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
DocumentParser::parseignored the schema entirely, unconditionally indexing and storing every field regardless of itsFieldOption. The production ingestion path (InvertedIndexWriter::analyze_document) already gates on(indexed, stored);DocumentParserdid not, even thoughInvertedIndexWriter::add_analyzed_document's own doc comment recommends parsing through it for "explicit control".Investigation confirmed
DocumentParseris currently unreachable from any production path (only 3 doctests + 3 unit tests exercised it), so this carried no runtime risk today -- but it's a public, re-exported type, and the doc-recommended usage pattern would have silently bypassed schema enforcement for anyone following it.What changed
Non-breaking API.
DocumentParser::new(analyzer)is unchanged and still means schema-less (index everything) -- every existing caller (3 doctests, 3 unit tests) needed zero changes. A newwith_fields(fields: HashMap<String, FieldOption>)consuming builder attaches the same map shapeInvertedIndexWriterConfig::fieldsalready uses, switching the parser into schema-aware mode. A new privatefield_flags(field_name)mirrorsanalyze_document's exact resolution: a declared field follows its own option; an internal (_-prefixed) field or any field while schema-less defaults to index-and-store; a field absent from an otherwise non-empty schema is skipped entirely.No delegation to
analyze_field_value. Compared the two implementations directly:parser.rs's privatetokens_to_analyzed_termsgroups by term text (one entry per unique term, summed frequency), whilewriter.rs's groups per token occurrence (one entry per occurrence, running frequency counter);DateTimeindexing text/point precision also differs. Delegating would changeDocumentParser's output shape, risking the backward-compat requirement. So this PR only ports the(should_index, should_store)gating decision intoparser.rs, leaving its existing per-type term-generation code untouched -- restructured only to gate on the resolved flags, with every arm'sstored_fields.insertcollapsed into one call at the end of the loop (each arm's reconstructed value was already equivalent tofield.clone()).Centralized a growing duplicate match. The
FieldOption8-arm(indexed, stored)derivation was already duplicated inInvertedIndexWriter::analyze_documentandEngine::update_field's rebuild path (indexed only). Adding a third copy here crossed the threshold to factor it out, mirroring theFieldOption::doc_values()precedent from #1047: newpub(crate) fn indexed(&self) -> bool/fn stored(&self) -> boolonFieldOption, with both existing call sites refactored to use them (pure refactor, no behavior change, first commit).Tests
7 new cases in
parser.rs:indexed: false(terms + lengths excluded),stored: false, point-value gating,Bytes.stored, schema-less passthrough (the backward-compat pin), schema-undeclared-field skipping, and_-prefixed internal fields bypassing the schema. Plus a direct unit test for the newFieldOption::indexed()/stored()accessors covering all 8 variants. Every correctness-sensitive assertion was RED-proven (temporarily short-circuitedfield_flagsto always return schema-less and confirmed the exact expected failures).Docs
Added a "Schema Awareness" module section and a third doctest demonstrating
with_fields(). Updatedadd_analyzed_document's example (previously schema-less) to usewith_fields(config.fields.clone()), since that doc actively recommends this exact usage pattern.Scope note (not fixed here)
While comparing
parser.rsandwriter.rs::analyze_field_value, found the term-frequency/position andDateTime-representation divergence described above is a real, separate bug for anyone actually using theDocumentParser::parse()->add_analyzed_document()pattern (repeated-term frequency ends up wrong). Left out of this PR since fixing it changesDocumentParser's output shape and risks the backward-compat criterion here; noting it in case a maintainer wants to track it separately.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 test -p laurus --doc lexical::core::parserand theadd_analyzed_documentdoctest -- both compile/passCloses #1114