From fc977a0fe65884edf40de829e1e218eaa39763d0 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 30 Dec 2020 00:23:18 +0000 Subject: [PATCH] Limit Filter "universes" to invocation::filter Universes are really an optimization of filtering tasks, so let's define them there, and derive them from the set of conditions. This means that complex filters might get missed and end up doing a full task scan, but that's probably OK. Note that this does not fix the working-set API issues (#108 and #123). --- cli/src/argparse/filter.rs | 150 ++++++++++++++++++++----------- cli/src/argparse/mod.rs | 2 +- cli/src/argparse/subcommand.rs | 56 ++++++------ cli/src/invocation/filter.rs | 155 +++++++++++++++++++++------------ cli/src/invocation/report.rs | 20 +++-- 5 files changed, 236 insertions(+), 147 deletions(-) diff --git a/cli/src/argparse/filter.rs b/cli/src/argparse/filter.rs index f381e5862..4645de5da 100644 --- a/cli/src/argparse/filter.rs +++ b/cli/src/argparse/filter.rs @@ -2,6 +2,7 @@ use super::args::{arg_matching, id_list, minus_tag, plus_tag, TaskId}; use super::ArgList; use crate::usage; use nom::{branch::alt, combinator::*, multi::fold_many0, IResult}; +use taskchampion::Status; /// A filter represents a selection of a particular set of tasks. /// @@ -10,40 +11,11 @@ use nom::{branch::alt, combinator::*, multi::fold_many0, IResult}; /// pending tasks, or all tasks. #[derive(Debug, PartialEq, Default, Clone)] pub(crate) struct Filter { - /// The universe of tasks from which this filter can select - pub(crate) universe: Universe, - /// A set of filter conditions, all of which must match a task in order for that task to be /// selected. pub(crate) conditions: Vec, } -/// The universe of tasks over which a filter should be applied. -#[derive(Debug, PartialEq, Clone)] -pub(crate) enum Universe { - /// Only the identified tasks. Note that this may contain duplicates. - IdList(Vec), - /// All tasks in the task database - AllTasks, - /// Only pending tasks (or as an approximation, the working set) - #[allow(dead_code)] // currently only used in tests - PendingTasks, -} - -impl Universe { - /// Testing shorthand to construct a simple universe - #[cfg(test)] - pub(super) fn for_ids(mut ids: Vec) -> Self { - Universe::IdList(ids.drain(..).map(|id| TaskId::WorkingSetId(id)).collect()) - } -} - -impl Default for Universe { - fn default() -> Self { - Self::AllTasks - } -} - /// A condition which tasks must match to be accepted by the filter. #[derive(Debug, PartialEq, Clone)] pub(crate) enum Condition { @@ -52,10 +24,18 @@ pub(crate) enum Condition { /// Task does not have the given tag NoTag(String), + + // TODO: add command-line syntax for this + /// Task has the given status + Status(Status), + + /// Task has one of the given IDs + IdList(Vec), } /// Internal struct representing a parsed filter argument enum FilterArg { + // TODO: get rid of this whole enum IdList(Vec), Condition(Condition), } @@ -67,30 +47,46 @@ impl Filter { Filter { ..Default::default() }, - Self::fold_args, + |acc, arg| acc.with_arg(arg), )(input) } /// fold multiple filter args into a single Filter instance - fn fold_args(mut acc: Filter, mod_arg: FilterArg) -> Filter { + fn with_arg(mut self, mod_arg: FilterArg) -> Filter { match mod_arg { FilterArg::IdList(mut id_list) => { - // If any IDs are specified, then the filter's universe - // is those IDs. If there are already IDs, append to the - // list. - if let Universe::IdList(ref mut existing) = acc.universe { + // If there is already an IdList condition, concatenate this one + // to it. Thus multiple IdList command-line args represent an OR + // operation. This assumes that the filter is still being built + // from command-line arguments and thus has at most one IdList + // condition. + if let Some(Condition::IdList(existing)) = self + .conditions + .iter_mut() + .find(|c| matches!(c, Condition::IdList(_))) + { existing.append(&mut id_list); } else { - acc.universe = Universe::IdList(id_list); + self.conditions.push(Condition::IdList(id_list)); } } FilterArg::Condition(cond) => { - acc.conditions.push(cond); + self.conditions.push(cond); } } - acc + self } + /// combine this filter with another filter in an AND operation + pub(crate) fn intersect(mut self, mut other: Filter) -> Filter { + // simply concatenate the conditions + self.conditions.append(&mut other.conditions); + + self + } + + // parsers + fn id_list(input: ArgList) -> IResult { fn to_filterarg(input: Vec) -> Result { Ok(FilterArg::IdList(input)) @@ -112,6 +108,8 @@ impl Filter { map_res(arg_matching(minus_tag), to_filterarg)(input) } + // usage + pub(super) fn get_usage(u: &mut usage::Usage) { u.filters.push(usage::Filter { syntax: "TASKID[,TASKID,..]", @@ -160,8 +158,7 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![TaskId::WorkingSetId(1)]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(1)])], } ); } @@ -173,12 +170,11 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::WorkingSetId(1), TaskId::WorkingSetId(2), TaskId::WorkingSetId(3), - ]), - ..Default::default() + ])], } ); } @@ -190,13 +186,12 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::WorkingSetId(1), TaskId::WorkingSetId(2), TaskId::WorkingSetId(3), TaskId::WorkingSetId(4), - ]), - ..Default::default() + ])], } ); } @@ -208,11 +203,10 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::WorkingSetId(1), TaskId::PartialUuid(s!("abcd1234")), - ]), - ..Default::default() + ])], } ); } @@ -224,12 +218,66 @@ mod test { assert_eq!( filter, Filter { - universe: Universe::IdList(vec![TaskId::WorkingSetId(1),]), conditions: vec![ + Condition::IdList(vec![TaskId::WorkingSetId(1),]), Condition::HasTag("yes".into()), Condition::NoTag("no".into()), ], - ..Default::default() + } + ); + } + + #[test] + fn intersect_idlist_idlist() { + let left = Filter::parse(argv!["1,2", "+yes"]).unwrap().1; + let right = Filter::parse(argv!["2,3", "+no"]).unwrap().1; + let both = left.intersect(right); + assert_eq!( + both, + Filter { + conditions: vec![ + // from first filter + Condition::IdList(vec![TaskId::WorkingSetId(1), TaskId::WorkingSetId(2),]), + Condition::HasTag("yes".into()), + // from second filter + Condition::IdList(vec![TaskId::WorkingSetId(2), TaskId::WorkingSetId(3)]), + Condition::HasTag("no".into()), + ], + } + ); + } + + #[test] + fn intersect_idlist_alltasks() { + let left = Filter::parse(argv!["1,2", "+yes"]).unwrap().1; + let right = Filter::parse(argv!["+no"]).unwrap().1; + let both = left.intersect(right); + assert_eq!( + both, + Filter { + conditions: vec![ + // from first filter + Condition::IdList(vec![TaskId::WorkingSetId(1), TaskId::WorkingSetId(2),]), + Condition::HasTag("yes".into()), + // from second filter + Condition::HasTag("no".into()), + ], + } + ); + } + + #[test] + fn intersect_alltasks_alltasks() { + let left = Filter::parse(argv!["+yes"]).unwrap().1; + let right = Filter::parse(argv!["+no"]).unwrap().1; + let both = left.intersect(right); + assert_eq!( + both, + Filter { + conditions: vec![ + Condition::HasTag("yes".into()), + Condition::HasTag("no".into()), + ], } ); } diff --git a/cli/src/argparse/mod.rs b/cli/src/argparse/mod.rs index 0bf656a8f..ab54326f1 100644 --- a/cli/src/argparse/mod.rs +++ b/cli/src/argparse/mod.rs @@ -19,7 +19,7 @@ mod subcommand; pub(crate) use args::TaskId; pub(crate) use command::Command; -pub(crate) use filter::{Condition, Filter, Universe}; +pub(crate) use filter::{Condition, Filter}; pub(crate) use modification::{DescriptionMod, Modification}; pub(crate) use subcommand::Subcommand; diff --git a/cli/src/argparse/subcommand.rs b/cli/src/argparse/subcommand.rs index 0caf17ebd..1d7ddc2ba 100644 --- a/cli/src/argparse/subcommand.rs +++ b/cli/src/argparse/subcommand.rs @@ -383,7 +383,7 @@ impl Sync { #[cfg(test)] mod test { use super::*; - use crate::argparse::Universe; + use crate::argparse::Condition; const EMPTY: Vec<&str> = vec![]; @@ -459,8 +459,7 @@ mod test { fn test_modify_description_multi() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Set(s!("foo bar")), @@ -477,8 +476,7 @@ mod test { fn test_append() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Append(s!("foo bar")), @@ -495,8 +493,7 @@ mod test { fn test_prepend() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Prepend(s!("foo bar")), @@ -513,8 +510,7 @@ mod test { fn test_done() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { status: Some(Status::Completed), @@ -531,8 +527,7 @@ mod test { fn test_done_modify() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Set(s!("now-finished")), @@ -550,8 +545,7 @@ mod test { fn test_start() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { active: Some(true), @@ -568,8 +562,7 @@ mod test { fn test_start_modify() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { active: Some(true), @@ -587,8 +580,7 @@ mod test { fn test_stop() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { active: Some(false), @@ -605,8 +597,7 @@ mod test { fn test_stop_modify() { let subcommand = Subcommand::Modify { filter: Filter { - universe: Universe::for_ids(vec![123]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(123)])], }, modification: Modification { description: DescriptionMod::Set(s!("mod")), @@ -636,8 +627,10 @@ mod test { fn test_report_filter_before() { let subcommand = Subcommand::Report { filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, report_name: "foo".to_owned(), }; @@ -651,8 +644,10 @@ mod test { fn test_report_filter_after() { let subcommand = Subcommand::Report { filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, report_name: "foo".to_owned(), }; @@ -666,8 +661,10 @@ mod test { fn test_report_filter_next() { let subcommand = Subcommand::Report { filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, report_name: "next".to_owned(), }; @@ -696,8 +693,10 @@ mod test { let subcommand = Subcommand::Info { debug: false, filter: Filter { - universe: Universe::for_ids(vec![12, 13]), - ..Default::default() + conditions: vec![Condition::IdList(vec![ + TaskId::WorkingSetId(12), + TaskId::WorkingSetId(13), + ])], }, }; assert_eq!( @@ -711,8 +710,7 @@ mod test { let subcommand = Subcommand::Info { debug: true, filter: Filter { - universe: Universe::for_ids(vec![12]), - ..Default::default() + conditions: vec![Condition::IdList(vec![TaskId::WorkingSetId(12)])], }, }; assert_eq!( diff --git a/cli/src/invocation/filter.rs b/cli/src/invocation/filter.rs index f87263270..34fc1b1e1 100644 --- a/cli/src/invocation/filter.rs +++ b/cli/src/invocation/filter.rs @@ -1,10 +1,10 @@ -use crate::argparse::{Condition, Filter, TaskId, Universe}; +use crate::argparse::{Condition, Filter, TaskId}; use failure::Fallible; use std::collections::HashSet; use std::convert::TryInto; -use taskchampion::{Replica, Tag, Task}; +use taskchampion::{Replica, Status, Tag, Task, Uuid}; -fn match_task(filter: &Filter, task: &Task) -> bool { +fn match_task(filter: &Filter, task: &Task, uuid: Uuid, working_set_id: Option) -> bool { for cond in &filter.conditions { match cond { Condition::HasTag(ref tag) => { @@ -21,11 +21,85 @@ fn match_task(filter: &Filter, task: &Task) -> bool { return false; } } + Condition::Status(status) => { + if task.get_status() != *status { + return false; + } + } + Condition::IdList(ids) => { + let uuid_str = uuid.to_string(); + let mut found = false; + for id in ids { + if match id { + TaskId::WorkingSetId(i) => Some(*i) == working_set_id, + TaskId::PartialUuid(partial) => uuid_str.starts_with(partial), + TaskId::Uuid(i) => *i == uuid, + } { + found = true; + break; + } + } + if !found { + return false; + } + } } } true } +// the universe of tasks we must consider +enum Universe { + /// Scan all the tasks + AllTasks, + /// Scan the working set (for pending tasks) + WorkingSet, + /// Scan an explicit set of tasks, "Absolute" meaning either full UUID or a working set + /// index + AbsoluteIdList(Vec), +} + +/// Determine the universe for the given filter; avoiding the need to scan all tasks in most cases. +fn universe_for_filter(filter: &Filter) -> Universe { + /// If there is a condition with Status::Pending, return true + fn has_pending_condition(filter: &Filter) -> bool { + filter + .conditions + .iter() + .any(|cond| matches!(cond, Condition::Status(Status::Pending))) + } + + /// If there is a condition with an IdList containing no partial UUIDs, + /// return that. + fn absolute_id_list_condition(filter: &Filter) -> Option> { + filter + .conditions + .iter() + .find(|cond| { + if let Condition::IdList(ids) = cond { + !ids.iter().any(|id| matches!(id, TaskId::PartialUuid(_))) + } else { + false + } + }) + .map(|cond| { + if let Condition::IdList(ids) = cond { + ids.to_vec() + } else { + unreachable!() // any condition found above must be an IdList(_) + } + }) + } + + if let Some(ids) = absolute_id_list_condition(filter) { + Universe::AbsoluteIdList(ids) + } else if has_pending_condition(filter) { + Universe::WorkingSet + } else { + Universe::AllTasks + } +} + /// Return the tasks matching the given filter. This will return each matching /// task once, even if the user specified the same task multiple times on the /// command line. @@ -35,38 +109,14 @@ pub(super) fn filtered_tasks( ) -> Fallible> { let mut res = vec![]; - fn is_partial_uuid(taskid: &TaskId) -> bool { - matches!(taskid, TaskId::PartialUuid(_)) - } + log::debug!("Applying filter {:?}", filter); // We will enumerate the universe of tasks for this filter, checking // each resulting task with match_task - match filter.universe { + match universe_for_filter(filter) { // A list of IDs, but some are partial so we need to iterate over // all tasks and pattern-match their Uuids - Universe::IdList(ref ids) if ids.iter().any(is_partial_uuid) => { - log::debug!("Scanning entire task database due to partial UUIDs in the filter"); - 'task: for (uuid, task) in replica.all_tasks()?.drain() { - for id in ids { - let in_universe = match id { - TaskId::WorkingSetId(id) => { - // NOTE: (#108) this results in many reads of the working set; it - // may be better to cache this information here or in the Replica. - replica.get_working_set_index(&uuid)? == Some(*id) - } - TaskId::PartialUuid(prefix) => uuid.to_string().starts_with(prefix), - TaskId::Uuid(id) => id == &uuid, - }; - if in_universe && match_task(filter, &task) { - res.push(task); - continue 'task; - } - } - } - } - - // A list of full IDs, which we can fetch directly - Universe::IdList(ref ids) => { + Universe::AbsoluteIdList(ref ids) => { log::debug!("Scanning only the tasks specified in the filter"); // this is the only case where we might accidentally return the same task // several times, so we must track the seen tasks. @@ -74,7 +124,7 @@ pub(super) fn filtered_tasks( for id in ids { let task = match id { TaskId::WorkingSetId(id) => replica.get_working_set_task(*id)?, - TaskId::PartialUuid(_) => unreachable!(), // handled above + TaskId::PartialUuid(_) => unreachable!(), // not present in absolute id list TaskId::Uuid(id) => replica.get_task(id)?, }; @@ -86,7 +136,9 @@ pub(super) fn filtered_tasks( } seen.insert(uuid); - if match_task(filter, &task) { + let working_set_id = replica.get_working_set_index(&uuid)?; + + if match_task(filter, &task, uuid, working_set_id) { res.push(task); } } @@ -96,19 +148,20 @@ pub(super) fn filtered_tasks( // All tasks -- iterate over the full set Universe::AllTasks => { log::debug!("Scanning all tasks in the task database"); - for (_, task) in replica.all_tasks()?.drain() { - if match_task(filter, &task) { + for (uuid, task) in replica.all_tasks()?.drain() { + // Yikes, slow! https://github.com/djmitche/taskchampion/issues/108 + let working_set_id = replica.get_working_set_index(&uuid)?; + if match_task(filter, &task, uuid, working_set_id) { res.push(task); } } } - - // Pending tasks -- just scan the working set - Universe::PendingTasks => { + Universe::WorkingSet => { log::debug!("Scanning only the working set (pending tasks)"); - for task in replica.working_set()?.drain(..) { + for (i, task) in replica.working_set()?.drain(..).enumerate() { if let Some(task) = task { - if match_task(filter, &task) { + let uuid = *task.get_uuid(); + if match_task(filter, &task, uuid, Some(i)) { res.push(task); } } @@ -136,12 +189,11 @@ mod test { let t1uuid = *t1.get_uuid(); let filter = Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::Uuid(t1uuid), // A TaskId::WorkingSetId(1), // A (again, dups filtered) TaskId::Uuid(*t2.get_uuid()), // B - ]), - ..Default::default() + ])], }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() @@ -165,12 +217,11 @@ mod test { let t2partial = t2uuid[..13].to_owned(); let filter = Filter { - universe: Universe::IdList(vec![ + conditions: vec![Condition::IdList(vec![ TaskId::Uuid(t1uuid), // A TaskId::WorkingSetId(1), // A (again, dups filtered) TaskId::PartialUuid(t2partial), // B - ]), - ..Default::default() + ])], }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() @@ -189,10 +240,7 @@ mod test { replica.new_task(Status::Deleted, s!("C")).unwrap(); replica.gc().unwrap(); - let filter = Filter { - universe: Universe::AllTasks, - ..Default::default() - }; + let filter = Filter { conditions: vec![] }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() .map(|t| t.get_description().to_owned()) @@ -224,9 +272,7 @@ mod test { // look for just "yes" (A and B) let filter = Filter { - universe: Universe::AllTasks, conditions: vec![Condition::HasTag(s!("yes"))], - ..Default::default() }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter)? .map(|t| t.get_description().to_owned()) @@ -236,9 +282,7 @@ mod test { // look for tags without "no" (A, D) let filter = Filter { - universe: Universe::AllTasks, conditions: vec![Condition::NoTag(s!("no"))], - ..Default::default() }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter)? .map(|t| t.get_description().to_owned()) @@ -248,9 +292,7 @@ mod test { // look for tags with "yes" and "no" (B) let filter = Filter { - universe: Universe::AllTasks, conditions: vec![Condition::HasTag(s!("yes")), Condition::HasTag(s!("no"))], - ..Default::default() }; let filtered: Vec<_> = filtered_tasks(&mut replica, &filter)? .map(|t| t.get_description().to_owned()) @@ -270,8 +312,7 @@ mod test { replica.gc().unwrap(); let filter = Filter { - universe: Universe::PendingTasks, - ..Default::default() + conditions: vec![Condition::Status(Status::Pending)], }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) .unwrap() diff --git a/cli/src/invocation/report.rs b/cli/src/invocation/report.rs index 0ed793f7d..82cc67bba 100644 --- a/cli/src/invocation/report.rs +++ b/cli/src/invocation/report.rs @@ -1,11 +1,11 @@ -use crate::argparse::Filter; +use crate::argparse::{Condition, Filter}; use crate::invocation::filtered_tasks; use crate::report::{Column, Property, Report, Sort, SortBy}; use crate::table; use failure::{bail, Fallible}; use prettytable::{Row, Table}; use std::cmp::Ordering; -use taskchampion::{Replica, Task, Uuid}; +use taskchampion::{Replica, Status, Task, Uuid}; use termcolor::WriteColor; // pending #123, this is a non-fallible way of looking up a task's working set index @@ -125,24 +125,26 @@ fn get_report(report_name: String, filter: Filter) -> Fallible { ascending: false, sort_by: SortBy::Uuid, }]; - use crate::argparse::Universe; - Ok(match report_name.as_ref() { + let mut report = match report_name.as_ref() { "list" => Report { columns, sort, - filter, + filter: Default::default(), }, "next" => Report { columns, sort, - // TODO: merge invocation filter and report filter filter: Filter { - universe: Universe::PendingTasks, - ..Default::default() + conditions: vec![Condition::Status(Status::Pending)], }, }, _ => bail!("Unknown report {:?}", report_name), - }) + }; + + // intersect the report's filter with the user-supplied filter + report.filter = report.filter.intersect(filter); + + Ok(report) } pub(super) fn display_report(