From 4a6767b1fcbd7a7852a8d50c4c6eaf784074514a Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 9 Apr 2022 05:22:27 +0200 Subject: [PATCH] Implicitly group when joining searches (#1759) * Implicitly group when joining searches * Allow joining search types directly * Test search joining * Add comment for future selves (dae) * Add one more assert that shows nested grouping (dae) * Join user searches without grouping again * Flatten a few clauses in custom study (dae) --- rslib/src/backend/search/mod.rs | 7 +- rslib/src/notetype/mod.rs | 4 +- rslib/src/notetype/notetypechange.rs | 12 +-- rslib/src/notetype/schemachange.rs | 12 +-- rslib/src/scheduler/bury_and_suspend.rs | 4 +- rslib/src/scheduler/filtered/custom_study.rs | 39 ++++++--- rslib/src/scheduler/new.rs | 4 +- rslib/src/search/builder.rs | 83 +++++++++++++++++--- rslib/src/search/mod.rs | 2 +- rslib/src/search/sqlwriter.rs | 3 + 10 files changed, 118 insertions(+), 52 deletions(-) diff --git a/rslib/src/backend/search/mod.rs b/rslib/src/backend/search/mod.rs index e9026757b..66b0fe406 100644 --- a/rslib/src/backend/search/mod.rs +++ b/rslib/src/backend/search/mod.rs @@ -13,7 +13,7 @@ use crate::{ backend_proto::sort_order::Value as SortOrderProto, browser_table::Column, prelude::*, - search::{replace_search_node, Node, SortMode}, + search::{replace_search_node, JoinSearches, Node, SortMode}, }; impl SearchService for Backend { @@ -45,12 +45,11 @@ impl SearchService for Backend { fn join_search_nodes(&self, input: pb::JoinSearchNodesRequest) -> Result { let existing_node: Node = input.existing_node.unwrap_or_default().try_into()?; let additional_node: Node = input.additional_node.unwrap_or_default().try_into()?; - let search = SearchBuilder::from_root(existing_node); Ok( match pb::search_node::group::Joiner::from_i32(input.joiner).unwrap_or_default() { - pb::search_node::group::Joiner::And => search.and(additional_node), - pb::search_node::group::Joiner::Or => search.or(additional_node), + pb::search_node::group::Joiner::And => existing_node.and_flat(additional_node), + pb::search_node::group::Joiner::Or => existing_node.or_flat(additional_node), } .write() .into(), diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index eabf38b98..eb00fc10d 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -44,7 +44,7 @@ use crate::{ define_newtype, error::{CardTypeError, CardTypeErrorDetails}, prelude::*, - search::{Node, SearchNode}, + search::{JoinSearches, Node, SearchNode}, storage::comma_separated_ids, template::{FieldRequirements, ParsedTemplate}, text::ensure_string_in_nfc, @@ -222,7 +222,7 @@ impl Collection { .ok_or(AnkiError::NotFound)?; if self - .search_notes_unordered(SearchBuilder::from(note1.notetype_id).and(nids_node))? + .search_notes_unordered(note1.notetype_id.and(nids_node))? .len() != note_ids.len() { diff --git a/rslib/src/notetype/notetypechange.rs b/rslib/src/notetype/notetypechange.rs index 35c796b99..af13bf787 100644 --- a/rslib/src/notetype/notetypechange.rs +++ b/rslib/src/notetype/notetypechange.rs @@ -8,7 +8,7 @@ use std::collections::{HashMap, HashSet}; use super::{CardGenContext, Notetype, NotetypeKind}; use crate::{ prelude::*, - search::{Node, SearchNode, SortMode, TemplateKind}, + search::{JoinSearches, Node, SearchNode, SortMode, TemplateKind}, storage::comma_separated_ids, }; @@ -294,10 +294,7 @@ impl Collection { if !map.removed.is_empty() { let ords = SearchBuilder::any(map.removed.iter().map(|o| TemplateKind::Ordinal(*o as u16))); - self.search_cards_into_table( - SearchBuilder::from(nids).and(ords.group()), - SortMode::NoOrder, - )?; + self.search_cards_into_table(nids.and(ords), SortMode::NoOrder)?; for card in self.storage.all_searched_cards()? { self.remove_card_and_add_grave_undoable(card, usn)?; } @@ -319,10 +316,7 @@ impl Collection { .keys() .map(|o| TemplateKind::Ordinal(*o as u16)), ); - self.search_cards_into_table( - SearchBuilder::from(nids).and(ords.group()), - SortMode::NoOrder, - )?; + self.search_cards_into_table(nids.and(ords), SortMode::NoOrder)?; for mut card in self.storage.all_searched_cards()? { let original = card.clone(); card.template_idx = diff --git a/rslib/src/notetype/schemachange.rs b/rslib/src/notetype/schemachange.rs index cb68f69f2..0ac12b484 100644 --- a/rslib/src/notetype/schemachange.rs +++ b/rslib/src/notetype/schemachange.rs @@ -8,7 +8,7 @@ use std::collections::HashMap; use super::{CardGenContext, Notetype}; use crate::{ prelude::*, - search::{SortMode, TemplateKind}, + search::{JoinSearches, SortMode, TemplateKind}, }; /// True if any ordinals added, removed or reordered. @@ -144,10 +144,7 @@ impl Collection { // remove any cards where the template was deleted if !changes.removed.is_empty() { let ords = SearchBuilder::any(changes.removed.into_iter().map(TemplateKind::Ordinal)); - self.search_cards_into_table( - SearchBuilder::from(nt.id).and(ords.group()), - SortMode::NoOrder, - )?; + self.search_cards_into_table(nt.id.and(ords), SortMode::NoOrder)?; for card in self.storage.all_searched_cards()? { self.remove_card_and_add_grave_undoable(card, usn)?; } @@ -157,10 +154,7 @@ impl Collection { // update ordinals for cards with a repositioned template if !changes.moved.is_empty() { let ords = SearchBuilder::any(changes.moved.keys().cloned().map(TemplateKind::Ordinal)); - self.search_cards_into_table( - SearchBuilder::from(nt.id).and(ords.group()), - SortMode::NoOrder, - )?; + self.search_cards_into_table(nt.id.and(ords), SortMode::NoOrder)?; for mut card in self.storage.all_searched_cards()? { let original = card.clone(); card.template_idx = *changes.moved.get(&card.template_idx).unwrap(); diff --git a/rslib/src/scheduler/bury_and_suspend.rs b/rslib/src/scheduler/bury_and_suspend.rs index 81114b90d..c3607d9c2 100644 --- a/rslib/src/scheduler/bury_and_suspend.rs +++ b/rslib/src/scheduler/bury_and_suspend.rs @@ -10,7 +10,7 @@ use crate::{ card::CardQueue, config::SchedulerVersion, prelude::*, - search::{SearchNode, SortMode, StateKind}, + search::{JoinSearches, SearchNode, SortMode, StateKind}, }; impl Card { @@ -79,7 +79,7 @@ impl Collection { }; self.transact(Op::UnburyUnsuspend, |col| { col.search_cards_into_table( - SearchBuilder::from(SearchNode::DeckIdWithChildren(deck_id)).and(state), + SearchNode::DeckIdWithChildren(deck_id).and(state), SortMode::NoOrder, )?; col.unsuspend_or_unbury_searched_cards() diff --git a/rslib/src/scheduler/filtered/custom_study.rs b/rslib/src/scheduler/filtered/custom_study.rs index 1ab390bdc..e3dcf042b 100644 --- a/rslib/src/scheduler/filtered/custom_study.rs +++ b/rslib/src/scheduler/filtered/custom_study.rs @@ -13,7 +13,7 @@ use crate::{ decks::{FilteredDeck, FilteredSearchOrder, FilteredSearchTerm}, error::{CustomStudyError, FilteredDeckError}, prelude::*, - search::{Negated, PropertyKind, RatingKind, SearchNode, StateKind}, + search::{JoinSearches, Negated, PropertyKind, RatingKind, SearchNode, StateKind}, }; impl Collection { @@ -184,29 +184,29 @@ fn custom_study_config( } fn forgot_config(deck_name: String, days: u32) -> FilteredDeck { - let search = SearchBuilder::from(SearchNode::Rated { + let search = SearchNode::Rated { days, ease: RatingKind::AnswerButton(1), - }) + } .and(SearchNode::from_deck_name(&deck_name)) .write(); custom_study_config(false, search, FilteredSearchOrder::Random, None) } fn ahead_config(deck_name: String, days: u32) -> FilteredDeck { - let search = SearchBuilder::from(SearchNode::Property { + let search = SearchNode::Property { operator: "<=".to_string(), kind: PropertyKind::Due(days as i32), - }) + } .and(SearchNode::from_deck_name(&deck_name)) .write(); custom_study_config(true, search, FilteredSearchOrder::Due, None) } fn preview_config(deck_name: String, days: u32) -> FilteredDeck { - let search = SearchBuilder::from(StateKind::New) - .and(SearchNode::AddedInDays(days)) - .and(SearchNode::from_deck_name(&deck_name)) + let search = StateKind::New + .and_flat(SearchNode::AddedInDays(days)) + .and_flat(SearchNode::from_deck_name(&deck_name)) .write(); custom_study_config( false, @@ -237,8 +237,8 @@ fn cram_config(deck_name: String, cram: &Cram) -> Result { }; let search = nodes - .and(tags_to_nodes(&cram.tags_to_include, &cram.tags_to_exclude)) .and(SearchNode::from_deck_name(&deck_name)) + .and_flat(tags_to_nodes(&cram.tags_to_include, &cram.tags_to_exclude)) .write(); Ok(custom_study_config( @@ -261,7 +261,7 @@ fn tags_to_nodes(tags_to_include: &[String], tags_to_exclude: &[String]) -> Sear .map(|tag| SearchNode::from_tag_name(tag).negated()), ); - include_nodes.group().and(exclude_nodes) + include_nodes.and(exclude_nodes) } #[cfg(test)] @@ -357,4 +357,23 @@ mod test { Ok(()) } + + #[test] + fn sql_grouping() -> Result<()> { + let mut deck = preview_config("d".into(), 1); + assert_eq!(&deck.search_terms[0].search, "is:new added:1 deck:d"); + + let cram = Cram { + tags_to_include: vec!["1".into(), "2".into()], + tags_to_exclude: vec!["3".into(), "4".into()], + ..Default::default() + }; + deck = cram_config("d".into(), &cram)?; + assert_eq!( + &deck.search_terms[0].search, + "is:due deck:d (tag:1 OR tag:2) (-tag:3 -tag:4)" + ); + + Ok(()) + } } diff --git a/rslib/src/scheduler/new.rs b/rslib/src/scheduler/new.rs index 05a0d6999..421e6fb5b 100644 --- a/rslib/src/scheduler/new.rs +++ b/rslib/src/scheduler/new.rs @@ -14,7 +14,7 @@ use crate::{ config::{BoolKey, SchedulerVersion}, deckconfig::NewCardInsertOrder, prelude::*, - search::{SearchNode, SortMode, StateKind}, + search::{JoinSearches, SearchNode, SortMode, StateKind}, }; impl Card { @@ -267,7 +267,7 @@ impl Collection { usn: Usn, ) -> Result { let cids = self.search_cards( - SearchBuilder::from(SearchNode::DeckIdWithoutChildren(deck)).and(StateKind::New), + SearchNode::DeckIdWithoutChildren(deck).and(StateKind::New), SortMode::NoOrder, )?; self.sort_cards_inner(&cids, 1, 1, order.into(), false, usn) diff --git a/rslib/src/search/builder.rs b/rslib/src/search/builder.rs index 8a1f4a6bd..c51c50ee9 100644 --- a/rslib/src/search/builder.rs +++ b/rslib/src/search/builder.rs @@ -12,6 +12,19 @@ pub trait Negated { fn negated(self) -> Node; } +pub trait JoinSearches { + /// Concatenates two sets of [Node]s, inserting [Node::And], and grouping, if appropriate. + fn and(self, other: impl Into) -> SearchBuilder; + /// Concatenates two sets of [Node]s, inserting [Node::Or], and grouping, if appropriate. + fn or(self, other: impl Into) -> SearchBuilder; + /// Concatenates two sets of [Node]s, inserting [Node::And] if appropriate, + /// but without grouping either set. + fn and_flat(self, other: impl Into) -> SearchBuilder; + /// Concatenates two sets of [Node]s, inserting [Node::Or] if appropriate, + /// but without grouping either set. + fn or_flat(self, other: impl Into) -> SearchBuilder; +} + impl> Negated for T { fn negated(self) -> Node { let node: Node = self.into(); @@ -23,6 +36,24 @@ impl> Negated for T { } } +impl> JoinSearches for T { + fn and(self, other: impl Into) -> SearchBuilder { + self.into().join_other(other.into(), Node::And, true) + } + + fn or(self, other: impl Into) -> SearchBuilder { + self.into().join_other(other.into(), Node::Or, true) + } + + fn and_flat(self, other: impl Into) -> SearchBuilder { + self.into().join_other(other.into(), Node::And, false) + } + + fn or_flat(self, other: impl Into) -> SearchBuilder { + self.into().join_other(other.into(), Node::Or, false) + } +} + /// Helper to programmatically build searches. #[derive(Debug, PartialEq, Clone)] pub struct SearchBuilder(Vec); @@ -59,19 +90,11 @@ impl SearchBuilder { self.0.len() } - /// Concatenates the two sets of [Node]s, inserting [Node::And] if appropriate. - /// No implicit grouping is done. - pub fn and(self, other: impl Into) -> Self { - self.join_other(other.into(), Node::And) - } - - /// Concatenates the two sets of [Node]s, inserting [Node::Or] if appropriate. - /// No implicit grouping is done. - pub fn or(self, other: impl Into) -> Self { - self.join_other(other.into(), Node::Or) - } - - fn join_other(mut self, mut other: Self, joiner: Node) -> Self { + fn join_other(mut self, mut other: Self, joiner: Node, group: bool) -> Self { + if group { + self = self.group(); + other = other.group(); + } if !(self.is_empty() || other.is_empty()) { self.0.push(joiner); } @@ -185,4 +208,38 @@ mod test { Node::Not(Box::new(Node::Search(SearchNode::State(StateKind::Due)))) ) } + + #[test] + fn joining() { + assert_eq!( + StateKind::Due + .or(StateKind::New) + .and(SearchBuilder::any((1..4).map(SearchNode::Flag))) + .write(), + "(is:due OR is:new) (flag:1 OR flag:2 OR flag:3)" + ); + assert_eq!( + StateKind::Due + .or(StateKind::New) + .and_flat(SearchBuilder::any((1..4).map(SearchNode::Flag))) + .write(), + "is:due OR is:new flag:1 OR flag:2 OR flag:3" + ); + assert_eq!( + StateKind::Due + .or(StateKind::New) + .or(StateKind::Learning) + .or(StateKind::Review) + .write(), + "((is:due OR is:new) OR is:learn) OR is:review" + ); + assert_eq!( + StateKind::Due + .or_flat(StateKind::New) + .or_flat(StateKind::Learning) + .or_flat(StateKind::Review) + .write(), + "is:due OR is:new OR is:learn OR is:review" + ); + } } diff --git a/rslib/src/search/mod.rs b/rslib/src/search/mod.rs index c2c5f06c0..721c57b80 100644 --- a/rslib/src/search/mod.rs +++ b/rslib/src/search/mod.rs @@ -8,7 +8,7 @@ pub(crate) mod writer; use std::borrow::Cow; -pub use builder::{Negated, SearchBuilder}; +pub use builder::{JoinSearches, Negated, SearchBuilder}; pub use parser::{ parse as parse_search, Node, PropertyKind, RatingKind, SearchNode, StateKind, TemplateKind, }; diff --git a/rslib/src/search/sqlwriter.rs b/rslib/src/search/sqlwriter.rs index c84584922..e28ceb0bd 100644 --- a/rslib/src/search/sqlwriter.rs +++ b/rslib/src/search/sqlwriter.rs @@ -113,6 +113,9 @@ impl SqlWriter<'_> { } } + // NOTE: when adding any new nodes in the future, make sure that they are either a single + // search term, or they wrap multiple terms in parentheses, as can be seen in the sql() unit + // test at the bottom of the file. fn write_search_node_to_sql(&mut self, node: &SearchNode) -> Result<()> { use normalize_to_nfc as norm; match node {