From 985ee2677f722c29bdbcbd036ff1a325f24651c3 Mon Sep 17 00:00:00 2001 From: Paul Masurel Date: Fri, 4 Sep 2026 14:26:05 +0200 Subject: [PATCH] CR comments --- .../doc_predicate_query/function_predicate.rs | 60 +++++------------ src/query/doc_predicate_query/mod.rs | 64 ++++++------------- 2 files changed, 38 insertions(+), 86 deletions(-) diff --git a/src/query/doc_predicate_query/function_predicate.rs b/src/query/doc_predicate_query/function_predicate.rs index 30d1d7d77..cce7785db 100644 --- a/src/query/doc_predicate_query/function_predicate.rs +++ b/src/query/doc_predicate_query/function_predicate.rs @@ -3,18 +3,11 @@ use crate::index::SegmentReader; use crate::DocId; /// Blanket [`SegmentDocPredicate`] implementation for any per-document -/// closure. -/// -/// Hidden contract: [`SegmentDocPredicate::eval`] takes `&self`, not -/// `&mut self`, so the closure must be callable through an immutable -/// reference (`Fn`, not `FnMut`). A caller that needs per-document mutable -/// state must capture it behind its own synchronization primitive (for -/// example `Arc>` or `Arc`) rather than relying on -/// interior mutability provided by this blanket impl. +/// function. impl SegmentDocPredicate for F where F: Fn(DocId) -> bool + Send + Sync + 'static { - fn eval(&self, doc_id: DocId) -> bool { + fn eval(&mut self, doc_id: DocId) -> bool { (self)(doc_id) } } @@ -23,43 +16,23 @@ where F: Fn(DocId) -> bool + Send + Sync + 'static /// /// `FunctionPredicate` wraps a factory closure that is called once per /// segment (via [`DocPredicate::doc_predicate`]) and returns a per-document -/// closure evaluated once per candidate document. This predicate has no -/// dependency on fast fields or any other segment data structure: the -/// factory receives the [`SegmentReader`] and is free to ignore it entirely, -/// for example to build a predicate over `doc_id` alone. -/// -/// # Example -/// -/// ``` -/// use tantivy::query::doc_predicate_query::FunctionPredicate; -/// use tantivy::query::doc_predicate_query::DocPredicateQuery; -/// use tantivy::index::SegmentReader; -/// use tantivy::DocId; -/// -/// // Matches every even doc id, in every segment. -/// let predicate = FunctionPredicate::new(|_segment_reader: &SegmentReader| { -/// Ok(move |doc_id: DocId| doc_id % 2 == 0) -/// }); -/// let _query: DocPredicateQuery = predicate.into(); -/// ``` +/// closure evaluated once per candidate document. pub struct FunctionPredicate { - factory: F, + segment_predicate_factory: F, } -impl FunctionPredicate { - /// Creates a new predicate from a per-segment factory function. - /// - /// `factory` is called once per segment and must return a per-document - /// closure implementing `Fn(DocId) -> bool`. - pub fn new(factory: F) -> Self { - Self { factory } +impl From for FunctionPredicate +where + F: Fn(&SegmentReader) -> crate::Result + Send + 'static, + SegmentF: SegmentDocPredicate, +{ + fn from(segment_predicate_factory: F) -> Self { + Self { + segment_predicate_factory, + } } } -// `F` is a plain closure or function type and will not generally implement -// `Debug`. `DocPredicate` requires `Debug`, so this manual impl exists purely -// to satisfy that bound; it intentionally does not attempt to expose the -// closure's captured state. impl std::fmt::Debug for FunctionPredicate { fn fmt(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { formatter @@ -76,7 +49,7 @@ where type SegmentDocPredicate = SegmentF; fn doc_predicate(&self, segment_reader: &SegmentReader) -> crate::Result { - (self.factory)(segment_reader) + (self.segment_predicate_factory)(segment_reader) } } @@ -84,13 +57,14 @@ where mod tests { use super::*; use crate::collector::Count; - use crate::query::doc_predicate_query::{create_index_for_test, DocPredicateQuery}; + use crate::query::doc_predicate_query::tests::create_index_for_test; + use crate::query::doc_predicate_query::DocPredicateQuery; #[test] fn test_function_predicate_matches_even_doc_ids() { let index = create_index_for_test(4); let searcher = index.reader().unwrap().searcher(); - let predicate = FunctionPredicate::new(|_segment_reader: &SegmentReader| { + let predicate = FunctionPredicate::from(|_segment_reader: &SegmentReader| { Ok(move |doc_id: DocId| doc_id % 2 == 0) }); let query: DocPredicateQuery = predicate.into(); diff --git a/src/query/doc_predicate_query/mod.rs b/src/query/doc_predicate_query/mod.rs index 904807ac6..46e3073b4 100644 --- a/src/query/doc_predicate_query/mod.rs +++ b/src/query/doc_predicate_query/mod.rs @@ -1,6 +1,5 @@ use std::sync::Arc; -/// A [`DocPredicate`] built from a plain Rust factory function. mod function_predicate; pub use function_predicate::FunctionPredicate; @@ -11,11 +10,7 @@ use crate::query::explanation::does_not_match; use crate::query::{ConstScorer, EnableScoring, Explanation, Query, Scorer, Weight}; use crate::{DocId, DocSet, Score}; -/// A query that evaluates a boolean JIT expression against fast-field columns. -/// -/// Variable names in the expression are resolved as fast-field names for each -/// segment. Multivalued columns contribute their first value. A document with a -/// missing input value does not match. +/// A query that evaluates, for each DocId, whether it matches or not. #[derive(Clone, Debug)] pub struct DocPredicateQuery { predicate: Arc, @@ -118,8 +113,7 @@ impl DocPredicateDocSet return target, SeekDangerResult::SeekLowerBound(next_target) => { - if next_target >= self.max_doc { - self.doc = TERMINATED; + if next_target >= TERMINATED { return TERMINATED; } target = next_target; @@ -128,11 +122,8 @@ impl DocPredicateDocSet` regardless of the concrete predicate type. pub trait DocPredicateBoxable: std::fmt::Debug + 'static + Send + Sync { /// Builds a [`Scorer`] over the predicate's matching documents in the /// given segment. @@ -181,10 +172,8 @@ impl DocPredicateBoxable for TDocPredicate { /// A per-query predicate that produces a [`SegmentDocPredicate`] for each /// segment. /// -/// Implementing this trait is all that's needed to make a type usable as a -/// [`DocPredicateQuery`]: [`DocPredicateBoxable`] and -/// `From for DocPredicateQuery` are both implemented -/// generically for every [`DocPredicate`]. +/// Implementing this trait is all that's needed to make a type usable in a +/// [`DocPredicateQuery`]. pub trait DocPredicate: Send + Sync + 'static + std::fmt::Debug { /// The per-segment predicate produced by [`Self::doc_predicate`]. type SegmentDocPredicate: SegmentDocPredicate; @@ -201,41 +190,30 @@ pub trait DocPredicate: Send + Sync + 'static + std::fmt::Debug { } /// The per-segment predicate produced by a [`DocPredicate`]. -pub trait SegmentDocPredicate: Send + Sync + 'static { +pub trait SegmentDocPredicate: Send + 'static { /// Returns whether `doc_id` matches the predicate. - /// - /// Hidden contract: this takes `&self`, not `&mut self`, so any scratch - /// state used during evaluation must be behind interior mutability (see - /// `jitexpr_predicate`'s use of `RefCell`) or a synchronization - /// primitive, rather than plain mutable fields. - fn eval(&self, doc_id: DocId) -> bool; -} - -/// Builds an in-RAM index with an empty schema and `num_docs` empty documents. -/// -/// Shared across the test modules of this module tree so each -/// `DocPredicate` implementation's tests don't each define their own. -#[cfg(test)] -fn create_index_for_test(num_docs: u32) -> crate::Index { - let schema_builder = crate::schema::Schema::builder(); - let schema = schema_builder.build(); - let index = crate::Index::create_in_ram(schema); - let mut writer = index.writer_for_tests().unwrap(); - for _ in 0..num_docs { - writer.add_document(doc!()).unwrap(); - } - writer.commit().unwrap(); - index + fn eval(&mut self, doc_id: DocId) -> bool; } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; use crate::collector::Count; - use crate::query::doc_predicate_query::FunctionPredicate; + + pub(crate) fn create_index_for_test(num_docs: u32) -> crate::Index { + let schema_builder = crate::schema::Schema::builder(); + let schema = schema_builder.build(); + let index = crate::Index::create_in_ram(schema); + let mut writer = index.writer_for_tests().unwrap(); + for _ in 0..num_docs { + writer.add_document(doc!()).unwrap(); + } + writer.commit().unwrap(); + index + } fn even_doc_id_query() -> DocPredicateQuery { - FunctionPredicate::new(|_segment_reader: &SegmentReader| { + FunctionPredicate::from(|_segment_reader: &SegmentReader| { Ok(move |doc_id: DocId| doc_id % 2 == 0) }) .into()