Repository navigation
Fix match output for optional nodes in queries with multiple alternatives - #348
Conversation
thomaskrause
left a comment
thomaskrause
left a comment
There was a problem hiding this comment.
Thanks for the PR! It looks good to me and simplifies the interfaces of Conjunction and Disjunction which I do like a lot. The responsibility to differentiate between "global" and "conjunction local" offsets is moved to the caller, which with your newly added interfaces has a much better chance to select the correct one without hacks like the offset variable.
I made an internal TODO for myself to refactor the things mentioned in #338 (comment) but it seems until then this is a good solution.
| 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()) | ||
| self.nodes.get(node_nr).map(|spec| spec.var.clone()) |
There was a problem hiding this comment.
Since this is only used at one other part of the code (where we need to find the variable name for the first node of the conjunction to add a meta constraint to it), this does indeed simplify the part where it is called. Good catch!
Hey there,
I found another bug (introduced in #299) caused by the confusion between global and alternative-local node indices, similar to #338: In alternatives other than the first, the check whether to include a node in the output uses the wrong index, so when there's an optional node before a non-optional node in such an alternative, the wrong nodes get included. E.g. for
tok | tok? !. lemma, an unrelated node gets included instead of thelemmanode.This PR doesn't include the refactoring mentioned in #338 (comment). Instead, I'm just passing the alternative index (which is available where we need it since #342) along with the alternative-local node index. I also considered using variable names instead of node indices to avoid the confusion altogether, but this would have been too big of a change for this bugfix - it's probably better addressed in a dedicated refactoring.