diff --git a/Cargo.lock b/Cargo.lock index 5f192b6d1..ff4981d11 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1794,7 +1794,7 @@ checksum = "95505c38b4572b2d910cecb0281560f54b440a19336cbbcb27bf6ce6adc6f5a8" [[package]] name = "heed" version = "0.12.5" -source = "git+https://github.com/meilisearch/heed?tag=v0.12.5#4158a6c484752afaaf9e2530a6ee0e7ab0f24ee8" +source = "git+https://github.com/meilisearch/heed?tag=v0.12.6#8c5b94225fc949c02bb7b900cc50ffaf6b584b1e" dependencies = [ "byteorder", "heed-traits", @@ -1811,12 +1811,12 @@ dependencies = [ [[package]] name = "heed-traits" version = "0.7.0" -source = "git+https://github.com/meilisearch/heed?tag=v0.12.5#4158a6c484752afaaf9e2530a6ee0e7ab0f24ee8" +source = "git+https://github.com/meilisearch/heed?tag=v0.12.6#8c5b94225fc949c02bb7b900cc50ffaf6b584b1e" [[package]] name = "heed-types" version = "0.7.2" -source = "git+https://github.com/meilisearch/heed?tag=v0.12.5#4158a6c484752afaaf9e2530a6ee0e7ab0f24ee8" +source = "git+https://github.com/meilisearch/heed?tag=v0.12.6#8c5b94225fc949c02bb7b900cc50ffaf6b584b1e" dependencies = [ "bincode", "heed-traits", diff --git a/config.toml b/config.toml index 71872d0d4..c47989f56 100644 --- a/config.toml +++ b/config.toml @@ -126,3 +126,6 @@ ssl_tickets = false # Experimental metrics feature. For more information, see: # Enables the Prometheus metrics on the `GET /metrics` endpoint. experimental_enable_metrics = false + +# Experimental RAM reduction during indexing, do not use in production, see: +experimental_reduce_indexing_memory_usage = false diff --git a/index-scheduler/src/batch.rs b/index-scheduler/src/batch.rs index c88234809..67f70d367 100644 --- a/index-scheduler/src/batch.rs +++ b/index-scheduler/src/batch.rs @@ -24,6 +24,7 @@ use std::io::BufWriter; use dump::IndexMetadata; use log::{debug, error, info}; +use meilisearch_types::error::Code; use meilisearch_types::heed::{RoTxn, RwTxn}; use meilisearch_types::milli::documents::{obkv_to_object, DocumentsBatchReader}; use meilisearch_types::milli::heed::CompactionOption; @@ -1491,7 +1492,12 @@ fn delete_document_by_filter(filter: &serde_json::Value, index: Index) -> Result Ok(if let Some(filter) = filter { let mut wtxn = index.write_txn()?; - let candidates = filter.evaluate(&wtxn, &index)?; + let candidates = filter.evaluate(&wtxn, &index).map_err(|err| match err { + milli::Error::UserError(milli::UserError::InvalidFilter(_)) => { + Error::from(err).with_custom_error_code(Code::InvalidDocumentFilter) + } + e => e.into(), + })?; let mut delete_operation = DeleteDocuments::new(&mut wtxn, &index)?; delete_operation.delete_documents(&candidates); let deleted_documents = diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index 3a19ed4d2..acab850d1 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -46,6 +46,8 @@ impl From for Code { #[allow(clippy::large_enum_variant)] #[derive(Error, Debug)] pub enum Error { + #[error("{1}")] + WithCustomErrorCode(Code, Box), #[error("Index `{0}` not found.")] IndexNotFound(String), #[error("Index `{0}` already exists.")] @@ -144,6 +146,7 @@ impl Error { pub fn is_recoverable(&self) -> bool { match self { Error::IndexNotFound(_) + | Error::WithCustomErrorCode(_, _) | Error::IndexAlreadyExists(_) | Error::SwapDuplicateIndexFound(_) | Error::SwapDuplicateIndexesFound(_) @@ -176,11 +179,16 @@ impl Error { Error::PlannedFailure => false, } } + + pub fn with_custom_error_code(self, code: Code) -> Self { + Self::WithCustomErrorCode(code, Box::new(self)) + } } impl ErrorCode for Error { fn error_code(&self) -> Code { match self { + Error::WithCustomErrorCode(code, _) => *code, Error::IndexNotFound(_) => Code::IndexNotFound, Error::IndexAlreadyExists(_) => Code::IndexAlreadyExists, Error::SwapDuplicateIndexesFound(_) => Code::InvalidSwapDuplicateIndexFound, diff --git a/index-scheduler/src/index_mapper/index_map.rs b/index-scheduler/src/index_mapper/index_map.rs index d140d4944..9bed4fe5d 100644 --- a/index-scheduler/src/index_mapper/index_map.rs +++ b/index-scheduler/src/index_mapper/index_map.rs @@ -5,6 +5,7 @@ use std::collections::BTreeMap; use std::path::Path; use std::time::Duration; +use meilisearch_types::heed::flags::Flags; use meilisearch_types::heed::{EnvClosingEvent, EnvOpenOptions}; use meilisearch_types::milli::Index; use time::OffsetDateTime; @@ -53,6 +54,7 @@ pub struct IndexMap { pub struct ClosingIndex { uuid: Uuid, closing_event: EnvClosingEvent, + enable_mdb_writemap: bool, map_size: usize, generation: usize, } @@ -68,6 +70,7 @@ impl ClosingIndex { pub fn wait_timeout(self, timeout: Duration) -> Option { self.closing_event.wait_timeout(timeout).then_some(ReopenableIndex { uuid: self.uuid, + enable_mdb_writemap: self.enable_mdb_writemap, map_size: self.map_size, generation: self.generation, }) @@ -76,6 +79,7 @@ impl ClosingIndex { pub struct ReopenableIndex { uuid: Uuid, + enable_mdb_writemap: bool, map_size: usize, generation: usize, } @@ -103,7 +107,7 @@ impl ReopenableIndex { return Ok(()); } map.unavailable.remove(&self.uuid); - map.create(&self.uuid, path, None, self.map_size)?; + map.create(&self.uuid, path, None, self.enable_mdb_writemap, self.map_size)?; } Ok(()) } @@ -170,16 +174,17 @@ impl IndexMap { uuid: &Uuid, path: &Path, date: Option<(OffsetDateTime, OffsetDateTime)>, + enable_mdb_writemap: bool, map_size: usize, ) -> Result { if !matches!(self.get_unavailable(uuid), Missing) { panic!("Attempt to open an index that was unavailable"); } - let index = create_or_open_index(path, date, map_size)?; + let index = create_or_open_index(path, date, enable_mdb_writemap, map_size)?; match self.available.insert(*uuid, index.clone()) { InsertionOutcome::InsertedNew => (), InsertionOutcome::Evicted(evicted_uuid, evicted_index) => { - self.close(evicted_uuid, evicted_index, 0); + self.close(evicted_uuid, evicted_index, enable_mdb_writemap, 0); } InsertionOutcome::Replaced(_) => { panic!("Attempt to open an index that was already opened") @@ -212,17 +217,30 @@ impl IndexMap { /// | Closing | Closing | /// | Available | Closing | /// - pub fn close_for_resize(&mut self, uuid: &Uuid, map_size_growth: usize) { + pub fn close_for_resize( + &mut self, + uuid: &Uuid, + enable_mdb_writemap: bool, + map_size_growth: usize, + ) { let Some(index) = self.available.remove(uuid) else { return; }; - self.close(*uuid, index, map_size_growth); + self.close(*uuid, index, enable_mdb_writemap, map_size_growth); } - fn close(&mut self, uuid: Uuid, index: Index, map_size_growth: usize) { + fn close( + &mut self, + uuid: Uuid, + index: Index, + enable_mdb_writemap: bool, + map_size_growth: usize, + ) { let map_size = index.map_size().unwrap_or(DEFAULT_MAP_SIZE) + map_size_growth; let closing_event = index.prepare_for_closing(); let generation = self.next_generation(); - self.unavailable - .insert(uuid, Some(ClosingIndex { uuid, closing_event, map_size, generation })); + self.unavailable.insert( + uuid, + Some(ClosingIndex { uuid, closing_event, enable_mdb_writemap, map_size, generation }), + ); } /// Attempts to delete and index. @@ -282,11 +300,15 @@ impl IndexMap { fn create_or_open_index( path: &Path, date: Option<(OffsetDateTime, OffsetDateTime)>, + enable_mdb_writemap: bool, map_size: usize, ) -> Result { let mut options = EnvOpenOptions::new(); options.map_size(clamp_to_page_size(map_size)); options.max_readers(1024); + if enable_mdb_writemap { + unsafe { options.flag(Flags::MdbWriteMap) }; + } if let Some((created, updated)) = date { Ok(Index::new_with_creation_dates(options, path, created, updated)?) diff --git a/index-scheduler/src/index_mapper/mod.rs b/index-scheduler/src/index_mapper/mod.rs index 8754e7168..18aed42b0 100644 --- a/index-scheduler/src/index_mapper/mod.rs +++ b/index-scheduler/src/index_mapper/mod.rs @@ -66,6 +66,8 @@ pub struct IndexMapper { index_base_map_size: usize, /// The quantity by which the map size of an index is incremented upon reopening, in bytes. index_growth_amount: usize, + /// Whether we open a meilisearch index with the MDB_WRITEMAP option or not. + enable_mdb_writemap: bool, pub indexer_config: Arc, } @@ -132,15 +134,22 @@ impl IndexMapper { index_base_map_size: usize, index_growth_amount: usize, index_count: usize, + enable_mdb_writemap: bool, indexer_config: IndexerConfig, ) -> Result { + let mut wtxn = env.write_txn()?; + let index_mapping = env.create_database(&mut wtxn, Some(INDEX_MAPPING))?; + let index_stats = env.create_database(&mut wtxn, Some(INDEX_STATS))?; + wtxn.commit()?; + Ok(Self { index_map: Arc::new(RwLock::new(IndexMap::new(index_count))), - index_mapping: env.create_database(Some(INDEX_MAPPING))?, - index_stats: env.create_database(Some(INDEX_STATS))?, + index_mapping, + index_stats, base_path, index_base_map_size, index_growth_amount, + enable_mdb_writemap, indexer_config: Arc::new(indexer_config), }) } @@ -171,6 +180,7 @@ impl IndexMapper { &uuid, &index_path, date, + self.enable_mdb_writemap, self.index_base_map_size, )?; @@ -282,7 +292,11 @@ impl IndexMapper { .ok_or_else(|| Error::IndexNotFound(name.to_string()))?; // We remove the index from the in-memory index map. - self.index_map.write().unwrap().close_for_resize(&uuid, self.index_growth_amount); + self.index_map.write().unwrap().close_for_resize( + &uuid, + self.enable_mdb_writemap, + self.index_growth_amount, + ); Ok(()) } @@ -347,6 +361,7 @@ impl IndexMapper { &uuid, &index_path, None, + self.enable_mdb_writemap, self.index_base_map_size, )?; } diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index 40570c668..b6a156a06 100644 --- a/index-scheduler/src/lib.rs +++ b/index-scheduler/src/lib.rs @@ -233,6 +233,8 @@ pub struct IndexSchedulerOptions { pub task_db_size: usize, /// The size, in bytes, with which a meilisearch index is opened the first time of each meilisearch index. pub index_base_map_size: usize, + /// Whether we open a meilisearch index with the MDB_WRITEMAP option or not. + pub enable_mdb_writemap: bool, /// The size, in bytes, by which the map size of an index is increased when it resized due to being full. pub index_growth_amount: usize, /// The number of indexes that can be concurrently opened in memory. @@ -374,6 +376,11 @@ impl IndexScheduler { std::fs::create_dir_all(&options.indexes_path)?; std::fs::create_dir_all(&options.dumps_path)?; + if cfg!(windows) && options.enable_mdb_writemap { + // programmer error if this happens: in normal use passing the option on Windows is an error in main + panic!("Windows doesn't support the MDB_WRITEMAP LMDB option"); + } + let task_db_size = clamp_to_page_size(options.task_db_size); let budget = if options.indexer_config.skip_index_budget { IndexBudget { @@ -396,25 +403,37 @@ impl IndexScheduler { .open(options.tasks_path)?; let file_store = FileStore::new(&options.update_file_path)?; + let mut wtxn = env.write_txn()?; + let all_tasks = env.create_database(&mut wtxn, Some(db_name::ALL_TASKS))?; + let status = env.create_database(&mut wtxn, Some(db_name::STATUS))?; + let kind = env.create_database(&mut wtxn, Some(db_name::KIND))?; + let index_tasks = env.create_database(&mut wtxn, Some(db_name::INDEX_TASKS))?; + let canceled_by = env.create_database(&mut wtxn, Some(db_name::CANCELED_BY))?; + let enqueued_at = env.create_database(&mut wtxn, Some(db_name::ENQUEUED_AT))?; + let started_at = env.create_database(&mut wtxn, Some(db_name::STARTED_AT))?; + let finished_at = env.create_database(&mut wtxn, Some(db_name::FINISHED_AT))?; + wtxn.commit()?; + // allow unreachable_code to get rids of the warning in the case of a test build. let this = Self { must_stop_processing: MustStopProcessing::default(), processing_tasks: Arc::new(RwLock::new(ProcessingTasks::new())), file_store, - all_tasks: env.create_database(Some(db_name::ALL_TASKS))?, - status: env.create_database(Some(db_name::STATUS))?, - kind: env.create_database(Some(db_name::KIND))?, - index_tasks: env.create_database(Some(db_name::INDEX_TASKS))?, - canceled_by: env.create_database(Some(db_name::CANCELED_BY))?, - enqueued_at: env.create_database(Some(db_name::ENQUEUED_AT))?, - started_at: env.create_database(Some(db_name::STARTED_AT))?, - finished_at: env.create_database(Some(db_name::FINISHED_AT))?, + all_tasks, + status, + kind, + index_tasks, + canceled_by, + enqueued_at, + started_at, + finished_at, index_mapper: IndexMapper::new( &env, options.indexes_path, budget.map_size, options.index_growth_amount, budget.index_count, + options.enable_mdb_writemap, options.indexer_config, )?, env, @@ -1509,6 +1528,7 @@ mod tests { dumps_path: tempdir.path().join("dumps"), task_db_size: 1000 * 1000, // 1 MB, we don't use MiB on purpose. index_base_map_size: 1000 * 1000, // 1 MB, we don't use MiB on purpose. + enable_mdb_writemap: false, index_growth_amount: 1000 * 1000, // 1 MB index_count: 5, indexer_config, diff --git a/index-scheduler/src/utils.rs b/index-scheduler/src/utils.rs index 97f437bed..3971d9116 100644 --- a/index-scheduler/src/utils.rs +++ b/index-scheduler/src/utils.rs @@ -466,7 +466,7 @@ impl IndexScheduler { } } Details::DocumentDeletionByFilter { deleted_documents, original_filter: _ } => { - assert_eq!(kind.as_kind(), Kind::DocumentDeletionByFilter); + assert_eq!(kind.as_kind(), Kind::DocumentDeletion); let (index_uid, _) = if let KindWithContent::DocumentDeletionByFilter { ref index_uid, ref filter_expr, diff --git a/meilisearch-auth/src/store.rs b/meilisearch-auth/src/store.rs index eb93f5a46..e6e30d18d 100644 --- a/meilisearch-auth/src/store.rs +++ b/meilisearch-auth/src/store.rs @@ -55,9 +55,11 @@ impl HeedAuthStore { let path = path.as_ref().join(AUTH_DB_PATH); create_dir_all(&path)?; let env = Arc::new(open_auth_store_env(path.as_ref())?); - let keys = env.create_database(Some(KEY_DB_NAME))?; + let mut wtxn = env.write_txn()?; + let keys = env.create_database(&mut wtxn, Some(KEY_DB_NAME))?; let action_keyid_index_expiration = - env.create_database(Some(KEY_ID_ACTION_INDEX_EXPIRATION_DB_NAME))?; + env.create_database(&mut wtxn, Some(KEY_ID_ACTION_INDEX_EXPIRATION_DB_NAME))?; + wtxn.commit()?; Ok(Self { env, keys, action_keyid_index_expiration, should_close_on_drop: true }) } diff --git a/meilisearch-types/src/deserr/mod.rs b/meilisearch-types/src/deserr/mod.rs index 3e6ec8b96..bbaa42dc0 100644 --- a/meilisearch-types/src/deserr/mod.rs +++ b/meilisearch-types/src/deserr/mod.rs @@ -150,6 +150,7 @@ make_missing_field_convenience_builder!(MissingApiKeyActions, missing_api_key_ac make_missing_field_convenience_builder!(MissingApiKeyExpiresAt, missing_api_key_expires_at); make_missing_field_convenience_builder!(MissingApiKeyIndexes, missing_api_key_indexes); make_missing_field_convenience_builder!(MissingSwapIndexes, missing_swap_indexes); +make_missing_field_convenience_builder!(MissingDocumentFilter, missing_document_filter); // Integrate a sub-error into a [`DeserrError`] by taking its error message but using // the default error code (C) from `Self` diff --git a/meilisearch-types/src/error.rs b/meilisearch-types/src/error.rs index bcd8320c9..1509847b7 100644 --- a/meilisearch-types/src/error.rs +++ b/meilisearch-types/src/error.rs @@ -214,12 +214,12 @@ InvalidApiKeyUid , InvalidRequest , BAD_REQUEST ; InvalidContentType , InvalidRequest , UNSUPPORTED_MEDIA_TYPE ; InvalidDocumentCsvDelimiter , InvalidRequest , BAD_REQUEST ; InvalidDocumentFields , InvalidRequest , BAD_REQUEST ; +MissingDocumentFilter , InvalidRequest , BAD_REQUEST ; InvalidDocumentFilter , InvalidRequest , BAD_REQUEST ; InvalidDocumentGeoField , InvalidRequest , BAD_REQUEST ; InvalidDocumentId , InvalidRequest , BAD_REQUEST ; InvalidDocumentLimit , InvalidRequest , BAD_REQUEST ; InvalidDocumentOffset , InvalidRequest , BAD_REQUEST ; -InvalidDocumentDeleteFilter , InvalidRequest , BAD_REQUEST ; InvalidIndexLimit , InvalidRequest , BAD_REQUEST ; InvalidIndexOffset , InvalidRequest , BAD_REQUEST ; InvalidIndexPrimaryKey , InvalidRequest , BAD_REQUEST ; diff --git a/meilisearch-types/src/tasks.rs b/meilisearch-types/src/tasks.rs index e746a53b8..693ee4242 100644 --- a/meilisearch-types/src/tasks.rs +++ b/meilisearch-types/src/tasks.rs @@ -395,7 +395,6 @@ impl std::error::Error for ParseTaskStatusError {} pub enum Kind { DocumentAdditionOrUpdate, DocumentDeletion, - DocumentDeletionByFilter, SettingsUpdate, IndexCreation, IndexDeletion, @@ -412,7 +411,6 @@ impl Kind { match self { Kind::DocumentAdditionOrUpdate | Kind::DocumentDeletion - | Kind::DocumentDeletionByFilter | Kind::SettingsUpdate | Kind::IndexCreation | Kind::IndexDeletion @@ -430,7 +428,6 @@ impl Display for Kind { match self { Kind::DocumentAdditionOrUpdate => write!(f, "documentAdditionOrUpdate"), Kind::DocumentDeletion => write!(f, "documentDeletion"), - Kind::DocumentDeletionByFilter => write!(f, "documentDeletionByFilter"), Kind::SettingsUpdate => write!(f, "settingsUpdate"), Kind::IndexCreation => write!(f, "indexCreation"), Kind::IndexDeletion => write!(f, "indexDeletion"), diff --git a/meilisearch/src/analytics/mock_analytics.rs b/meilisearch/src/analytics/mock_analytics.rs index 03aed0189..68c3a7dff 100644 --- a/meilisearch/src/analytics/mock_analytics.rs +++ b/meilisearch/src/analytics/mock_analytics.rs @@ -5,7 +5,7 @@ use actix_web::HttpRequest; use meilisearch_types::InstanceUid; use serde_json::Value; -use super::{find_user_id, Analytics, DocumentDeletionKind}; +use super::{find_user_id, Analytics, DocumentDeletionKind, DocumentFetchKind}; use crate::routes::indexes::documents::UpdateDocumentsQuery; use crate::routes::tasks::TasksFilterQuery; use crate::Opt; @@ -71,6 +71,8 @@ impl Analytics for MockAnalytics { _request: &HttpRequest, ) { } + fn get_fetch_documents(&self, _documents_query: &DocumentFetchKind, _request: &HttpRequest) {} + fn post_fetch_documents(&self, _documents_query: &DocumentFetchKind, _request: &HttpRequest) {} fn get_tasks(&self, _query: &TasksFilterQuery, _request: &HttpRequest) {} fn health_seen(&self, _request: &HttpRequest) {} } diff --git a/meilisearch/src/analytics/mod.rs b/meilisearch/src/analytics/mod.rs index 6223b9db7..c48564dff 100644 --- a/meilisearch/src/analytics/mod.rs +++ b/meilisearch/src/analytics/mod.rs @@ -67,6 +67,12 @@ pub enum DocumentDeletionKind { PerFilter, } +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum DocumentFetchKind { + PerDocumentId, + Normal { with_filter: bool, limit: usize, offset: usize }, +} + pub trait Analytics: Sync + Send { fn instance_uid(&self) -> Option<&InstanceUid>; @@ -90,6 +96,12 @@ pub trait Analytics: Sync + Send { request: &HttpRequest, ); + // this method should be called to aggregate a fetch documents request + fn get_fetch_documents(&self, documents_query: &DocumentFetchKind, request: &HttpRequest); + + // this method should be called to aggregate a fetch documents request + fn post_fetch_documents(&self, documents_query: &DocumentFetchKind, request: &HttpRequest); + // this method should be called to aggregate a add documents request fn delete_documents(&self, kind: DocumentDeletionKind, request: &HttpRequest); diff --git a/meilisearch/src/analytics/segment_analytics.rs b/meilisearch/src/analytics/segment_analytics.rs index 3e40c09e8..afef95ed7 100644 --- a/meilisearch/src/analytics/segment_analytics.rs +++ b/meilisearch/src/analytics/segment_analytics.rs @@ -23,7 +23,9 @@ use tokio::select; use tokio::sync::mpsc::{self, Receiver, Sender}; use uuid::Uuid; -use super::{config_user_id_path, DocumentDeletionKind, MEILISEARCH_CONFIG_PATH}; +use super::{ + config_user_id_path, DocumentDeletionKind, DocumentFetchKind, MEILISEARCH_CONFIG_PATH, +}; use crate::analytics::Analytics; use crate::option::{default_http_addr, IndexerOpts, MaxMemory, MaxThreads, ScheduleSnapshot}; use crate::routes::indexes::documents::UpdateDocumentsQuery; @@ -72,6 +74,8 @@ pub enum AnalyticsMsg { AggregateAddDocuments(DocumentsAggregator), AggregateDeleteDocuments(DocumentsDeletionAggregator), AggregateUpdateDocuments(DocumentsAggregator), + AggregateGetFetchDocuments(DocumentsFetchAggregator), + AggregatePostFetchDocuments(DocumentsFetchAggregator), AggregateTasks(TasksAggregator), AggregateHealth(HealthAggregator), } @@ -139,6 +143,8 @@ impl SegmentAnalytics { add_documents_aggregator: DocumentsAggregator::default(), delete_documents_aggregator: DocumentsDeletionAggregator::default(), update_documents_aggregator: DocumentsAggregator::default(), + get_fetch_documents_aggregator: DocumentsFetchAggregator::default(), + post_fetch_documents_aggregator: DocumentsFetchAggregator::default(), get_tasks_aggregator: TasksAggregator::default(), health_aggregator: HealthAggregator::default(), }); @@ -205,6 +211,16 @@ impl super::Analytics for SegmentAnalytics { let _ = self.sender.try_send(AnalyticsMsg::AggregateUpdateDocuments(aggregate)); } + fn get_fetch_documents(&self, documents_query: &DocumentFetchKind, request: &HttpRequest) { + let aggregate = DocumentsFetchAggregator::from_query(documents_query, request); + let _ = self.sender.try_send(AnalyticsMsg::AggregateGetFetchDocuments(aggregate)); + } + + fn post_fetch_documents(&self, documents_query: &DocumentFetchKind, request: &HttpRequest) { + let aggregate = DocumentsFetchAggregator::from_query(documents_query, request); + let _ = self.sender.try_send(AnalyticsMsg::AggregatePostFetchDocuments(aggregate)); + } + fn get_tasks(&self, query: &TasksFilterQuery, request: &HttpRequest) { let aggregate = TasksAggregator::from_query(query, request); let _ = self.sender.try_send(AnalyticsMsg::AggregateTasks(aggregate)); @@ -225,6 +241,7 @@ impl super::Analytics for SegmentAnalytics { struct Infos { env: String, experimental_enable_metrics: bool, + experimental_reduce_indexing_memory_usage: bool, db_path: bool, import_dump: bool, dump_dir: bool, @@ -258,6 +275,7 @@ impl From for Infos { let Opt { db_path, experimental_enable_metrics, + experimental_reduce_indexing_memory_usage, http_addr, master_key: _, env, @@ -300,6 +318,7 @@ impl From for Infos { Self { env, experimental_enable_metrics, + experimental_reduce_indexing_memory_usage, db_path: db_path != PathBuf::from("./data.ms"), import_dump: import_dump.is_some(), dump_dir: dump_dir != PathBuf::from("dumps/"), @@ -338,6 +357,8 @@ pub struct Segment { add_documents_aggregator: DocumentsAggregator, delete_documents_aggregator: DocumentsDeletionAggregator, update_documents_aggregator: DocumentsAggregator, + get_fetch_documents_aggregator: DocumentsFetchAggregator, + post_fetch_documents_aggregator: DocumentsFetchAggregator, get_tasks_aggregator: TasksAggregator, health_aggregator: HealthAggregator, } @@ -400,6 +421,8 @@ impl Segment { Some(AnalyticsMsg::AggregateAddDocuments(agreg)) => self.add_documents_aggregator.aggregate(agreg), Some(AnalyticsMsg::AggregateDeleteDocuments(agreg)) => self.delete_documents_aggregator.aggregate(agreg), Some(AnalyticsMsg::AggregateUpdateDocuments(agreg)) => self.update_documents_aggregator.aggregate(agreg), + Some(AnalyticsMsg::AggregateGetFetchDocuments(agreg)) => self.get_fetch_documents_aggregator.aggregate(agreg), + Some(AnalyticsMsg::AggregatePostFetchDocuments(agreg)) => self.post_fetch_documents_aggregator.aggregate(agreg), Some(AnalyticsMsg::AggregateTasks(agreg)) => self.get_tasks_aggregator.aggregate(agreg), Some(AnalyticsMsg::AggregateHealth(agreg)) => self.health_aggregator.aggregate(agreg), None => (), @@ -450,6 +473,10 @@ impl Segment { .into_event(&self.user, "Documents Deleted"); let update_documents = std::mem::take(&mut self.update_documents_aggregator) .into_event(&self.user, "Documents Updated"); + let get_fetch_documents = std::mem::take(&mut self.get_fetch_documents_aggregator) + .into_event(&self.user, "Documents Fetched GET"); + let post_fetch_documents = std::mem::take(&mut self.post_fetch_documents_aggregator) + .into_event(&self.user, "Documents Fetched POST"); let get_tasks = std::mem::take(&mut self.get_tasks_aggregator).into_event(&self.user, "Tasks Seen"); let health = @@ -473,6 +500,12 @@ impl Segment { if let Some(update_documents) = update_documents { let _ = self.batcher.push(update_documents).await; } + if let Some(get_fetch_documents) = get_fetch_documents { + let _ = self.batcher.push(get_fetch_documents).await; + } + if let Some(post_fetch_documents) = post_fetch_documents { + let _ = self.batcher.push(post_fetch_documents).await; + } if let Some(get_tasks) = get_tasks { let _ = self.batcher.push(get_tasks).await; } @@ -1135,3 +1168,76 @@ impl HealthAggregator { }) } } + +#[derive(Default, Serialize)] +pub struct DocumentsFetchAggregator { + #[serde(skip)] + timestamp: Option, + + // context + #[serde(rename = "user-agent")] + user_agents: HashSet, + + #[serde(rename = "requests.max_limit")] + total_received: usize, + + // a call on ../documents/:doc_id + per_document_id: bool, + // if a filter was used + per_filter: bool, + + // pagination + #[serde(rename = "pagination.max_limit")] + max_limit: usize, + #[serde(rename = "pagination.max_offset")] + max_offset: usize, +} + +impl DocumentsFetchAggregator { + pub fn from_query(query: &DocumentFetchKind, request: &HttpRequest) -> Self { + let (limit, offset) = match query { + DocumentFetchKind::PerDocumentId => (1, 0), + DocumentFetchKind::Normal { limit, offset, .. } => (*limit, *offset), + }; + Self { + timestamp: Some(OffsetDateTime::now_utc()), + user_agents: extract_user_agents(request).into_iter().collect(), + total_received: 1, + per_document_id: matches!(query, DocumentFetchKind::PerDocumentId), + per_filter: matches!(query, DocumentFetchKind::Normal { with_filter, .. } if *with_filter), + max_limit: limit, + max_offset: offset, + } + } + + /// Aggregate one [DocumentsFetchAggregator] into another. + pub fn aggregate(&mut self, other: Self) { + if self.timestamp.is_none() { + self.timestamp = other.timestamp; + } + for user_agent in other.user_agents { + self.user_agents.insert(user_agent); + } + + self.total_received = self.total_received.saturating_add(other.total_received); + self.per_document_id |= other.per_document_id; + self.per_filter |= other.per_filter; + + self.max_limit = self.max_limit.max(other.max_limit); + self.max_offset = self.max_offset.max(other.max_offset); + } + + pub fn into_event(self, user: &User, event_name: &str) -> Option { + // if we had no timestamp it means we never encountered any events and + // thus we don't need to send this event. + let timestamp = self.timestamp?; + + Some(Track { + timestamp: Some(timestamp), + user: user.clone(), + event: event_name.to_string(), + properties: serde_json::to_value(self).ok()?, + ..Default::default() + }) + } +} diff --git a/meilisearch/src/error.rs b/meilisearch/src/error.rs index 9d0a03c34..ca10c4593 100644 --- a/meilisearch/src/error.rs +++ b/meilisearch/src/error.rs @@ -1,5 +1,6 @@ use actix_web as aweb; use aweb::error::{JsonPayloadError, QueryPayloadError}; +use byte_unit::Byte; use meilisearch_types::document_formats::{DocumentFormatError, PayloadType}; use meilisearch_types::error::{Code, ErrorCode, ResponseError}; use meilisearch_types::index_uid::{IndexUid, IndexUidFormatError}; @@ -26,8 +27,8 @@ pub enum MeilisearchHttpError { InvalidExpression(&'static [&'static str], Value), #[error("A {0} payload is missing.")] MissingPayload(PayloadType), - #[error("The provided payload reached the size limit.")] - PayloadTooLarge, + #[error("The provided payload reached the size limit. The maximum accepted payload size is {}.", Byte::from_bytes(*.0 as u64).get_appropriate_unit(true))] + PayloadTooLarge(usize), #[error("Two indexes must be given for each swap. The list `[{}]` contains {} indexes.", .0.iter().map(|uid| format!("\"{uid}\"")).collect::>().join(", "), .0.len() )] @@ -60,9 +61,9 @@ impl ErrorCode for MeilisearchHttpError { MeilisearchHttpError::MissingPayload(_) => Code::MissingPayload, MeilisearchHttpError::InvalidContentType(_, _) => Code::InvalidContentType, MeilisearchHttpError::DocumentNotFound(_) => Code::DocumentNotFound, - MeilisearchHttpError::EmptyFilter => Code::InvalidDocumentDeleteFilter, + MeilisearchHttpError::EmptyFilter => Code::InvalidDocumentFilter, MeilisearchHttpError::InvalidExpression(_, _) => Code::InvalidSearchFilter, - MeilisearchHttpError::PayloadTooLarge => Code::PayloadTooLarge, + MeilisearchHttpError::PayloadTooLarge(_) => Code::PayloadTooLarge, MeilisearchHttpError::SwapIndexPayloadWrongLength(_) => Code::InvalidSwapIndexes, MeilisearchHttpError::IndexUid(e) => e.error_code(), MeilisearchHttpError::SerdeJson(_) => Code::Internal, diff --git a/meilisearch/src/extractors/payload.rs b/meilisearch/src/extractors/payload.rs index 0ccebe8f9..5e7e9cf7e 100644 --- a/meilisearch/src/extractors/payload.rs +++ b/meilisearch/src/extractors/payload.rs @@ -11,6 +11,7 @@ use crate::error::MeilisearchHttpError; pub struct Payload { payload: Decompress, limit: usize, + remaining: usize, } pub struct PayloadConfig { @@ -43,6 +44,7 @@ impl FromRequest for Payload { ready(Ok(Payload { payload: Decompress::from_headers(payload.take(), req.headers()), limit, + remaining: limit, })) } } @@ -54,12 +56,14 @@ impl Stream for Payload { fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { match Pin::new(&mut self.payload).poll_next(cx) { Poll::Ready(Some(result)) => match result { - Ok(bytes) => match self.limit.checked_sub(bytes.len()) { + Ok(bytes) => match self.remaining.checked_sub(bytes.len()) { Some(new_limit) => { - self.limit = new_limit; + self.remaining = new_limit; Poll::Ready(Some(Ok(bytes))) } - None => Poll::Ready(Some(Err(MeilisearchHttpError::PayloadTooLarge))), + None => { + Poll::Ready(Some(Err(MeilisearchHttpError::PayloadTooLarge(self.limit)))) + } }, x => Poll::Ready(Some(x.map_err(MeilisearchHttpError::from))), }, diff --git a/meilisearch/src/lib.rs b/meilisearch/src/lib.rs index 67d8bbd5c..bee53f6f8 100644 --- a/meilisearch/src/lib.rs +++ b/meilisearch/src/lib.rs @@ -232,6 +232,7 @@ fn open_or_create_database_unchecked( dumps_path: opt.dump_dir.clone(), task_db_size: opt.max_task_db_size.get_bytes() as usize, index_base_map_size: opt.max_index_size.get_bytes() as usize, + enable_mdb_writemap: opt.experimental_reduce_indexing_memory_usage, indexer_config: (&opt.indexer_options).try_into()?, autobatching_enabled: true, max_number_of_tasks: 1_000_000, diff --git a/meilisearch/src/main.rs b/meilisearch/src/main.rs index 2ab37488c..1b5e918dc 100644 --- a/meilisearch/src/main.rs +++ b/meilisearch/src/main.rs @@ -29,6 +29,11 @@ fn setup(opt: &Opt) -> anyhow::Result<()> { async fn main() -> anyhow::Result<()> { let (opt, config_read_from) = Opt::try_build()?; + anyhow::ensure!( + !(cfg!(windows) && opt.experimental_reduce_indexing_memory_usage), + "The `experimental-reduce-indexing-memory-usage` flag is not supported on Windows" + ); + setup(&opt)?; match (opt.env.as_ref(), &opt.master_key) { diff --git a/meilisearch/src/option.rs b/meilisearch/src/option.rs index 8e6ca9006..0511b5033 100644 --- a/meilisearch/src/option.rs +++ b/meilisearch/src/option.rs @@ -48,6 +48,8 @@ const MEILI_IGNORE_DUMP_IF_DB_EXISTS: &str = "MEILI_IGNORE_DUMP_IF_DB_EXISTS"; const MEILI_DUMP_DIR: &str = "MEILI_DUMP_DIR"; const MEILI_LOG_LEVEL: &str = "MEILI_LOG_LEVEL"; const MEILI_EXPERIMENTAL_ENABLE_METRICS: &str = "MEILI_EXPERIMENTAL_ENABLE_METRICS"; +const MEILI_EXPERIMENTAL_REDUCE_INDEXING_MEMORY_USAGE: &str = + "MEILI_EXPERIMENTAL_REDUCE_INDEXING_MEMORY_USAGE"; const DEFAULT_CONFIG_FILE_PATH: &str = "./config.toml"; const DEFAULT_DB_PATH: &str = "./data.ms"; @@ -293,6 +295,11 @@ pub struct Opt { #[serde(default)] pub experimental_enable_metrics: bool, + /// Experimental RAM reduction during indexing, do not use in production, see: + #[clap(long, env = MEILI_EXPERIMENTAL_REDUCE_INDEXING_MEMORY_USAGE)] + #[serde(default)] + pub experimental_reduce_indexing_memory_usage: bool, + #[serde(flatten)] #[clap(flatten)] pub indexer_options: IndexerOpts, @@ -385,6 +392,7 @@ impl Opt { #[cfg(all(not(debug_assertions), feature = "analytics"))] no_analytics, experimental_enable_metrics: enable_metrics_route, + experimental_reduce_indexing_memory_usage: reduce_indexing_memory_usage, } = self; export_to_env_if_not_present(MEILI_DB_PATH, db_path); export_to_env_if_not_present(MEILI_HTTP_ADDR, http_addr); @@ -426,6 +434,10 @@ impl Opt { MEILI_EXPERIMENTAL_ENABLE_METRICS, enable_metrics_route.to_string(), ); + export_to_env_if_not_present( + MEILI_EXPERIMENTAL_REDUCE_INDEXING_MEMORY_USAGE, + reduce_indexing_memory_usage.to_string(), + ); indexer_options.export_to_env(); } diff --git a/meilisearch/src/routes/indexes/documents.rs b/meilisearch/src/routes/indexes/documents.rs index eb0f5a59e..2afc1b5fb 100644 --- a/meilisearch/src/routes/indexes/documents.rs +++ b/meilisearch/src/routes/indexes/documents.rs @@ -29,7 +29,7 @@ use tempfile::tempfile; use tokio::fs::File; use tokio::io::{AsyncSeekExt, AsyncWriteExt, BufWriter}; -use crate::analytics::{Analytics, DocumentDeletionKind}; +use crate::analytics::{Analytics, DocumentDeletionKind, DocumentFetchKind}; use crate::error::MeilisearchHttpError; use crate::error::PayloadError::ReceivePayload; use crate::extractors::authentication::policies::*; @@ -97,10 +97,14 @@ pub async fn get_document( index_scheduler: GuardedData, Data>, document_param: web::Path, params: AwebQueryParameter, + req: HttpRequest, + analytics: web::Data, ) -> Result { let DocumentParam { index_uid, document_id } = document_param.into_inner(); let index_uid = IndexUid::try_from(index_uid)?; + analytics.get_fetch_documents(&DocumentFetchKind::PerDocumentId, &req); + let GetDocument { fields } = params.into_inner(); let attributes_to_retrieve = fields.merge_star_and_none(); @@ -161,16 +165,31 @@ pub async fn documents_by_query_post( index_scheduler: GuardedData, Data>, index_uid: web::Path, body: AwebJson, + req: HttpRequest, + analytics: web::Data, ) -> Result { debug!("called with body: {:?}", body); - documents_by_query(&index_scheduler, index_uid, body.into_inner()) + let body = body.into_inner(); + + analytics.post_fetch_documents( + &DocumentFetchKind::Normal { + with_filter: body.filter.is_some(), + limit: body.limit, + offset: body.offset, + }, + &req, + ); + + documents_by_query(&index_scheduler, index_uid, body) } pub async fn get_documents( index_scheduler: GuardedData, Data>, index_uid: web::Path, params: AwebQueryParameter, + req: HttpRequest, + analytics: web::Data, ) -> Result { debug!("called with params: {:?}", params); @@ -191,6 +210,15 @@ pub async fn get_documents( filter, }; + analytics.get_fetch_documents( + &DocumentFetchKind::Normal { + with_filter: query.filter.is_some(), + limit: query.limit, + offset: query.offset, + }, + &req, + ); + documents_by_query(&index_scheduler, index_uid, query) } @@ -458,7 +486,7 @@ pub async fn delete_documents_batch( #[derive(Debug, Deserr)] #[deserr(error = DeserrJsonError, rename_all = camelCase, deny_unknown_fields)] pub struct DocumentDeletionByFilter { - #[deserr(error = DeserrJsonError)] + #[deserr(error = DeserrJsonError, missing_field_error = DeserrJsonError::missing_document_filter)] filter: Value, } @@ -480,8 +508,8 @@ pub async fn delete_documents_by_filter( || -> Result<_, ResponseError> { Ok(crate::search::parse_filter(&filter)?.ok_or(MeilisearchHttpError::EmptyFilter)?) }() - // and whatever was the error, the error code should always be an InvalidDocumentDeleteFilter - .map_err(|err| ResponseError::from_msg(err.message, Code::InvalidDocumentDeleteFilter))?; + // and whatever was the error, the error code should always be an InvalidDocumentFilter + .map_err(|err| ResponseError::from_msg(err.message, Code::InvalidDocumentFilter))?; let task = KindWithContent::DocumentDeletionByFilter { index_uid, filter_expr: filter }; let task: SummarizedTaskView = @@ -540,7 +568,12 @@ fn retrieve_documents>( }; let candidates = if let Some(filter) = filter { - filter.evaluate(&rtxn, index)? + filter.evaluate(&rtxn, index).map_err(|err| match err { + milli::Error::UserError(milli::UserError::InvalidFilter(_)) => { + ResponseError::from_msg(err.to_string(), Code::InvalidDocumentFilter) + } + e => e.into(), + })? } else { index.documents_ids(&rtxn)? }; diff --git a/meilisearch/src/routes/tasks.rs b/meilisearch/src/routes/tasks.rs index cab0f7197..2713d0988 100644 --- a/meilisearch/src/routes/tasks.rs +++ b/meilisearch/src/routes/tasks.rs @@ -99,7 +99,7 @@ pub struct DetailsView { #[serde(skip_serializing_if = "Option::is_none")] pub deleted_tasks: Option>, #[serde(skip_serializing_if = "Option::is_none")] - pub original_filter: Option, + pub original_filter: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub dump_uid: Option>, #[serde(skip_serializing_if = "Option::is_none")] @@ -131,12 +131,13 @@ impl From
for DetailsView { } => DetailsView { provided_ids: Some(received_document_ids), deleted_documents: Some(deleted_documents), + original_filter: Some(None), ..DetailsView::default() }, Details::DocumentDeletionByFilter { original_filter, deleted_documents } => { DetailsView { provided_ids: Some(0), - original_filter: Some(original_filter), + original_filter: Some(Some(original_filter)), deleted_documents: Some(deleted_documents), ..DetailsView::default() } @@ -148,7 +149,7 @@ impl From
for DetailsView { DetailsView { matched_tasks: Some(matched_tasks), canceled_tasks: Some(canceled_tasks), - original_filter: Some(original_filter), + original_filter: Some(Some(original_filter)), ..DetailsView::default() } } @@ -156,7 +157,7 @@ impl From
for DetailsView { DetailsView { matched_tasks: Some(matched_tasks), deleted_tasks: Some(deleted_tasks), - original_filter: Some(original_filter), + original_filter: Some(Some(original_filter)), ..DetailsView::default() } } @@ -729,7 +730,7 @@ mod tests { let err = deserr_query_params::(params).unwrap_err(); snapshot!(meili_snap::json_string!(err), @r###" { - "message": "Invalid value in parameter `types`: `createIndex` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `documentDeletionByFilter`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", + "message": "Invalid value in parameter `types`: `createIndex` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", "code": "invalid_task_types", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_task_types" diff --git a/meilisearch/tests/auth/authorization.rs b/meilisearch/tests/auth/authorization.rs index ef4a7eaa1..58fba4481 100644 --- a/meilisearch/tests/auth/authorization.rs +++ b/meilisearch/tests/auth/authorization.rs @@ -16,8 +16,11 @@ pub static AUTHORIZATIONS: Lazy hashset!{"search", "*"}, ("POST", "/indexes/products/documents") => hashset!{"documents.add", "documents.*", "*"}, ("GET", "/indexes/products/documents") => hashset!{"documents.get", "documents.*", "*"}, + ("POST", "/indexes/products/documents/fetch") => hashset!{"documents.get", "documents.*", "*"}, ("GET", "/indexes/products/documents/0") => hashset!{"documents.get", "documents.*", "*"}, ("DELETE", "/indexes/products/documents/0") => hashset!{"documents.delete", "documents.*", "*"}, + ("POST", "/indexes/products/documents/delete-batch") => hashset!{"documents.delete", "documents.*", "*"}, + ("POST", "/indexes/products/documents/delete") => hashset!{"documents.delete", "documents.*", "*"}, ("GET", "/tasks") => hashset!{"tasks.get", "tasks.*", "*"}, ("DELETE", "/tasks") => hashset!{"tasks.delete", "tasks.*", "*"}, ("GET", "/tasks?indexUid=products") => hashset!{"tasks.get", "tasks.*", "*"}, diff --git a/meilisearch/tests/documents/add_documents.rs b/meilisearch/tests/documents/add_documents.rs index c2ba2ccaa..11a5e0fc8 100644 --- a/meilisearch/tests/documents/add_documents.rs +++ b/meilisearch/tests/documents/add_documents.rs @@ -1781,7 +1781,7 @@ async fn error_add_documents_payload_size() { snapshot!(json_string!(response, { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }), @r###" { - "message": "The provided payload reached the size limit.", + "message": "The provided payload reached the size limit. The maximum accepted payload size is 10.00 MiB.", "code": "payload_too_large", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#payload_too_large" diff --git a/meilisearch/tests/documents/errors.rs b/meilisearch/tests/documents/errors.rs index b72dc40f3..7dab16a25 100644 --- a/meilisearch/tests/documents/errors.rs +++ b/meilisearch/tests/documents/errors.rs @@ -180,9 +180,9 @@ async fn get_all_documents_bad_filter() { snapshot!(json_string!(response), @r###" { "message": "Attribute `doggo` is not filterable. This index does not have configured filterable attributes.\n1:6 doggo=bernese", - "code": "invalid_search_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" } "###); } @@ -547,9 +547,9 @@ async fn delete_document_by_filter() { snapshot!(json_string!(response), @r###" { "message": "Invalid syntax for the filter parameter: `expected String, Array, found: true`.", - "code": "invalid_document_delete_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_document_delete_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" } "###); @@ -559,9 +559,9 @@ async fn delete_document_by_filter() { snapshot!(json_string!(response), @r###" { "message": "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `IN`, `NOT IN`, `TO`, `EXISTS`, `NOT EXISTS`, `IS NULL`, `IS NOT NULL`, `IS EMPTY`, `IS NOT EMPTY`, `_geoRadius`, or `_geoBoundingBox` at `hello`.\n1:6 hello", - "code": "invalid_document_delete_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_document_delete_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" } "###); @@ -571,9 +571,21 @@ async fn delete_document_by_filter() { snapshot!(json_string!(response), @r###" { "message": "Sending an empty filter is forbidden.", - "code": "invalid_document_delete_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_document_delete_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" + } + "###); + + // do not send any filter + let (response, code) = index.delete_document_by_filter(json!({})).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Missing field `filter`", + "code": "missing_document_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#missing_document_filter" } "###); @@ -630,9 +642,9 @@ async fn delete_document_by_filter() { }, "error": { "message": "Attribute `doggo` is not filterable. This index does not have configured filterable attributes.\n1:6 doggo = bernese", - "code": "invalid_search_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" }, "duration": "[duration]", "enqueuedAt": "[date]", @@ -664,9 +676,9 @@ async fn delete_document_by_filter() { }, "error": { "message": "Attribute `catto` is not filterable. Available filterable attributes are: `doggo`.\n1:6 catto = jorts", - "code": "invalid_search_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" }, "duration": "[duration]", "enqueuedAt": "[date]", @@ -748,4 +760,27 @@ async fn fetch_document_by_filter() { "link": "https://docs.meilisearch.com/errors#invalid_document_filter" } "###); + + let (response, code) = index.get_document_by_filter(json!({ "filter": "cool doggo" })).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `IN`, `NOT IN`, `TO`, `EXISTS`, `NOT EXISTS`, `IS NULL`, `IS NOT NULL`, `IS EMPTY`, `IS NOT EMPTY`, `_geoRadius`, or `_geoBoundingBox` at `cool doggo`.\n1:11 cool doggo", + "code": "invalid_document_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" + } + "###); + + let (response, code) = + index.get_document_by_filter(json!({ "filter": "doggo = bernese" })).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Attribute `doggo` is not filterable. Available filterable attributes are: `color`.\n1:6 doggo = bernese", + "code": "invalid_document_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" + } + "###); } diff --git a/meilisearch/tests/search/errors.rs b/meilisearch/tests/search/errors.rs index a9a2969bb..f314e8800 100644 --- a/meilisearch/tests/search/errors.rs +++ b/meilisearch/tests/search/errors.rs @@ -946,7 +946,7 @@ async fn sort_unset_ranking_rule() { index.wait_task(1).await; let expected_response = json!({ - "message": "The sort ranking rule must be specified in the ranking rules settings to use the sort parameter at search time.", + "message": "You must specify where `sort` is listed in the rankingRules setting to use the sort parameter at search time.", "code": "invalid_search_sort", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_search_sort" diff --git a/meilisearch/tests/tasks/errors.rs b/meilisearch/tests/tasks/errors.rs index 065ff1aa9..830c4c8e7 100644 --- a/meilisearch/tests/tasks/errors.rs +++ b/meilisearch/tests/tasks/errors.rs @@ -97,7 +97,7 @@ async fn task_bad_types() { snapshot!(code, @"400 Bad Request"); snapshot!(json_string!(response), @r###" { - "message": "Invalid value in parameter `types`: `doggo` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `documentDeletionByFilter`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", + "message": "Invalid value in parameter `types`: `doggo` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", "code": "invalid_task_types", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_task_types" @@ -108,7 +108,7 @@ async fn task_bad_types() { snapshot!(code, @"400 Bad Request"); snapshot!(json_string!(response), @r###" { - "message": "Invalid value in parameter `types`: `doggo` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `documentDeletionByFilter`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", + "message": "Invalid value in parameter `types`: `doggo` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", "code": "invalid_task_types", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_task_types" @@ -119,7 +119,7 @@ async fn task_bad_types() { snapshot!(code, @"400 Bad Request"); snapshot!(json_string!(response), @r###" { - "message": "Invalid value in parameter `types`: `doggo` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `documentDeletionByFilter`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", + "message": "Invalid value in parameter `types`: `doggo` is not a valid task type. Available types are `documentAdditionOrUpdate`, `documentDeletion`, `settingsUpdate`, `indexCreation`, `indexDeletion`, `indexUpdate`, `indexSwap`, `taskCancelation`, `taskDeletion`, `dumpCreation`, `snapshotCreation`.", "code": "invalid_task_types", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_task_types" diff --git a/meilisearch/tests/tasks/mod.rs b/meilisearch/tests/tasks/mod.rs index e9b5a2325..4ac134871 100644 --- a/meilisearch/tests/tasks/mod.rs +++ b/meilisearch/tests/tasks/mod.rs @@ -413,7 +413,7 @@ async fn test_summarized_document_addition_or_update() { } #[actix_web::test] -async fn test_summarized_delete_batch() { +async fn test_summarized_delete_documents_by_batch() { let server = Server::new().await; let index = server.index("test"); index.delete_batch(vec![1, 2, 3]).await; @@ -430,7 +430,8 @@ async fn test_summarized_delete_batch() { "canceledBy": null, "details": { "providedIds": 3, - "deletedDocuments": 0 + "deletedDocuments": 0, + "originalFilter": null }, "error": { "message": "Index `test` not found.", @@ -460,7 +461,8 @@ async fn test_summarized_delete_batch() { "canceledBy": null, "details": { "providedIds": 1, - "deletedDocuments": 0 + "deletedDocuments": 0, + "originalFilter": null }, "error": null, "duration": "[duration]", @@ -472,7 +474,100 @@ async fn test_summarized_delete_batch() { } #[actix_web::test] -async fn test_summarized_delete_document() { +async fn test_summarized_delete_documents_by_filter() { + let server = Server::new().await; + let index = server.index("test"); + + index.delete_document_by_filter(json!({ "filter": "doggo = bernese" })).await; + index.wait_task(0).await; + let (task, _) = index.get_task(0).await; + assert_json_snapshot!(task, + { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }, + @r###" + { + "uid": 0, + "indexUid": "test", + "status": "failed", + "type": "documentDeletion", + "canceledBy": null, + "details": { + "providedIds": 0, + "deletedDocuments": 0, + "originalFilter": "\"doggo = bernese\"" + }, + "error": { + "message": "Index `test` not found.", + "code": "index_not_found", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#index_not_found" + }, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); + + index.create(None).await; + index.delete_document_by_filter(json!({ "filter": "doggo = bernese" })).await; + index.wait_task(2).await; + let (task, _) = index.get_task(2).await; + assert_json_snapshot!(task, + { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }, + @r###" + { + "uid": 2, + "indexUid": "test", + "status": "failed", + "type": "documentDeletion", + "canceledBy": null, + "details": { + "providedIds": 0, + "deletedDocuments": 0, + "originalFilter": "\"doggo = bernese\"" + }, + "error": { + "message": "Attribute `doggo` is not filterable. This index does not have configured filterable attributes.\n1:6 doggo = bernese", + "code": "invalid_document_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" + }, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); + + index.update_settings(json!({ "filterableAttributes": ["doggo"] })).await; + index.delete_document_by_filter(json!({ "filter": "doggo = bernese" })).await; + index.wait_task(4).await; + let (task, _) = index.get_task(4).await; + assert_json_snapshot!(task, + { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }, + @r###" + { + "uid": 4, + "indexUid": "test", + "status": "succeeded", + "type": "documentDeletion", + "canceledBy": null, + "details": { + "providedIds": 0, + "deletedDocuments": 0, + "originalFilter": "\"doggo = bernese\"" + }, + "error": null, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); +} + +#[actix_web::test] +async fn test_summarized_delete_document_by_id() { let server = Server::new().await; let index = server.index("test"); index.delete_document(1).await; @@ -489,7 +584,8 @@ async fn test_summarized_delete_document() { "canceledBy": null, "details": { "providedIds": 1, - "deletedDocuments": 0 + "deletedDocuments": 0, + "originalFilter": null }, "error": { "message": "Index `test` not found.", @@ -519,7 +615,8 @@ async fn test_summarized_delete_document() { "canceledBy": null, "details": { "providedIds": 1, - "deletedDocuments": 0 + "deletedDocuments": 0, + "originalFilter": null }, "error": null, "duration": "[duration]", diff --git a/milli/Cargo.toml b/milli/Cargo.toml index de0f4e31d..be4c88f23 100644 --- a/milli/Cargo.toml +++ b/milli/Cargo.toml @@ -25,8 +25,13 @@ flatten-serde-json = { path = "../flatten-serde-json" } fst = "0.4.7" fxhash = "0.2.1" geoutils = "0.5.1" -grenad = { version = "0.4.4", default-features = false, features = ["tempfile"] } -heed = { git = "https://github.com/meilisearch/heed", tag = "v0.12.5", default-features = false, features = ["lmdb", "sync-read-txn"] } +grenad = { version = "0.4.4", default-features = false, features = [ + "tempfile", +] } +heed = { git = "https://github.com/meilisearch/heed", tag = "v0.12.6", default-features = false, features = [ + "lmdb", + "sync-read-txn", +] } json-depth-checker = { path = "../json-depth-checker" } levenshtein_automata = { version = "0.2.1", features = ["fst_automaton"] } memmap2 = "0.5.10" @@ -39,12 +44,17 @@ rstar = { version = "0.10.0", features = ["serde"] } serde = { version = "1.0.160", features = ["derive"] } serde_json = { version = "1.0.95", features = ["preserve_order"] } slice-group-by = "0.3.0" -smallstr = { version = "0.3.0", features = ["serde"] } +smallstr = { version = "0.3.0", features = ["serde"] } smallvec = "1.10.0" smartstring = "1.0.1" tempfile = "3.5.0" thiserror = "1.0.40" -time = { version = "0.3.20", features = ["serde-well-known", "formatting", "parsing", "macros"] } +time = { version = "0.3.20", features = [ + "serde-well-known", + "formatting", + "parsing", + "macros", +] } uuid = { version = "1.3.1", features = ["v4"] } filter-parser = { path = "../filter-parser" } @@ -63,13 +73,13 @@ big_s = "1.0.2" insta = "1.29.0" maplit = "1.0.2" md5 = "0.7.0" -rand = {version = "0.8.5", features = ["small_rng"] } +rand = { version = "0.8.5", features = ["small_rng"] } [target.'cfg(fuzzing)'.dev-dependencies] fuzzcheck = "0.12.1" [features] -all-tokenizations = [ "charabia/default" ] +all-tokenizations = ["charabia/default"] # Use POSIX semaphores instead of SysV semaphores in LMDB # For more information on this feature, see heed's Cargo.toml diff --git a/milli/src/error.rs b/milli/src/error.rs index 7f0faf2fd..8d55eabbd 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -126,7 +126,7 @@ only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and undersco InvalidSortableAttribute { field: String, valid_fields: BTreeSet }, #[error("{}", HeedError::BadOpenOptions)] InvalidLmdbOpenOptions, - #[error("The sort ranking rule must be specified in the ranking rules settings to use the sort parameter at search time.")] + #[error("You must specify where `sort` is listed in the rankingRules setting to use the sort parameter at search time.")] SortRankingRuleMissing, #[error("The database file is in an invalid state.")] InvalidStoreFile, diff --git a/milli/src/index.rs b/milli/src/index.rs index ad53e79ea..9ea7b628c 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -170,33 +170,46 @@ impl Index { unsafe { options.flag(Flags::MdbAlwaysFreePages) }; let env = options.open(path)?; - let main = env.create_poly_database(Some(MAIN))?; - let word_docids = env.create_database(Some(WORD_DOCIDS))?; - let exact_word_docids = env.create_database(Some(EXACT_WORD_DOCIDS))?; - let word_prefix_docids = env.create_database(Some(WORD_PREFIX_DOCIDS))?; - let exact_word_prefix_docids = env.create_database(Some(EXACT_WORD_PREFIX_DOCIDS))?; - let docid_word_positions = env.create_database(Some(DOCID_WORD_POSITIONS))?; - let word_pair_proximity_docids = env.create_database(Some(WORD_PAIR_PROXIMITY_DOCIDS))?; - let script_language_docids = env.create_database(Some(SCRIPT_LANGUAGE_DOCIDS))?; + let mut wtxn = env.write_txn()?; + let main = env.create_poly_database(&mut wtxn, Some(MAIN))?; + let word_docids = env.create_database(&mut wtxn, Some(WORD_DOCIDS))?; + let exact_word_docids = env.create_database(&mut wtxn, Some(EXACT_WORD_DOCIDS))?; + let word_prefix_docids = env.create_database(&mut wtxn, Some(WORD_PREFIX_DOCIDS))?; + let exact_word_prefix_docids = + env.create_database(&mut wtxn, Some(EXACT_WORD_PREFIX_DOCIDS))?; + let docid_word_positions = env.create_database(&mut wtxn, Some(DOCID_WORD_POSITIONS))?; + let word_pair_proximity_docids = + env.create_database(&mut wtxn, Some(WORD_PAIR_PROXIMITY_DOCIDS))?; + let script_language_docids = + env.create_database(&mut wtxn, Some(SCRIPT_LANGUAGE_DOCIDS))?; let word_prefix_pair_proximity_docids = - env.create_database(Some(WORD_PREFIX_PAIR_PROXIMITY_DOCIDS))?; + env.create_database(&mut wtxn, Some(WORD_PREFIX_PAIR_PROXIMITY_DOCIDS))?; let prefix_word_pair_proximity_docids = - env.create_database(Some(PREFIX_WORD_PAIR_PROXIMITY_DOCIDS))?; - let word_position_docids = env.create_database(Some(WORD_POSITION_DOCIDS))?; - let word_fid_docids = env.create_database(Some(WORD_FIELD_ID_DOCIDS))?; - let field_id_word_count_docids = env.create_database(Some(FIELD_ID_WORD_COUNT_DOCIDS))?; - let word_prefix_position_docids = env.create_database(Some(WORD_PREFIX_POSITION_DOCIDS))?; - let word_prefix_fid_docids = env.create_database(Some(WORD_PREFIX_FIELD_ID_DOCIDS))?; - let facet_id_f64_docids = env.create_database(Some(FACET_ID_F64_DOCIDS))?; - let facet_id_string_docids = env.create_database(Some(FACET_ID_STRING_DOCIDS))?; - let facet_id_exists_docids = env.create_database(Some(FACET_ID_EXISTS_DOCIDS))?; - let facet_id_is_null_docids = env.create_database(Some(FACET_ID_IS_NULL_DOCIDS))?; - let facet_id_is_empty_docids = env.create_database(Some(FACET_ID_IS_EMPTY_DOCIDS))?; + env.create_database(&mut wtxn, Some(PREFIX_WORD_PAIR_PROXIMITY_DOCIDS))?; + let word_position_docids = env.create_database(&mut wtxn, Some(WORD_POSITION_DOCIDS))?; + let word_fid_docids = env.create_database(&mut wtxn, Some(WORD_FIELD_ID_DOCIDS))?; + let field_id_word_count_docids = + env.create_database(&mut wtxn, Some(FIELD_ID_WORD_COUNT_DOCIDS))?; + let word_prefix_position_docids = + env.create_database(&mut wtxn, Some(WORD_PREFIX_POSITION_DOCIDS))?; + let word_prefix_fid_docids = + env.create_database(&mut wtxn, Some(WORD_PREFIX_FIELD_ID_DOCIDS))?; + let facet_id_f64_docids = env.create_database(&mut wtxn, Some(FACET_ID_F64_DOCIDS))?; + let facet_id_string_docids = + env.create_database(&mut wtxn, Some(FACET_ID_STRING_DOCIDS))?; + let facet_id_exists_docids = + env.create_database(&mut wtxn, Some(FACET_ID_EXISTS_DOCIDS))?; + let facet_id_is_null_docids = + env.create_database(&mut wtxn, Some(FACET_ID_IS_NULL_DOCIDS))?; + let facet_id_is_empty_docids = + env.create_database(&mut wtxn, Some(FACET_ID_IS_EMPTY_DOCIDS))?; - let field_id_docid_facet_f64s = env.create_database(Some(FIELD_ID_DOCID_FACET_F64S))?; + let field_id_docid_facet_f64s = + env.create_database(&mut wtxn, Some(FIELD_ID_DOCID_FACET_F64S))?; let field_id_docid_facet_strings = - env.create_database(Some(FIELD_ID_DOCID_FACET_STRINGS))?; - let documents = env.create_database(Some(DOCUMENTS))?; + env.create_database(&mut wtxn, Some(FIELD_ID_DOCID_FACET_STRINGS))?; + let documents = env.create_database(&mut wtxn, Some(DOCUMENTS))?; + wtxn.commit()?; Index::set_creation_dates(&env, main, created_at, updated_at)?; diff --git a/milli/src/search/new/graph_based_ranking_rule.rs b/milli/src/search/new/graph_based_ranking_rule.rs index d8f6836e7..dd25ddd4a 100644 --- a/milli/src/search/new/graph_based_ranking_rule.rs +++ b/milli/src/search/new/graph_based_ranking_rule.rs @@ -46,7 +46,7 @@ use super::logger::SearchLogger; use super::query_graph::QueryNode; use super::ranking_rule_graph::{ ConditionDocIdsCache, DeadEndsCache, ExactnessGraph, FidGraph, PositionGraph, ProximityGraph, - RankingRuleGraph, RankingRuleGraphTrait, TypoGraph, + RankingRuleGraph, RankingRuleGraphTrait, TypoGraph, WordsGraph, }; use super::small_bitmap::SmallBitmap; use super::{QueryGraph, RankingRule, RankingRuleOutput, SearchContext}; @@ -54,6 +54,12 @@ use crate::search::new::query_term::LocatedQueryTermSubset; use crate::search::new::ranking_rule_graph::PathVisitor; use crate::{Result, TermsMatchingStrategy}; +pub type Words = GraphBasedRankingRule; +impl GraphBasedRankingRule { + pub fn new(terms_matching_strategy: TermsMatchingStrategy) -> Self { + Self::new_with_id("words".to_owned(), Some(terms_matching_strategy)) + } +} pub type Proximity = GraphBasedRankingRule; impl GraphBasedRankingRule { pub fn new(terms_matching_strategy: Option) -> Self { diff --git a/milli/src/search/new/logger/visual.rs b/milli/src/search/new/logger/visual.rs index 1cbe007d3..8df56da89 100644 --- a/milli/src/search/new/logger/visual.rs +++ b/milli/src/search/new/logger/visual.rs @@ -4,7 +4,6 @@ use std::io::{BufWriter, Write}; use std::path::{Path, PathBuf}; use std::time::Instant; -// use rand::random; use roaring::RoaringBitmap; use crate::search::new::interner::Interned; @@ -13,6 +12,7 @@ use crate::search::new::query_term::LocatedQueryTermSubset; use crate::search::new::ranking_rule_graph::{ Edge, FidCondition, FidGraph, PositionCondition, PositionGraph, ProximityCondition, ProximityGraph, RankingRuleGraph, RankingRuleGraphTrait, TypoCondition, TypoGraph, + WordsCondition, WordsGraph, }; use crate::search::new::ranking_rules::BoxRankingRule; use crate::search::new::{QueryGraph, QueryNode, RankingRule, SearchContext, SearchLogger}; @@ -24,11 +24,12 @@ pub enum SearchEvents { RankingRuleSkipBucket { ranking_rule_idx: usize, bucket_len: u64 }, RankingRuleEndIteration { ranking_rule_idx: usize, universe_len: u64 }, ExtendResults { new: Vec }, - WordsGraph { query_graph: QueryGraph }, ProximityGraph { graph: RankingRuleGraph }, ProximityPaths { paths: Vec>> }, TypoGraph { graph: RankingRuleGraph }, TypoPaths { paths: Vec>> }, + WordsGraph { graph: RankingRuleGraph }, + WordsPaths { paths: Vec>> }, FidGraph { graph: RankingRuleGraph }, FidPaths { paths: Vec>> }, PositionGraph { graph: RankingRuleGraph }, @@ -139,8 +140,11 @@ impl SearchLogger for VisualSearchLogger { let Some(location) = self.location.last() else { return }; match location { Location::Words => { - if let Some(query_graph) = state.downcast_ref::() { - self.events.push(SearchEvents::WordsGraph { query_graph: query_graph.clone() }); + if let Some(graph) = state.downcast_ref::>() { + self.events.push(SearchEvents::WordsGraph { graph: graph.clone() }); + } + if let Some(paths) = state.downcast_ref::>>>() { + self.events.push(SearchEvents::WordsPaths { paths: paths.clone() }); } } Location::Typo => { @@ -329,7 +333,6 @@ impl<'ctx> DetailedLoggerFinish<'ctx> { SearchEvents::ExtendResults { new } => { self.write_extend_results(new)?; } - SearchEvents::WordsGraph { query_graph } => self.write_words_graph(query_graph)?, SearchEvents::ProximityGraph { graph } => self.write_rr_graph(&graph)?, SearchEvents::ProximityPaths { paths } => { self.write_rr_graph_paths::(paths)?; @@ -338,6 +341,10 @@ impl<'ctx> DetailedLoggerFinish<'ctx> { SearchEvents::TypoPaths { paths } => { self.write_rr_graph_paths::(paths)?; } + SearchEvents::WordsGraph { graph } => self.write_rr_graph(&graph)?, + SearchEvents::WordsPaths { paths } => { + self.write_rr_graph_paths::(paths)?; + } SearchEvents::FidGraph { graph } => self.write_rr_graph(&graph)?, SearchEvents::FidPaths { paths } => { self.write_rr_graph_paths::(paths)?; @@ -455,7 +462,7 @@ fill: \"#B6E2D3\" shape: class max_nbr_typo: {}", term_subset.description(ctx), - term_subset.max_nbr_typos(ctx) + term_subset.max_typo_cost(ctx) )?; for w in term_subset.all_single_words_except_prefix_db(ctx)? { @@ -482,13 +489,6 @@ fill: \"#B6E2D3\" } Ok(()) } - fn write_words_graph(&mut self, qg: QueryGraph) -> Result<()> { - self.make_new_file_for_internal_state_if_needed()?; - - self.write_query_graph(&qg)?; - - Ok(()) - } fn write_rr_graph( &mut self, graph: &RankingRuleGraph, diff --git a/milli/src/search/new/mod.rs b/milli/src/search/new/mod.rs index 7e8426bf9..a28f42f35 100644 --- a/milli/src/search/new/mod.rs +++ b/milli/src/search/new/mod.rs @@ -15,11 +15,7 @@ mod resolve_query_graph; mod small_bitmap; mod exact_attribute; -// TODO: documentation + comments -// implementation is currently an adaptation of the previous implementation to fit with the new model mod sort; -// TODO: documentation + comments -mod words; #[cfg(test)] mod tests; @@ -43,10 +39,10 @@ use ranking_rules::{ use resolve_query_graph::{compute_query_graph_docids, PhraseDocIdsCache}; use roaring::RoaringBitmap; use sort::Sort; -use words::Words; use self::geo_sort::GeoSort; pub use self::geo_sort::Strategy as GeoSortStrategy; +use self::graph_based_ranking_rule::Words; use self::interner::Interned; use crate::search::new::distinct::apply_distinct_rule; use crate::{AscDesc, DocumentId, Filter, Index, Member, Result, TermsMatchingStrategy, UserError}; @@ -202,6 +198,11 @@ fn get_ranking_rules_for_query_graph_search<'ctx>( let mut sorted_fields = HashSet::new(); let mut geo_sorted = false; + // Don't add the `words` ranking rule if the term matching strategy is `All` + if matches!(terms_matching_strategy, TermsMatchingStrategy::All) { + words = true; + } + let mut ranking_rules: Vec> = vec![]; let settings_ranking_rules = ctx.index.criteria(ctx.txn)?; for rr in settings_ranking_rules { @@ -397,8 +398,8 @@ pub fn execute_search( None }; let bucket_sort_output = if let Some(query_terms) = query_terms { - let graph = QueryGraph::from_query(ctx, &query_terms)?; - located_query_terms = Some(query_terms); + let (graph, new_located_query_terms) = QueryGraph::from_query(ctx, &query_terms)?; + located_query_terms = Some(new_located_query_terms); let ranking_rules = get_ranking_rules_for_query_graph_search( ctx, diff --git a/milli/src/search/new/query_graph.rs b/milli/src/search/new/query_graph.rs index 0e7d5a7f3..dc25d1bc3 100644 --- a/milli/src/search/new/query_graph.rs +++ b/milli/src/search/new/query_graph.rs @@ -88,12 +88,15 @@ pub struct QueryGraph { } impl QueryGraph { - /// Build the query graph from the parsed user search query. + /// Build the query graph from the parsed user search query, return an updated list of the located query terms + /// which contains ngrams. pub fn from_query( ctx: &mut SearchContext, // NOTE: the terms here must be consecutive terms: &[LocatedQueryTerm], - ) -> Result { + ) -> Result<(QueryGraph, Vec)> { + let mut new_located_query_terms = terms.to_vec(); + let nbr_typos = number_of_typos_allowed(ctx)?; let mut nodes_data: Vec = vec![QueryNodeData::Start, QueryNodeData::End]; @@ -107,10 +110,11 @@ impl QueryGraph { let original_terms_len = terms.len(); for term_idx in 0..original_terms_len { let mut new_nodes = vec![]; + let new_node_idx = add_node( &mut nodes_data, QueryNodeData::Term(LocatedQueryTermSubset { - term_subset: QueryTermSubset::full(Interned::from_raw(term_idx as u16)), + term_subset: QueryTermSubset::full(terms[term_idx].value), positions: terms[term_idx].positions.clone(), term_ids: term_idx as u8..=term_idx as u8, }), @@ -121,6 +125,7 @@ impl QueryGraph { if let Some(ngram) = query_term::make_ngram(ctx, &terms[term_idx - 1..=term_idx], &nbr_typos)? { + new_located_query_terms.push(ngram.clone()); let ngram_idx = add_node( &mut nodes_data, QueryNodeData::Term(LocatedQueryTermSubset { @@ -136,6 +141,7 @@ impl QueryGraph { if let Some(ngram) = query_term::make_ngram(ctx, &terms[term_idx - 2..=term_idx], &nbr_typos)? { + new_located_query_terms.push(ngram.clone()); let ngram_idx = add_node( &mut nodes_data, QueryNodeData::Term(LocatedQueryTermSubset { @@ -167,7 +173,7 @@ impl QueryGraph { let mut graph = QueryGraph { root_node, end_node, nodes }; graph.build_initial_edges(); - Ok(graph) + Ok((graph, new_located_query_terms)) } /// Remove the given nodes, connecting all their predecessors to all their successors. diff --git a/milli/src/search/new/query_term/compute_derivations.rs b/milli/src/search/new/query_term/compute_derivations.rs index 0da841890..d5dfbbcd0 100644 --- a/milli/src/search/new/query_term/compute_derivations.rs +++ b/milli/src/search/new/query_term/compute_derivations.rs @@ -28,16 +28,14 @@ pub enum ZeroOrOneTypo { impl Interned { pub fn compute_fully_if_needed(self, ctx: &mut SearchContext) -> Result<()> { let s = ctx.term_interner.get_mut(self); - if s.max_nbr_typos == 0 { - s.one_typo = Lazy::Init(OneTypoTerm::default()); - s.two_typo = Lazy::Init(TwoTypoTerm::default()); - } else if s.max_nbr_typos == 1 && s.one_typo.is_uninit() { + if s.max_levenshtein_distance <= 1 && s.one_typo.is_uninit() { assert!(s.two_typo.is_uninit()); + // Initialize one_typo subterm even if max_nbr_typo is 0 because of split words self.initialize_one_typo_subterm(ctx)?; let s = ctx.term_interner.get_mut(self); assert!(s.one_typo.is_init()); s.two_typo = Lazy::Init(TwoTypoTerm::default()); - } else if s.max_nbr_typos > 1 && s.two_typo.is_uninit() { + } else if s.max_levenshtein_distance > 1 && s.two_typo.is_uninit() { assert!(s.two_typo.is_uninit()); self.initialize_one_and_two_typo_subterm(ctx)?; let s = ctx.term_interner.get_mut(self); @@ -187,7 +185,7 @@ pub fn partially_initialized_term_from_word( original: ctx.word_interner.insert(word.to_owned()), ngram_words: None, is_prefix: false, - max_nbr_typos: 0, + max_levenshtein_distance: 0, zero_typo: <_>::default(), one_typo: Lazy::Init(<_>::default()), two_typo: Lazy::Init(<_>::default()), @@ -258,7 +256,7 @@ pub fn partially_initialized_term_from_word( Ok(QueryTerm { original: word_interned, ngram_words: None, - max_nbr_typos: max_typo, + max_levenshtein_distance: max_typo, is_prefix, zero_typo, one_typo: Lazy::Uninit, @@ -277,7 +275,16 @@ fn find_split_words(ctx: &mut SearchContext, word: &str) -> Result { fn initialize_one_typo_subterm(self, ctx: &mut SearchContext) -> Result<()> { let self_mut = ctx.term_interner.get_mut(self); - let QueryTerm { original, is_prefix, one_typo, .. } = self_mut; + + let allows_split_words = self_mut.allows_split_words(); + let QueryTerm { + original, + is_prefix, + one_typo, + max_levenshtein_distance: max_nbr_typos, + .. + } = self_mut; + let original = *original; let is_prefix = *is_prefix; // let original_str = ctx.word_interner.get(*original).to_owned(); @@ -286,26 +293,33 @@ impl Interned { } let mut one_typo_words = BTreeSet::new(); - find_zero_one_typo_derivations(ctx, original, is_prefix, |derived_word, nbr_typos| { - match nbr_typos { - ZeroOrOneTypo::Zero => {} - ZeroOrOneTypo::One => { - if one_typo_words.len() < limits::MAX_ONE_TYPO_COUNT { - one_typo_words.insert(derived_word); - } else { - return Ok(ControlFlow::Break(())); + if *max_nbr_typos > 0 { + find_zero_one_typo_derivations(ctx, original, is_prefix, |derived_word, nbr_typos| { + match nbr_typos { + ZeroOrOneTypo::Zero => {} + ZeroOrOneTypo::One => { + if one_typo_words.len() < limits::MAX_ONE_TYPO_COUNT { + one_typo_words.insert(derived_word); + } else { + return Ok(ControlFlow::Break(())); + } } } - } - Ok(ControlFlow::Continue(())) - })?; - let original_str = ctx.word_interner.get(original).to_owned(); - let split_words = find_split_words(ctx, original_str.as_str())?; + Ok(ControlFlow::Continue(())) + })?; + } + + let split_words = if allows_split_words { + let original_str = ctx.word_interner.get(original).to_owned(); + find_split_words(ctx, original_str.as_str())? + } else { + None + }; let self_mut = ctx.term_interner.get_mut(self); // Only add the split words to the derivations if: - // 1. the term is not an ngram; OR + // 1. the term is neither an ngram nor a phrase; OR // 2. the term is an ngram, but the split words are different from the ngram's component words let split_words = if let Some((ngram_words, split_words)) = self_mut.ngram_words.as_ref().zip(split_words.as_ref()) @@ -327,7 +341,13 @@ impl Interned { } fn initialize_one_and_two_typo_subterm(self, ctx: &mut SearchContext) -> Result<()> { let self_mut = ctx.term_interner.get_mut(self); - let QueryTerm { original, is_prefix, two_typo, .. } = self_mut; + let QueryTerm { + original, + is_prefix, + two_typo, + max_levenshtein_distance: max_nbr_typos, + .. + } = self_mut; let original_str = ctx.word_interner.get(*original).to_owned(); if two_typo.is_init() { return Ok(()); @@ -335,34 +355,37 @@ impl Interned { let mut one_typo_words = BTreeSet::new(); let mut two_typo_words = BTreeSet::new(); - find_zero_one_two_typo_derivations( - *original, - *is_prefix, - ctx.index.words_fst(ctx.txn)?, - &mut ctx.word_interner, - |derived_word, nbr_typos| { - if one_typo_words.len() >= limits::MAX_ONE_TYPO_COUNT - && two_typo_words.len() >= limits::MAX_TWO_TYPOS_COUNT - { - // No chance we will add either one- or two-typo derivations anymore, stop iterating. - return Ok(ControlFlow::Break(())); - } - match nbr_typos { - NumberOfTypos::Zero => {} - NumberOfTypos::One => { - if one_typo_words.len() < limits::MAX_ONE_TYPO_COUNT { - one_typo_words.insert(derived_word); + if *max_nbr_typos > 0 { + find_zero_one_two_typo_derivations( + *original, + *is_prefix, + ctx.index.words_fst(ctx.txn)?, + &mut ctx.word_interner, + |derived_word, nbr_typos| { + if one_typo_words.len() >= limits::MAX_ONE_TYPO_COUNT + && two_typo_words.len() >= limits::MAX_TWO_TYPOS_COUNT + { + // No chance we will add either one- or two-typo derivations anymore, stop iterating. + return Ok(ControlFlow::Break(())); + } + match nbr_typos { + NumberOfTypos::Zero => {} + NumberOfTypos::One => { + if one_typo_words.len() < limits::MAX_ONE_TYPO_COUNT { + one_typo_words.insert(derived_word); + } + } + NumberOfTypos::Two => { + if two_typo_words.len() < limits::MAX_TWO_TYPOS_COUNT { + two_typo_words.insert(derived_word); + } } } - NumberOfTypos::Two => { - if two_typo_words.len() < limits::MAX_TWO_TYPOS_COUNT { - two_typo_words.insert(derived_word); - } - } - } - Ok(ControlFlow::Continue(())) - }, - )?; + Ok(ControlFlow::Continue(())) + }, + )?; + } + let split_words = find_split_words(ctx, original_str.as_str())?; let self_mut = ctx.term_interner.get_mut(self); diff --git a/milli/src/search/new/query_term/mod.rs b/milli/src/search/new/query_term/mod.rs index bf521d9b2..fb749a797 100644 --- a/milli/src/search/new/query_term/mod.rs +++ b/milli/src/search/new/query_term/mod.rs @@ -43,7 +43,7 @@ pub struct QueryTermSubset { pub struct QueryTerm { original: Interned, ngram_words: Option>>, - max_nbr_typos: u8, + max_levenshtein_distance: u8, is_prefix: bool, zero_typo: ZeroTypoTerm, // May not be computed yet @@ -342,10 +342,16 @@ impl QueryTermSubset { } None } - pub fn max_nbr_typos(&self, ctx: &SearchContext) -> u8 { + pub fn max_typo_cost(&self, ctx: &SearchContext) -> u8 { let t = ctx.term_interner.get(self.original); - match t.max_nbr_typos { - 0 => 0, + match t.max_levenshtein_distance { + 0 => { + if t.allows_split_words() { + 1 + } else { + 0 + } + } 1 => { if self.one_typo_subset.is_empty() { 0 @@ -438,6 +444,9 @@ impl QueryTerm { self.zero_typo.is_empty() && one_typo.is_empty() && two_typo.is_empty() } + fn allows_split_words(&self) -> bool { + self.zero_typo.phrase.is_none() + } } impl Interned { diff --git a/milli/src/search/new/query_term/parse_query.rs b/milli/src/search/new/query_term/parse_query.rs index dc317a0fb..69c2cd9c9 100644 --- a/milli/src/search/new/query_term/parse_query.rs +++ b/milli/src/search/new/query_term/parse_query.rs @@ -77,13 +77,9 @@ pub fn located_query_terms_from_tokens( } } TokenKind::Separator(separator_kind) => { - match separator_kind { - SeparatorKind::Hard => { - position += 1; - } - SeparatorKind::Soft => { - position += 0; - } + // add penalty for hard separators + if let SeparatorKind::Hard = separator_kind { + position = position.wrapping_add(1); } phrase = 'phrase: { @@ -217,7 +213,7 @@ pub fn make_ngram( original: ngram_str_interned, ngram_words: Some(words_interned), is_prefix, - max_nbr_typos, + max_levenshtein_distance: max_nbr_typos, zero_typo: term.zero_typo, one_typo: Lazy::Uninit, two_typo: Lazy::Uninit, @@ -271,7 +267,7 @@ impl PhraseBuilder { QueryTerm { original: ctx.word_interner.insert(phrase_desc), ngram_words: None, - max_nbr_typos: 0, + max_levenshtein_distance: 0, is_prefix: false, zero_typo: ZeroTypoTerm { phrase: Some(phrase), @@ -288,3 +284,36 @@ impl PhraseBuilder { }) } } + +#[cfg(test)] +mod tests { + use charabia::TokenizerBuilder; + + use super::*; + use crate::index::tests::TempIndex; + + fn temp_index_with_documents() -> TempIndex { + let temp_index = TempIndex::new(); + temp_index + .add_documents(documents!([ + { "id": 1, "name": "split this world westfali westfalia the Ŵôřlḑôle" }, + { "id": 2, "name": "Westfália" }, + { "id": 3, "name": "Ŵôřlḑôle" }, + ])) + .unwrap(); + temp_index + } + + #[test] + fn start_with_hard_separator() -> Result<()> { + let tokenizer = TokenizerBuilder::new().build(); + let tokens = tokenizer.tokenize("."); + let index = temp_index_with_documents(); + let rtxn = index.read_txn()?; + let mut ctx = SearchContext::new(&index, &rtxn); + // panics with `attempt to add with overflow` before + let located_query_terms = located_query_terms_from_tokens(&mut ctx, tokens, None)?; + assert!(located_query_terms.is_empty()); + Ok(()) + } +} diff --git a/milli/src/search/new/ranking_rule_graph/cheapest_paths.rs b/milli/src/search/new/ranking_rule_graph/cheapest_paths.rs index c065cc706..8fd943e6e 100644 --- a/milli/src/search/new/ranking_rule_graph/cheapest_paths.rs +++ b/milli/src/search/new/ranking_rule_graph/cheapest_paths.rs @@ -205,18 +205,12 @@ impl VisitorState { impl RankingRuleGraph { pub fn find_all_costs_to_end(&self) -> MappedInterner> { let mut costs_to_end = self.query_graph.nodes.map(|_| vec![]); - let mut enqueued = SmallBitmap::new(self.query_graph.nodes.len()); - let mut node_stack = VecDeque::new(); - - *costs_to_end.get_mut(self.query_graph.end_node) = vec![0]; - - for prev_node in self.query_graph.nodes.get(self.query_graph.end_node).predecessors.iter() { - node_stack.push_back(prev_node); - enqueued.insert(prev_node); - } - - while let Some(cur_node) = node_stack.pop_front() { + self.traverse_breadth_first_backward(self.query_graph.end_node, |cur_node| { + if cur_node == self.query_graph.end_node { + *costs_to_end.get_mut(self.query_graph.end_node) = vec![0]; + return; + } let mut self_costs = Vec::::new(); let cur_node_edges = &self.edges_of_node.get(cur_node); @@ -232,13 +226,7 @@ impl RankingRuleGraph { self_costs.dedup(); *costs_to_end.get_mut(cur_node) = self_costs; - for prev_node in self.query_graph.nodes.get(cur_node).predecessors.iter() { - if !enqueued.contains(prev_node) { - node_stack.push_back(prev_node); - enqueued.insert(prev_node); - } - } - } + }); costs_to_end } @@ -247,17 +235,12 @@ impl RankingRuleGraph { node_with_removed_outgoing_conditions: Interned, costs: &mut MappedInterner>, ) { - let mut enqueued = SmallBitmap::new(self.query_graph.nodes.len()); - let mut node_stack = VecDeque::new(); - - enqueued.insert(node_with_removed_outgoing_conditions); - node_stack.push_back(node_with_removed_outgoing_conditions); - - 'main_loop: while let Some(cur_node) = node_stack.pop_front() { + // Traverse the graph backward from the target node, recomputing the cost for each of its predecessors. + // We first check that no other node is contributing the same total cost to a predecessor before removing + // the cost from the predecessor. + self.traverse_breadth_first_backward(node_with_removed_outgoing_conditions, |cur_node| { let mut costs_to_remove = FxHashSet::default(); - for c in costs.get(cur_node) { - costs_to_remove.insert(*c); - } + costs_to_remove.extend(costs.get(cur_node).iter().copied()); let cur_node_edges = &self.edges_of_node.get(cur_node); for edge_idx in cur_node_edges.iter() { @@ -265,22 +248,75 @@ impl RankingRuleGraph { for cost in costs.get(edge.dest_node).iter() { costs_to_remove.remove(&(*cost + edge.cost as u64)); if costs_to_remove.is_empty() { - continue 'main_loop; + return; } } } if costs_to_remove.is_empty() { - continue 'main_loop; + return; } let mut new_costs = BTreeSet::from_iter(costs.get(cur_node).iter().copied()); for c in costs_to_remove { new_costs.remove(&c); } *costs.get_mut(cur_node) = new_costs.into_iter().collect(); + }); + } + /// Traverse the graph backwards from the given node such that every time + /// a node is visited, we are guaranteed that all its successors either: + /// 1. have already been visited; OR + /// 2. were not reachable from the given node + pub fn traverse_breadth_first_backward( + &self, + from: Interned, + mut visit: impl FnMut(Interned), + ) { + let mut reachable = SmallBitmap::for_interned_values_in(&self.query_graph.nodes); + { + // go backward to get the set of all reachable nodes from the given node + // the nodes that are not reachable will be set as `visited` + let mut stack = VecDeque::new(); + let mut enqueued = SmallBitmap::for_interned_values_in(&self.query_graph.nodes); + enqueued.insert(from); + stack.push_back(from); + while let Some(n) = stack.pop_front() { + if reachable.contains(n) { + continue; + } + reachable.insert(n); + for prev_node in self.query_graph.nodes.get(n).predecessors.iter() { + if !enqueued.contains(prev_node) && !reachable.contains(prev_node) { + stack.push_back(prev_node); + enqueued.insert(prev_node); + } + } + } + }; + let mut unreachable_or_visited = + SmallBitmap::for_interned_values_in(&self.query_graph.nodes); + for (n, _) in self.query_graph.nodes.iter() { + if !reachable.contains(n) { + unreachable_or_visited.insert(n); + } + } + + let mut enqueued = SmallBitmap::for_interned_values_in(&self.query_graph.nodes); + let mut stack = VecDeque::new(); + + enqueued.insert(from); + stack.push_back(from); + + while let Some(cur_node) = stack.pop_front() { + if !self.query_graph.nodes.get(cur_node).successors.is_subset(&unreachable_or_visited) { + stack.push_back(cur_node); + continue; + } + unreachable_or_visited.insert(cur_node); + visit(cur_node); for prev_node in self.query_graph.nodes.get(cur_node).predecessors.iter() { - if !enqueued.contains(prev_node) { - node_stack.push_back(prev_node); + if !enqueued.contains(prev_node) && !unreachable_or_visited.contains(prev_node) { + stack.push_back(prev_node); enqueued.insert(prev_node); } } diff --git a/milli/src/search/new/ranking_rule_graph/mod.rs b/milli/src/search/new/ranking_rule_graph/mod.rs index f60c481de..8de455822 100644 --- a/milli/src/search/new/ranking_rule_graph/mod.rs +++ b/milli/src/search/new/ranking_rule_graph/mod.rs @@ -20,6 +20,8 @@ mod position; mod proximity; /// Implementation of the `typo` ranking rule mod typo; +/// Implementation of the `words` ranking rule +mod words; use std::collections::BTreeSet; use std::hash::Hash; @@ -33,6 +35,7 @@ pub use position::{PositionCondition, PositionGraph}; pub use proximity::{ProximityCondition, ProximityGraph}; use roaring::RoaringBitmap; pub use typo::{TypoCondition, TypoGraph}; +pub use words::{WordsCondition, WordsGraph}; use super::interner::{DedupInterner, FixedSizeInterner, Interned, MappedInterner}; use super::query_term::LocatedQueryTermSubset; diff --git a/milli/src/search/new/ranking_rule_graph/position/mod.rs b/milli/src/search/new/ranking_rule_graph/position/mod.rs index d4640097e..9b0b6478f 100644 --- a/milli/src/search/new/ranking_rule_graph/position/mod.rs +++ b/milli/src/search/new/ranking_rule_graph/position/mod.rs @@ -111,23 +111,16 @@ impl RankingRuleGraphTrait for PositionGraph { fn cost_from_position(sum_positions: u32) -> u32 { match sum_positions { - 0 | 1 | 2 | 3 => sum_positions, - 4 | 5 => 4, - 6 | 7 => 5, - 8 | 9 => 6, - 10 | 11 => 7, - 12 | 13 => 8, - 14 | 15 => 9, - 16 | 17..=24 => 10, - 25..=32 => 11, - 33..=64 => 12, - 65..=128 => 13, - 129..=256 => 14, - 257..=512 => 15, - 513..=1024 => 16, - 1025..=2048 => 17, - 2049..=4096 => 18, - 4097..=8192 => 19, - _ => 20, + 0 => 0, + 1 => 1, + 2..=4 => 2, + 5..=7 => 3, + 8..=11 => 4, + 12..=16 => 5, + 17..=24 => 6, + 25..=64 => 7, + 65..=256 => 8, + 257..=1024 => 9, + _ => 10, } } diff --git a/milli/src/search/new/ranking_rule_graph/typo/mod.rs b/milli/src/search/new/ranking_rule_graph/typo/mod.rs index da5198c23..a44be6015 100644 --- a/milli/src/search/new/ranking_rule_graph/typo/mod.rs +++ b/milli/src/search/new/ranking_rule_graph/typo/mod.rs @@ -50,7 +50,7 @@ impl RankingRuleGraphTrait for TypoGraph { // 3-gram -> equivalent to 2 typos let base_cost = if term.term_ids.len() == 1 { 0 } else { term.term_ids.len() as u32 }; - for nbr_typos in 0..=term.term_subset.max_nbr_typos(ctx) { + for nbr_typos in 0..=term.term_subset.max_typo_cost(ctx) { let mut term = term.clone(); match nbr_typos { 0 => { diff --git a/milli/src/search/new/ranking_rule_graph/words/mod.rs b/milli/src/search/new/ranking_rule_graph/words/mod.rs new file mode 100644 index 000000000..0a0cc112b --- /dev/null +++ b/milli/src/search/new/ranking_rule_graph/words/mod.rs @@ -0,0 +1,49 @@ +use roaring::RoaringBitmap; + +use super::{ComputedCondition, RankingRuleGraphTrait}; +use crate::search::new::interner::{DedupInterner, Interned}; +use crate::search::new::query_term::LocatedQueryTermSubset; +use crate::search::new::resolve_query_graph::compute_query_term_subset_docids; +use crate::search::new::SearchContext; +use crate::Result; + +#[derive(Clone, PartialEq, Eq, Hash)] +pub struct WordsCondition { + term: LocatedQueryTermSubset, +} + +pub enum WordsGraph {} + +impl RankingRuleGraphTrait for WordsGraph { + type Condition = WordsCondition; + + fn resolve_condition( + ctx: &mut SearchContext, + condition: &Self::Condition, + universe: &RoaringBitmap, + ) -> Result { + let WordsCondition { term, .. } = condition; + // maybe compute_query_term_subset_docids should accept a universe as argument + let mut docids = compute_query_term_subset_docids(ctx, &term.term_subset)?; + docids &= universe; + + Ok(ComputedCondition { + docids, + universe_len: universe.len(), + start_term_subset: None, + end_term_subset: term.clone(), + }) + } + + fn build_edges( + _ctx: &mut SearchContext, + conditions_interner: &mut DedupInterner, + _from: Option<&LocatedQueryTermSubset>, + to_term: &LocatedQueryTermSubset, + ) -> Result)>> { + Ok(vec![( + to_term.term_ids.len() as u32, + conditions_interner.insert(WordsCondition { term: to_term.clone() }), + )]) + } +} diff --git a/milli/src/search/new/tests/attribute_position.rs b/milli/src/search/new/tests/attribute_position.rs index 5e16cd023..37f303b10 100644 --- a/milli/src/search/new/tests/attribute_position.rs +++ b/milli/src/search/new/tests/attribute_position.rs @@ -138,7 +138,7 @@ fn test_attribute_position_simple() { s.terms_matching_strategy(TermsMatchingStrategy::All); s.query("quick brown"); let SearchResult { documents_ids, .. } = s.execute().unwrap(); - insta::assert_snapshot!(format!("{documents_ids:?}"), @"[10, 11, 12, 13, 3, 4, 2, 1, 0, 6, 8, 7, 9, 5]"); + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[10, 11, 12, 13, 2, 3, 4, 1, 0, 6, 8, 7, 9, 5]"); } #[test] fn test_attribute_position_repeated() { @@ -163,7 +163,7 @@ fn test_attribute_position_different_fields() { s.terms_matching_strategy(TermsMatchingStrategy::All); s.query("quick brown"); let SearchResult { documents_ids, .. } = s.execute().unwrap(); - insta::assert_snapshot!(format!("{documents_ids:?}"), @"[10, 11, 12, 13, 3, 4, 2, 1, 0, 6, 8, 7, 9, 5]"); + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[10, 11, 12, 13, 2, 3, 4, 1, 0, 6, 8, 7, 9, 5]"); } #[test] @@ -176,5 +176,5 @@ fn test_attribute_position_ngrams() { s.terms_matching_strategy(TermsMatchingStrategy::All); s.query("quick brown"); let SearchResult { documents_ids, .. } = s.execute().unwrap(); - insta::assert_snapshot!(format!("{documents_ids:?}"), @"[10, 11, 12, 13, 3, 4, 2, 1, 0, 6, 8, 7, 9, 5]"); + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[10, 11, 12, 13, 2, 3, 4, 1, 0, 6, 8, 7, 9, 5]"); } diff --git a/milli/src/search/new/tests/ngram_split_words.rs b/milli/src/search/new/tests/ngram_split_words.rs index 2a0365bac..fb99b8ba2 100644 --- a/milli/src/search/new/tests/ngram_split_words.rs +++ b/milli/src/search/new/tests/ngram_split_words.rs @@ -3,9 +3,9 @@ This module tests the following properties: 1. Two consecutive words from a query can be combined into a "2gram" 2. Three consecutive words from a query can be combined into a "3gram" -3. A word from the query can be split into two consecutive words (split words) +3. A word from the query can be split into two consecutive words (split words), no matter how short it is 4. A 2gram can be split into two words -5. A 3gram cannot be split into two words +5. A 3gram can be split into two words 6. 2grams can contain up to 1 typo 7. 3grams cannot have typos 8. 2grams and 3grams can be prefix tolerant @@ -14,6 +14,7 @@ This module tests the following properties: 11. Disabling typo tolerance does not disable ngram tolerance 12. Prefix tolerance is disabled for the last word if a space follows it 13. Ngrams cannot be formed by combining a phrase and a word or two phrases +14. Split words are not disabled by the `disableOnAttribute` or `disableOnWords` typo settings */ use crate::index::tests::TempIndex; @@ -56,6 +57,10 @@ fn create_index() -> TempIndex { { "id": 5, "text": "sunflowering is not a verb" + }, + { + "id": 6, + "text": "xy z" } ])) .unwrap(); @@ -263,10 +268,11 @@ fn test_disable_split_words() { s.query("sunflower "); let SearchResult { documents_ids, .. } = s.execute().unwrap(); // no document containing `sun flower` - insta::assert_snapshot!(format!("{documents_ids:?}"), @"[3]"); + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[1, 3]"); let texts = collect_field_values(&index, &txn, "text", &documents_ids); insta::assert_debug_snapshot!(texts, @r###" [ + "\"the sun flower is tall\"", "\"the sunflower is tall\"", ] "###); @@ -307,10 +313,11 @@ fn test_3gram_no_split_words() { let SearchResult { documents_ids, .. } = s.execute().unwrap(); // no document with `sun flower` - insta::assert_snapshot!(format!("{documents_ids:?}"), @"[2, 3, 5]"); + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[1, 2, 3, 5]"); let texts = collect_field_values(&index, &txn, "text", &documents_ids); insta::assert_debug_snapshot!(texts, @r###" [ + "\"the sun flower is tall\"", "\"the sunflowers are pretty\"", "\"the sunflower is tall\"", "\"sunflowering is not a verb\"", @@ -369,3 +376,50 @@ fn test_no_ngram_phrases() { ] "###); } + +#[test] +fn test_short_split_words() { + let index = create_index(); + let txn = index.read_txn().unwrap(); + + let mut s = Search::new(&txn, &index); + s.terms_matching_strategy(TermsMatchingStrategy::All); + s.query("xyz"); + let SearchResult { documents_ids, .. } = s.execute().unwrap(); + + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[6]"); + let texts = collect_field_values(&index, &txn, "text", &documents_ids); + insta::assert_debug_snapshot!(texts, @r###" + [ + "\"xy z\"", + ] + "###); +} + +#[test] +fn test_split_words_never_disabled() { + let index = create_index(); + + index + .update_settings(|s| { + s.set_exact_words(["sunflower"].iter().map(ToString::to_string).collect()); + s.set_exact_attributes(["text"].iter().map(ToString::to_string).collect()); + }) + .unwrap(); + + let txn = index.read_txn().unwrap(); + + let mut s = Search::new(&txn, &index); + s.terms_matching_strategy(TermsMatchingStrategy::All); + s.query("the sunflower is tall"); + let SearchResult { documents_ids, .. } = s.execute().unwrap(); + + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[1, 3]"); + let texts = collect_field_values(&index, &txn, "text", &documents_ids); + insta::assert_debug_snapshot!(texts, @r###" + [ + "\"the sun flower is tall\"", + "\"the sunflower is tall\"", + ] + "###); +} diff --git a/milli/src/search/new/tests/typo.rs b/milli/src/search/new/tests/typo.rs index 33b165a94..8fd9de5fc 100644 --- a/milli/src/search/new/tests/typo.rs +++ b/milli/src/search/new/tests/typo.rs @@ -9,7 +9,7 @@ This module tests the following properties: 6. A typo on the first letter of a word counts as two typos 7. Phrases are not typo tolerant 8. 2grams can have 1 typo if they are larger than `min_word_len_two_typos` -9. 3grams are not typo tolerant +9. 3grams are not typo tolerant (but they can be split into two words) 10. The `typo` ranking rule assumes the role of the `words` ranking rule implicitly if `words` doesn't exist before it. 11. The `typo` ranking rule places documents with the same number of typos in the same bucket @@ -287,16 +287,17 @@ fn test_typo_exact_word() { ] "###); - // exact words do not disable prefix (sunflowering OK, but no sunflowar or sun flower) + // exact words do not disable prefix (sunflowering OK, but no sunflowar) let mut s = Search::new(&txn, &index); s.terms_matching_strategy(TermsMatchingStrategy::All); s.query("network interconnection sunflower"); let SearchResult { documents_ids, .. } = s.execute().unwrap(); - insta::assert_snapshot!(format!("{documents_ids:?}"), @"[16, 18]"); + insta::assert_snapshot!(format!("{documents_ids:?}"), @"[16, 17, 18]"); let texts = collect_field_values(&index, &txn, "text", &documents_ids); insta::assert_debug_snapshot!(texts, @r###" [ "\"network interconnection sunflower\"", + "\"network interconnection sun flower\"", "\"network interconnection sunflowering\"", ] "###); diff --git a/milli/src/search/new/words.rs b/milli/src/search/new/words.rs deleted file mode 100644 index 72b7b5916..000000000 --- a/milli/src/search/new/words.rs +++ /dev/null @@ -1,87 +0,0 @@ -use roaring::RoaringBitmap; - -use super::logger::SearchLogger; -use super::query_graph::QueryNode; -use super::resolve_query_graph::compute_query_graph_docids; -use super::small_bitmap::SmallBitmap; -use super::{QueryGraph, RankingRule, RankingRuleOutput, SearchContext}; -use crate::{Result, TermsMatchingStrategy}; - -pub struct Words { - exhausted: bool, // TODO: remove - query_graph: Option, - nodes_to_remove: Vec>, - terms_matching_strategy: TermsMatchingStrategy, -} -impl Words { - pub fn new(terms_matching_strategy: TermsMatchingStrategy) -> Self { - Self { - exhausted: true, - query_graph: None, - nodes_to_remove: vec![], - terms_matching_strategy, - } - } -} - -impl<'ctx> RankingRule<'ctx, QueryGraph> for Words { - fn id(&self) -> String { - "words".to_owned() - } - fn start_iteration( - &mut self, - ctx: &mut SearchContext<'ctx>, - _logger: &mut dyn SearchLogger, - _universe: &RoaringBitmap, - parent_query_graph: &QueryGraph, - ) -> Result<()> { - self.exhausted = false; - self.query_graph = Some(parent_query_graph.clone()); - self.nodes_to_remove = match self.terms_matching_strategy { - TermsMatchingStrategy::Last => { - let mut ns = parent_query_graph.removal_order_for_terms_matching_strategy_last(ctx); - ns.reverse(); - ns - } - TermsMatchingStrategy::All => { - vec![] - } - }; - Ok(()) - } - - fn next_bucket( - &mut self, - ctx: &mut SearchContext<'ctx>, - logger: &mut dyn SearchLogger, - universe: &RoaringBitmap, - ) -> Result>> { - if self.exhausted { - return Ok(None); - } - let Some(query_graph) = &mut self.query_graph else { panic!() }; - logger.log_internal_state(query_graph); - - let this_bucket = compute_query_graph_docids(ctx, query_graph, universe)?; - - let child_query_graph = query_graph.clone(); - - if self.nodes_to_remove.is_empty() { - self.exhausted = true; - } else { - let nodes_to_remove = self.nodes_to_remove.pop().unwrap(); - query_graph.remove_nodes_keep_edges(&nodes_to_remove.iter().collect::>()); - } - Ok(Some(RankingRuleOutput { query: child_query_graph, candidates: this_bucket })) - } - - fn end_iteration( - &mut self, - _ctx: &mut SearchContext<'ctx>, - _logger: &mut dyn SearchLogger, - ) { - self.exhausted = true; - self.nodes_to_remove = vec![]; - self.query_graph = None; - } -} diff --git a/milli/src/update/facet/mod.rs b/milli/src/update/facet/mod.rs index 39a3ef437..2fd748d4d 100644 --- a/milli/src/update/facet/mod.rs +++ b/milli/src/update/facet/mod.rs @@ -261,7 +261,9 @@ pub(crate) mod test_helpers { let options = options.map_size(4096 * 4 * 1000 * 100); let tempdir = tempfile::TempDir::new().unwrap(); let env = options.open(tempdir.path()).unwrap(); - let content = env.create_database(None).unwrap(); + let mut wtxn = env.write_txn().unwrap(); + let content = env.create_database(&mut wtxn, None).unwrap(); + wtxn.commit().unwrap(); FacetIndex { content,