Merge pull request #299 from korpling/feature/optional-node-check-tests

Add fix for handling of optional nodes that are not at the end of the query

Author: thomaskrause

CHANGELOG.md

@@ -5,6 +5,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- When optional nodes where located not at the end but somewhere in between the
+  query, the output of the `find` query could include the wrong node ID.
+
 ## [3.3.0] - 2024-05-27
 
 ### Changed

graphannis/src/annis/db/aql/conjunction.rs

@@ -292,14 +292,15 @@ impl Conjunction {
         } else {
             (idx + 1).to_string()
         };
+        self.variables.insert(variable.clone(), self.nodes.len());
+
         self.nodes.push(NodeSearchSpecEntry {
             var: variable.clone(),
             spec: node,
             optional,
             location: location.clone(),
         });
 
-        self.variables.insert(variable.clone(), idx);
         if included_in_output && !optional {
             self.include_in_output.insert(variable.clone());
         }
@@ -315,9 +316,11 @@ impl Conjunction {
         var: &str,
         location: Option<LineColumnRange>,
     ) -> Result<()> {
-        if let Some(idx) = self.variables.get(var) {
-            self.unary_operators
-                .push(UnaryOperatorSpecEntry { op, idx: *idx });
+        if let Some(local_idx) = self.variables.get(var) {
+            self.unary_operators.push(UnaryOperatorSpecEntry {
+                op,
+                idx: *local_idx,
+            });
             Ok(())
         } else {
             Err(GraphAnnisError::AQLSemanticError(AQLError {
@@ -370,34 +373,28 @@ impl Conjunction {
         location: Option<LineColumnRange>,
     ) -> Result<usize> {
         if let Some(pos) = self.variables.get(variable) {
-            return Ok(*pos);
+            return Ok(pos + self.var_idx_offset);
         }
         Err(GraphAnnisError::AQLSemanticError(AQLError {
             desc: format!("Operand \"#{}\" not found", variable),
             location,
         }))
     }
 
+    /// 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 the variable name for a given position in the match output list.
-    ///
-    /// Optional nodes that are not part of the output are ignored. If there are
-    /// no optional nodes, this corresponds to the index of the node in the
-    /// query.
-    pub fn get_variable_by_pos(&self, pos: usize) -> Option<String> {
-        let mut output_pos = 0;
-        for n in self.nodes.iter() {
-            if self.is_included_in_output(&n.var) {
-                if output_pos == pos {
-                    return Some(n.var.clone());
-                }
-                output_pos += 1;
-            }
-        }
-        None
+    /// Return the variable name for a node number. The node number is the
+    /// position of an 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 conjunction.
+    pub fn get_variable_by_node_nr(&self, node_nr: usize) -> Option<String> {
+        let pos = node_nr.checked_sub(self.var_idx_offset)?;
+        self.nodes.get(pos).map(|spec| spec.var.clone())
     }
 
     pub fn resolve_variable(
@@ -725,7 +722,7 @@ impl Conjunction {
             .ok_or(GraphAnnisError::NoComponentForNode(spec_idx_right + 1))?);
 
         // get the original execution node
-        let exec_left: Box<dyn ExecutionNode<Item = Result<MatchGroup>> + 'a> = component2exec
+        let exec_left = component2exec
             .remove(&component_left)
             .ok_or(GraphAnnisError::NoExecutionNode(component_left))?;
 
@@ -821,7 +818,7 @@ impl Conjunction {
 
         // 2. add unary operators as filter to the existing node search
         for op_spec_entry in self.unary_operators.iter() {
-            let child_exec: Box<dyn ExecutionNode<Item = Result<MatchGroup>> + 'a> = component2exec
+            let child_exec = component2exec
                 .remove(&op_spec_entry.idx)
                 .ok_or(GraphAnnisError::NoExecutionNode(op_spec_entry.idx))?;
 

graphannis/src/annis/db/aql/disjunction.rs

@@ -36,17 +36,23 @@ impl Disjunction {
         None
     }
 
-    /// Return the variable name for a given position in the match output list.
-    ///
-    /// Optional nodes that are not part of the output are ignored. If there are
-    /// no optional nodes, this corresponds to the index of the node in the
-    /// query.
-    pub(crate) fn get_variable_by_pos(&self, pos: usize) -> Option<String> {
+    /// 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<String> {
         for alt in &self.alternatives {
-            if let Some(var) = alt.get_variable_by_pos(pos) {
+            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
+    }
 }

graphannis/src/annis/db/aql/mod.rs

@@ -359,31 +359,30 @@ fn add_node_specs_by_start(
 fn add_legacy_metadata_constraints(
     q: &mut Conjunction,
     legacy_meta_search: Vec<(NodeSearchSpec, ast::Pos)>,
-    first_node_pos: Option<String>,
+    first_node_var: Option<String>,
 ) -> Result<()> {
     {
-        let mut first_meta_idx: Option<String> = None;
-        // TODO: add warning to the user not to use this construct anymore
+        let mut first_meta_var: Option<String> = None;
         for (spec, _pos) in legacy_meta_search {
             // add an artificial node that describes the document/corpus node
-            let meta_node_idx = q.add_node_from_query(spec, None, None, false, false);
-            if let Some(first_meta_idx) = first_meta_idx.clone() {
+            let meta_node_var = q.add_node_from_query(spec, None, None, false, false);
+            if let Some(first_meta_var) = first_meta_var.clone() {
                 // avoid nested loops by joining additional meta nodes with a "identical node"
                 q.add_operator(
                     Arc::new(IdenticalNodeSpec {}),
-                    &first_meta_idx,
-                    &meta_node_idx,
+                    &first_meta_var,
+                    &meta_node_var,
                     true,
                 )?;
-            } else if let Some(first_node_pos) = first_node_pos.clone() {
-                first_meta_idx = Some(meta_node_idx.clone());
+            } else if let Some(first_node_var) = first_node_var.clone() {
+                first_meta_var = Some(meta_node_var.clone());
                 // add a special join to the first node of the query
                 q.add_operator(
                     Arc::new(PartOfSubCorpusSpec {
                         dist: RangeSpec::Unbound,
                     }),
-                    &first_node_pos,
-                    &meta_node_idx,
+                    &first_node_var,
+                    &meta_node_var,
                     true,
                 )?;
                 // Also make sure the matched node is actually a document
@@ -402,7 +401,7 @@ fn add_legacy_metadata_constraints(
                 );
                 q.add_operator(
                     Arc::new(IdenticalNodeSpec {}),
-                    &meta_node_idx,
+                    &meta_node_var,
                     &doc_anno_idx,
                     true,
                 )?;
@@ -501,12 +500,12 @@ pub fn parse(query_as_aql: &str, quirks_mode: bool) -> Result<Disjunction> {
                 let mut mapped = map_conjunction(c, &offsets, var_idx_offset, quirks_mode)?;
 
                 if quirks_mode {
-                    // apply the meta constraints from all conjunctions to conjunctions
-                    let first_node_pos = mapped.get_variable_by_pos(0);
+                    // apply the meta constraints to first node of all conjunctions
+                    let first_node_var = mapped.get_variable_by_node_nr(0);
                     add_legacy_metadata_constraints(
                         &mut mapped,
                         legacy_meta_search.clone(),
-                        first_node_pos,
+                        first_node_var,
                     )?;
                 }
                 var_idx_offset += mapped.num_of_nodes();

graphannis/src/annis/db/corpusstorage.rs

@@ -1908,14 +1908,20 @@ impl CorpusStorage {
             let m = m?;
             let mut match_desc = String::new();
 
-            for (i, singlematch) in m.iter().enumerate() {
+            let mut any_nodes_added = false;
+
+            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_pos(i).is_some();
+                let include_in_output = prep
+                    .query
+                    .get_variable_by_node_nr(node_nr)
+                    .map_or(false, |var| prep.query.is_included_in_output(&var));
 
                 if include_in_output {
-                    if i > 0 {
+                    if any_nodes_added {
                         match_desc.push(' ');
                     }
+                    any_nodes_added = true;
 
                     let singlematch_anno_key = &singlematch.anno_key;
                     if singlematch_anno_key.ns != ANNIS_NS || singlematch_anno_key.name != NODE_TYPE

graphannis/src/annis/db/plan.rs

@@ -91,11 +91,11 @@ impl<'a> ExecutionPlan<'a> {
         })
     }
 
+    /// Re-orders the match vector from the top execution node to match the
+    /// requested query node order. If query nodes are not part of the result,
+    /// they are still included in the vector but you can not use the node ID at
+    /// this position.
     fn reorder_match(&self, tmp: MatchGroup) -> MatchGroup {
-        if tmp.len() <= 1 {
-            // nothing to reorder
-            return tmp;
-        }
         if let Some(ref inverse_node_pos) = self.inverse_node_pos[self.current_plan] {
             // re-order the matched nodes by the original node position of the query
             let mut result = MatchGroup::new();

graphannis/tests/searchtest.rs

@@ -267,6 +267,54 @@ fn meta_node_output_standard() {
     );
 }
 
+#[ignore]
+#[test]
+fn exclude_optional_node_in_between() {
+    let cs = CORPUS_STORAGE.as_ref().unwrap();
+
+    let q = SearchQuery {
+        corpus_names: &["GUM"],
+        query: r#"entity="person" !_o_ q? & infstat="giv" & #1 _r_ #3"#,
+        query_language: QueryLanguage::AQL,
+        timeout: None,
+    };
+    let result = cs
+        .find(
+            q.clone(),
+            0,
+            Some(1),
+            graphannis::corpusstorage::ResultOrder::Normal,
+        )
+        .unwrap();
+
+    // Only node #1 and #3 should be part of the output
+    assert_eq!(vec!["ref::entity::GUM/GUM_interview_ants#referent_291 ref::infstat::GUM/GUM_interview_ants#referent_321"], result);
+}
+
+#[ignore]
+#[test]
+fn exclude_optional_node_at_end() {
+    let cs = CORPUS_STORAGE.as_ref().unwrap();
+
+    let q = SearchQuery {
+        corpus_names: &["GUM"],
+        query: r#"entity="person" _r_ infstat="giv" &  q? & #1 !_o_ #3"#,
+        query_language: QueryLanguage::AQL,
+        timeout: None,
+    };
+    let result = cs
+        .find(
+            q.clone(),
+            0,
+            Some(1),
+            graphannis::corpusstorage::ResultOrder::Normal,
+        )
+        .unwrap();
+
+    // Only node #1 and #2 should be part of the output
+    assert_eq!(vec!["ref::entity::GUM/GUM_interview_ants#referent_291 ref::infstat::GUM/GUM_interview_ants#referent_321"], result);
+}
+
 #[ignore]
 #[test]
 fn token_search_loads_components_for_leaf_filter() {

graphannis/tests/searchtest_queries.csv

@@ -47,6 +47,9 @@ NonExistingRight,"quote & /""/? & #1 !_r_ #2",GUM,19
 NonExistingRightInv,"quote & /""/? & #2 !_r_ #1",GUM,19
 InvalidNonExisting,"tok !. tok? !_o_  node?",GUM,0
 NotDocument,"tok @* doc!=""maz-11299""",pcc2.1,34709
+OptionalQueryNodeAtEnd,"entity=""person"" _r_ infstat=""giv"" &  q? & #1 !_o_ #3",GUM,2404
+OptionalQueryNodeInBetween,"entity=""person"" !_o_ q? & infstat=""giv"" & #1 _r_ #3",GUM,2404
+OptionalQueryAlternative,"(entity=""person"" !_o_ q? & infstat=""giv"" & #1 _r_ #3) | (entity=""organization"" !_o_ q? & infstat=""new"" & #4 _r_ #6)",GUM,2702
 StructureInclusionSeed,"cat=""S"" _i_ cat=""AP""",pcc2.1,726
 IsConnectedRange,"""Jugendlichen"" .3,10 ""Musikcafé""",pcc2.1,1
 Regex,cat=/.P/ >* /A.*/,pcc2.1,1130