diff --git a/CHANGELOG.md b/CHANGELOG.md index 533da16b1..23e7f5514 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fixed the match nodes returned by `CorpusStorage::find`, + `CorpusStorage::find_extra` and `aql::execute_query_on_graph` for queries + that have an optional node in an alternative other than the first one. + ## [4.3.0] - 2026-10-15 ### Added diff --git a/graphannis/src/annis/db/aql/conjunction.rs b/graphannis/src/annis/db/aql/conjunction.rs index dd3d0ac73..1c9f1aa75 100644 --- a/graphannis/src/annis/db/aql/conjunction.rs +++ b/graphannis/src/annis/db/aql/conjunction.rs @@ -387,20 +387,19 @@ impl Conjunction { })) } - /// Return true if the given query node variable is supposed to be included - /// in a match output. This is not the case if it is an optional node or a - /// legacy `meta::` query reference. - pub fn is_included_in_output(&self, variable: &str) -> bool { - self.include_in_output.contains(variable) + /// Return true if the query node with the given node number inside this + /// conjunction is supposed to be included in a match output. This is not + /// the case if it is an optional node or a legacy `meta::` query reference. + pub fn is_included_in_output(&self, node_nr: usize) -> bool { + self.nodes + .get(node_nr) + .is_some_and(|node| self.include_in_output.contains(&node.var)) } - /// Return the variable name for a node number. The node number is the - /// position of a node description in the query. In case of a query with - /// multiple alternatives, this refers to the node number of the whole query and - /// not only the one inside the conjunction. + /// Return the variable name for the query node with the given node number + /// inside this conjunction. pub fn get_variable_by_node_nr(&self, node_nr: usize) -> Option { - let pos = node_nr.checked_sub(self.var_idx_offset)?; - self.nodes.get(pos).map(|spec| spec.var.clone()) + self.nodes.get(node_nr).map(|spec| spec.var.clone()) } pub fn resolve_variable( diff --git a/graphannis/src/annis/db/aql/disjunction.rs b/graphannis/src/annis/db/aql/disjunction.rs index b9613bfe3..da427fac6 100644 --- a/graphannis/src/annis/db/aql/disjunction.rs +++ b/graphannis/src/annis/db/aql/disjunction.rs @@ -36,23 +36,12 @@ impl Disjunction { None } - /// Return the variable name for a node number. The node number is the - /// position of an AQL query node in the disjunction. - pub(crate) fn get_variable_by_node_nr(&self, node_nr: usize) -> Option { - for alt in &self.alternatives { - if let Some(var) = alt.get_variable_by_node_nr(node_nr) { - return Some(var); - } - } - None - } - - pub(crate) fn is_included_in_output(&self, variable: &str) -> bool { - for alt in &self.alternatives { - if alt.is_included_in_output(variable) { - return true; - } - } - false + /// Return true if the query node with the given node number inside the given + /// alternative is supposed to be included in a match output. The node number + /// is the one inside the alternative and not the one in the whole query. + pub(crate) fn is_included_in_output(&self, alternative: usize, node_nr: usize) -> bool { + self.alternatives + .get(alternative) + .is_some_and(|alt| alt.is_included_in_output(node_nr)) } } diff --git a/graphannis/src/annis/db/aql/mod.rs b/graphannis/src/annis/db/aql/mod.rs index fc2522e8e..55ce591f3 100644 --- a/graphannis/src/annis/db/aql/mod.rs +++ b/graphannis/src/annis/db/aql/mod.rs @@ -79,15 +79,13 @@ pub fn execute_query_on_graph<'a>( let config = Config { use_parallel_joins }; let it = ExecutionPlan::from_disjunction(query, graph, &config, timeout)? - .map_ok(|mg| { + .matches_with_alternative() + .map_ok(|(alternative, mg)| { // Rewrite match group to only include nodes that are included in the query // check if query node actually should be included let mut new_match_group = MatchGroup::with_capacity(mg.len()); for (node_nr, m) in mg.into_iter().enumerate() { - let include_in_output = query - .get_variable_by_node_nr(node_nr) - .is_some_and(|var| query.is_included_in_output(&var)); - if include_in_output { + if query.is_included_in_output(alternative, node_nr) { new_match_group.push(m); } } @@ -522,7 +520,7 @@ pub fn parse(query_as_aql: &str, quirks_mode: bool) -> Result { if quirks_mode { // apply the meta constraints to first node of all conjunctions - let first_node_var = mapped.get_variable_by_node_nr(var_idx_offset); + let first_node_var = mapped.get_variable_by_node_nr(0); add_legacy_metadata_constraints( &mut mapped, legacy_meta_search.clone(), @@ -677,7 +675,7 @@ mod tests { #[test] fn query_on_annotation_graph() { let cargo_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - let input_file = File::open(&cargo_dir.join("tests/SaltSampleCorpus.graphml")).unwrap(); + let input_file = File::open(cargo_dir.join("tests/SaltSampleCorpus.graphml")).unwrap(); let (graph, _config_str): (AnnotationGraph, _) = graphannis_core::graph::serialization::graphml::import(input_file, false, |_status| {}) .unwrap(); @@ -690,7 +688,7 @@ mod tests { #[test] fn query_on_annotation_graph_with_negation() { let cargo_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - let input_file = File::open(&cargo_dir.join("tests/SaltSampleCorpus.graphml")).unwrap(); + let input_file = File::open(cargo_dir.join("tests/SaltSampleCorpus.graphml")).unwrap(); let (graph, _config_str): (AnnotationGraph, _) = graphannis_core::graph::serialization::graphml::import(input_file, false, |_status| {}) .unwrap(); @@ -707,4 +705,29 @@ mod tests { assert_eq!(1, matches[2].len()); assert_eq!(1, matches[3].len()); } + + #[test] + fn query_on_annotation_graph_with_optional_node_in_later_alternative() { + let cargo_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let input_file = File::open(cargo_dir.join("tests/SaltSampleCorpus.graphml")).unwrap(); + let (graph, _config_str): (AnnotationGraph, _) = + graphannis_core::graph::serialization::graphml::import(input_file, false, |_status| {}) + .unwrap(); + + let query = parse("tok | tok? !. lemma", false).unwrap(); + let it = execute_query_on_graph(&graph, &query, true, None).unwrap(); + + let matches: Result> = it.collect(); + let matches = matches.unwrap(); + // 44 matches from the first alternative and 4 from the second one + assert_eq!(48, matches.len()); + // The optional node must not be included in any of the matches, only + // the lemma after it + assert!(matches.iter().all(|m| m.len() == 1)); + let lemma_matches = matches + .iter() + .filter(|m| m[0].anno_key.name == "lemma") + .count(); + assert_eq!(4, lemma_matches); + } } diff --git a/graphannis/src/annis/db/corpusstorage.rs b/graphannis/src/annis/db/corpusstorage.rs index d6d75fc1e..97b19f763 100644 --- a/graphannis/src/annis/db/corpusstorage.rs +++ b/graphannis/src/annis/db/corpusstorage.rs @@ -1954,12 +1954,7 @@ impl CorpusStorage { for (node_nr, singlematch) in m.iter().enumerate() { // check if query node actually should be included - let include_in_output = prep - .query - .get_variable_by_node_nr(node_nr) - .is_some_and(|var| prep.query.is_included_in_output(&var)); - - if include_in_output { + if prep.query.is_included_in_output(alternative, node_nr) { if any_nodes_added { match_desc.push(' '); } diff --git a/graphannis/src/annis/db/corpusstorage/tests.rs b/graphannis/src/annis/db/corpusstorage/tests.rs index 7122a85ce..31fadec36 100644 --- a/graphannis/src/annis/db/corpusstorage/tests.rs +++ b/graphannis/src/annis/db/corpusstorage/tests.rs @@ -11,6 +11,7 @@ use crate::annis::db::corpusstorage::{CacheEntry, get_read_or_error}; use crate::annis::db::example_generator::create_token_node; use crate::annis::db::{aql::model::AnnotationComponentType, example_generator}; use crate::annis::errors::GraphAnnisError; +use crate::annis::util::node_names_from_match; use crate::corpusstorage::{ImportFormat, QueryLanguage, ResultOrder}; use crate::errors::Result; use crate::update::{GraphUpdate, UpdateEvent}; @@ -1858,6 +1859,42 @@ fn optional_node_first_in_query() { ); } +#[test] +fn optional_node_first_in_later_alternative() { + let tmp = tempfile::tempdir().unwrap(); + let cargo_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + + let cs = CorpusStorage::with_auto_cache_size(tmp.path(), true).unwrap(); + cs.import_from_fs( + &cargo_dir.join("tests/SaltSampleCorpus.graphml"), + ImportFormat::GraphML, + Some("SaltSampleCorpus".into()), + false, + true, + |_| {}, + ) + .unwrap(); + + let query = SearchQuery { + corpus_names: &["SaltSampleCorpus"], + query: "tok | tok? !. lemma", + query_language: QueryLanguage::AQL, + timeout: None, + }; + let results = cs.find(query, 0, None, ResultOrder::Normal).unwrap(); + + // 44 matches from the first alternative and 4 from the second one + assert_eq!(48, results.len()); + // The optional node must not be included in any of the matches, only the + // lemma after it + assert!(results.iter().all(|m| node_names_from_match(m).len() == 1)); + let lemma_matches = results + .iter() + .filter(|m| m.starts_with("salt::lemma::")) + .count(); + assert_eq!(4, lemma_matches); +} + #[test] #[serial] fn unload_corpus() {