Skip to content

Commit

Permalink
Fix missing fieldnorm indexing
Browse files Browse the repository at this point in the history
Fixes broken search (no results) with BM25 for u64, i64, f64, bool, bytes and date after deletion and merge.
There were no fieldnorms recorded for those field. After merge InvertedIndexReader::total_num_tokens returns 0 (Sum over the fieldnorms is 0). BM25 does not work when total_num_tokens is 0.
Fixes #1617
  • Loading branch information
PSeitz committed Oct 14, 2022
1 parent 4b4c231 commit a1f688d
Show file tree
Hide file tree
Showing 2 changed files with 208 additions and 8 deletions.
184 changes: 176 additions & 8 deletions src/indexer/index_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,9 @@ mod tests {
TextFieldIndexing, TextOptions, FAST, INDEXED, STORED, STRING, TEXT,
};
use crate::store::DOCSTORE_CACHE_CAPACITY;
use crate::{DocAddress, Index, IndexSettings, IndexSortByField, Order, ReloadPolicy, Term};
use crate::{
DateTime, DocAddress, Index, IndexSettings, IndexSortByField, Order, ReloadPolicy, Term,
};

const LOREM: &str = "Doc Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do \
eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad \
Expand Down Expand Up @@ -1601,6 +1603,9 @@ mod tests {
IpAddrOptions::default().set_fast(Cardinality::MultiValues),
);
let id_field = schema_builder.add_u64_field("id", FAST | INDEXED | STORED);
let i64_field = schema_builder.add_i64_field("i64", INDEXED);
let f64_field = schema_builder.add_f64_field("f64", INDEXED);
let date_field = schema_builder.add_date_field("date", INDEXED);
let bytes_field = schema_builder.add_bytes_field("bytes", FAST | INDEXED | STORED);
let bool_field = schema_builder.add_bool_field("bool", FAST | INDEXED | STORED);
let text_field = schema_builder.add_text_field(
Expand Down Expand Up @@ -1664,6 +1669,9 @@ mod tests {
multi_numbers=> id,
multi_numbers => id,
bool_field => (id % 2u64) != 0,
i64_field => id as i64,
f64_field => id as f64,
date_field => DateTime::from_timestamp_secs(id as i64),
multi_bools => (id % 2u64) != 0,
multi_bools => (id % 2u64) == 0,
text_field => id.to_string(),
Expand All @@ -1679,6 +1687,9 @@ mod tests {
multi_numbers=> id,
multi_numbers => id,
bool_field => (id % 2u64) != 0,
i64_field => id as i64,
f64_field => id as f64,
date_field => DateTime::from_timestamp_secs(id as i64),
multi_bools => (id % 2u64) != 0,
multi_bools => (id % 2u64) == 0,
text_field => id.to_string(),
Expand Down Expand Up @@ -1889,10 +1900,8 @@ mod tests {
}
}
// test search
let my_text_field = index.schema().get_field("text_field").unwrap();

let do_search = |term: &str| {
let query = QueryParser::for_index(&index, vec![my_text_field])
let do_search = |term: &str, field| {
let query = QueryParser::for_index(&index, vec![field])
.parse_query(term)
.unwrap();
let top_docs: Vec<(f32, DocAddress)> =
Expand All @@ -1901,11 +1910,57 @@ mod tests {
top_docs.iter().map(|el| el.1).collect::<Vec<_>>()
};

let do_search2 = |term: Term| {
let query = TermQuery::new(term, IndexRecordOption::Basic);
let top_docs: Vec<(f32, DocAddress)> =
searcher.search(&query, &TopDocs::with_limit(1000)).unwrap();

top_docs.iter().map(|el| el.1).collect::<Vec<_>>()
};

for (existing_id, count) in expected_ids_and_num_occurrences {
assert_eq!(do_search(&existing_id.to_string()).len() as u64, count);
let assert_field = |field| do_search(&existing_id.to_string(), field).len() as u64;
assert_eq!(assert_field(text_field), count);
assert_eq!(assert_field(i64_field), count);
assert_eq!(assert_field(f64_field), count);
assert_eq!(assert_field(id_field), count);

// Test bool
let term = Term::from_field_bool(bool_field, (existing_id % 2u64) != 0);
assert_eq!(do_search2(term).len() as u64, count);

// Test bytes
let term = Term::from_field_bytes(bytes_field, existing_id.to_le_bytes().as_slice());
assert_eq!(do_search2(term).len() as u64, count);

// Test date
let term = Term::from_field_date(
date_field,
DateTime::from_timestamp_secs(existing_id as i64),
);
assert_eq!(do_search2(term).len() as u64, count);
}
for existing_id in deleted_ids {
assert_eq!(do_search(&existing_id.to_string()).len(), 0);
for deleted_id in deleted_ids {
let assert_field = |field| {
assert_eq!(do_search(&deleted_id.to_string(), field).len() as u64, 0);
};
assert_field(text_field);
assert_field(f64_field);
assert_field(i64_field);
assert_field(id_field);

// Test bool
let term = Term::from_field_bool(bool_field, (deleted_id % 2u64) != 0);
assert_eq!(do_search2(term).len() as u64, 0);

// Test bytes
let term = Term::from_field_bytes(bool_field, deleted_id.to_le_bytes().as_slice());
assert_eq!(do_search2(term).len() as u64, 0);

// Test date
let term =
Term::from_field_date(date_field, DateTime::from_timestamp_secs(deleted_id as i64));
assert_eq!(do_search2(term).len() as u64, 0);
}
// test facets
for segment_reader in searcher.segment_readers().iter() {
Expand Down Expand Up @@ -2050,4 +2105,117 @@ mod tests {
index_writer.commit()?;
Ok(())
}

#[test]
fn test_bug_1617_2() {
assert!(test_operation_strategy(
&[
IndexingOp::AddDoc { id: 13 },
IndexingOp::DeleteDoc { id: 13 },
IndexingOp::Commit,
IndexingOp::AddDoc { id: 30 },
IndexingOp::Commit,
IndexingOp::Merge,
],
false,
true
)
.is_ok());
}

#[test]
fn test_bug_1617() -> crate::Result<()> {
let mut schema_builder = schema::Schema::builder();
let id_field = schema_builder.add_u64_field("id", INDEXED);

let schema = schema_builder.build();
let index = Index::builder().schema(schema).create_in_ram()?;
let mut index_writer = index.writer_for_tests()?;
index_writer.set_merge_policy(Box::new(NoMergePolicy));

let existing_id = 16u64;
let deleted_id = 13u64;
index_writer.add_document(doc!(
id_field=>existing_id,
))?;
index_writer.add_document(doc!(
id_field=>deleted_id,
))?;
index_writer.delete_term(Term::from_field_u64(id_field, deleted_id));
index_writer.commit()?;

// Merge
{
assert!(index_writer.wait_merging_threads().is_ok());
let mut index_writer = index.writer_for_tests()?;
let segment_ids = index
.searchable_segment_ids()
.expect("Searchable segments failed.");
index_writer.merge(&segment_ids).wait().unwrap();
assert!(index_writer.wait_merging_threads().is_ok());
}
let searcher = index.reader()?.searcher();

let query = TermQuery::new(
Term::from_field_u64(id_field, existing_id),
IndexRecordOption::Basic,
);
let top_docs: Vec<(f32, DocAddress)> =
searcher.search(&query, &TopDocs::with_limit(10)).unwrap();

assert_eq!(top_docs.len(), 1); // Fails

Ok(())
}

#[test]
fn test_bug_1618() -> crate::Result<()> {
let mut schema_builder = schema::Schema::builder();
let id_field = schema_builder.add_i64_field("id", INDEXED);

let schema = schema_builder.build();
let index = Index::builder().schema(schema).create_in_ram()?;
let mut index_writer = index.writer_for_tests()?;
index_writer.set_merge_policy(Box::new(NoMergePolicy));

index_writer.add_document(doc!(
id_field=>10i64,
))?;
index_writer.add_document(doc!(
id_field=>30i64,
))?;
index_writer.commit()?;

// Merge
{
assert!(index_writer.wait_merging_threads().is_ok());
let mut index_writer = index.writer_for_tests()?;
let segment_ids = index
.searchable_segment_ids()
.expect("Searchable segments failed.");
index_writer.merge(&segment_ids).wait().unwrap();
assert!(index_writer.wait_merging_threads().is_ok());
}
let searcher = index.reader()?.searcher();

let query = TermQuery::new(
Term::from_field_i64(id_field, 10i64),
IndexRecordOption::Basic,
);
let top_docs: Vec<(f32, DocAddress)> =
searcher.search(&query, &TopDocs::with_limit(10)).unwrap();

assert_eq!(top_docs.len(), 1); // Fails

let query = TermQuery::new(
Term::from_field_i64(id_field, 30i64),
IndexRecordOption::Basic,
);
let top_docs: Vec<(f32, DocAddress)> =
searcher.search(&query, &TopDocs::with_limit(10)).unwrap();

assert_eq!(top_docs.len(), 1); // Fails

Ok(())
}
}
32 changes: 32 additions & 0 deletions src/indexer/segment_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,12 @@ impl SegmentWriter {
if !field_entry.is_indexed() {
continue;
}

let (term_buffer, ctx) = (&mut self.term_buffer, &mut self.ctx);
let postings_writer: &mut dyn PostingsWriter =
self.per_field_postings_writers.get_for_field_mut(field);
term_buffer.set_field(field_entry.field_type().value_type(), field);

match *field_entry.field_type() {
FieldType::Facet(_) => {
for value in values {
Expand Down Expand Up @@ -240,46 +242,76 @@ impl SegmentWriter {
}
}
FieldType::U64(_) => {
let mut num_vals = 0;
for value in values {
num_vals += 1;
let u64_val = value.as_u64().ok_or_else(make_schema_error)?;
term_buffer.set_u64(u64_val);
postings_writer.subscribe(doc_id, 0u32, term_buffer, ctx);
}
if field_entry.has_fieldnorms() {
self.fieldnorms_writer.record(doc_id, field, num_vals);
}
}
FieldType::Date(_) => {
let mut num_vals = 0;
for value in values {
num_vals += 1;
let date_val = value.as_date().ok_or_else(make_schema_error)?;
term_buffer.set_u64(date_val.truncate(DatePrecision::Seconds).to_u64());
postings_writer.subscribe(doc_id, 0u32, term_buffer, ctx);
}
if field_entry.has_fieldnorms() {
self.fieldnorms_writer.record(doc_id, field, num_vals);
}
}
FieldType::I64(_) => {
let mut num_vals = 0;
for value in values {
num_vals += 1;
let i64_val = value.as_i64().ok_or_else(make_schema_error)?;
term_buffer.set_i64(i64_val);
postings_writer.subscribe(doc_id, 0u32, term_buffer, ctx);
}
if field_entry.has_fieldnorms() {
self.fieldnorms_writer.record(doc_id, field, num_vals);
}
}
FieldType::F64(_) => {
let mut num_vals = 0;
for value in values {
num_vals += 1;
let f64_val = value.as_f64().ok_or_else(make_schema_error)?;
term_buffer.set_f64(f64_val);
postings_writer.subscribe(doc_id, 0u32, term_buffer, ctx);
}
if field_entry.has_fieldnorms() {
self.fieldnorms_writer.record(doc_id, field, num_vals);
}
}
FieldType::Bool(_) => {
let mut num_vals = 0;
for value in values {
num_vals += 1;
let bool_val = value.as_bool().ok_or_else(make_schema_error)?;
term_buffer.set_bool(bool_val);
postings_writer.subscribe(doc_id, 0u32, term_buffer, ctx);
}
if field_entry.has_fieldnorms() {
self.fieldnorms_writer.record(doc_id, field, num_vals);
}
}
FieldType::Bytes(_) => {
let mut num_vals = 0;
for value in values {
num_vals += 1;
let bytes = value.as_bytes().ok_or_else(make_schema_error)?;
term_buffer.set_bytes(bytes);
postings_writer.subscribe(doc_id, 0u32, term_buffer, ctx);
}
if field_entry.has_fieldnorms() {
self.fieldnorms_writer.record(doc_id, field, num_vals);
}
}
FieldType::JsonObject(_) => {
let text_analyzer = &self.per_field_text_analyzers[field.field_id() as usize];
Expand Down

0 comments on commit a1f688d

Please sign in to comment.