From 5e83bac448dabea20dc9d43eb161d07de0f15d3a Mon Sep 17 00:00:00 2001 From: ManyTheFish Date: Mon, 26 Feb 2024 15:40:15 +0100 Subject: [PATCH] Fix PR comments --- milli/src/update/facet/incremental.rs | 59 +++++++++++++++------------ 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/milli/src/update/facet/incremental.rs b/milli/src/update/facet/incremental.rs index 584870a7a..798e0fe3d 100644 --- a/milli/src/update/facet/incremental.rs +++ b/milli/src/update/facet/incremental.rs @@ -18,6 +18,32 @@ use crate::update::index_documents::valid_lmdb_key; use crate::update::MergeFn; use crate::{CboRoaringBitmapCodec, Index, Result}; +/// Enum used as a return value for the facet incremental indexing. +/// +/// - `ModificationResult::InPlace` means that modifying the `facet_value` into the `level` did not have +/// an effect on the number of keys in that level. Therefore, it did not increase the number of children +/// of the parent node. +/// +/// - `ModificationResult::Insert` means that modifying the `facet_value` into the `level` resulted +/// in the addition of a new key in that level, and that therefore the number of children +/// of the parent node should be incremented. +/// +/// - `ModificationResult::Remove` means that modifying the `facet_value` into the `level` resulted in a change in the +/// number of keys in the level. For example, removing a document id from the facet value `3` could +/// cause it to have no corresponding document in level 0 anymore, and therefore the key was deleted +/// entirely. In that case, `ModificationResult::Remove` is returned. The parent of the deleted key must +/// then adjust its group size. If its group size falls to 0, then it will need to be deleted as well. +/// +/// - `ModificationResult::Reduce/Expand` means that modifying the `facet_value` into the `level` resulted in a change in the +/// bounds of the keys of the level. For example, removing a document id from the facet value +/// `3` might have caused the facet value `3` to have no corresponding document in level 0. Therefore, +/// in level 1, the key with the left bound `3` had to be changed to the next facet value (e.g. 4). +/// In that case `ModificationResult::Reduce` is returned. The parent of the reduced key may need to adjust +/// its left bound as well. +/// +/// - `ModificationResult::Nothing` means that modifying the `facet_value` didn't have any impact into the `level`. +/// This case is reachable when a document id is removed from a sub-level node but is still present in another one. +/// For example, removing `2` from a document containing `2` and `3`, the document id will removed form the `level 0` but should remain in the group node [1..4] in `level 1`. enum ModificationResult { InPlace, Expand, @@ -315,6 +341,9 @@ impl FacetsUpdateIncrementalInner { Ok(ModificationResult::Insert) } + /// Remove the docids still present in the related sub-level nodes from the del_docids. + /// + /// This process is needed to avoid removing docids from a group node where the docid is present in several sub-nodes. fn trim_del_docids<'a>( &self, txn: &mut RwTxn, @@ -352,30 +381,6 @@ impl FacetsUpdateIncrementalInner { /// ## Return /// Returns the effect of modifying the facet value to the database on the given `level`. /// - /// - `ModificationResult::InPlace` means that modifying the `facet_value` into the `level` did not have - /// an effect on the number of keys in that level. Therefore, it did not increase the number of children - /// of the parent node. - /// - /// - `ModificationResult::Insert` means that modifying the `facet_value` into the `level` resulted - /// in the addition of a new key in that level, and that therefore the number of children - /// of the parent node should be incremented. - /// - /// - `ModificationResult::Remove` means that modifying the `facet_value` into the `level` resulted in a change in the - /// number of keys in the level. For example, removing a document id from the facet value `3` could - /// cause it to have no corresponding document in level 0 anymore, and therefore the key was deleted - /// entirely. In that case, `ModificationResult::Remove` is returned. The parent of the deleted key must - /// then adjust its group size. If its group size falls to 0, then it will need to be deleted as well. - /// - /// - `ModificationResult::Reduce/Expand` means that modifying the `facet_value` into the `level` resulted in a change in the - /// bounds of the keys of the level. For example, removing a document id from the facet value - /// `3` might have caused the facet value `3` to have no corresponding document in level 0. Therefore, - /// in level 1, the key with the left bound `3` had to be changed to the next facet value (e.g. 4). - /// In that case `ModificationResult::Reduce` is returned. The parent of the reduced key may need to adjust - /// its left bound as well. - /// - /// - `ModificationResult::Nothing` means that modifying the `facet_value` didn't have any impact into the `level`. - /// This case is reachable when a document id is removed from a sub-level node but is still present in another one. - /// For example, removing `2` from a document containing `2` and `3`, the document id will removed form the `level 0` but should remain in the group node [1..4] in `level 1`. fn modify_in_level( &self, txn: &mut RwTxn, @@ -465,7 +470,7 @@ impl FacetsUpdateIncrementalInner { if updated_value.size < self.max_group_size { // If there are docids to delete, trim them avoiding unexpected removal. - let del_docids = del_docids + if let Some(del_docids) = del_docids .map(|ids| { self.trim_del_docids( txn, @@ -477,8 +482,8 @@ impl FacetsUpdateIncrementalInner { ) }) .transpose()? - .filter(|ids| !ids.is_empty()); - if let Some(del_docids) = del_docids { + .filter(|ids| !ids.is_empty()) + { updated_value.bitmap -= &*del_docids; insertion_value_was_modified = true; }