Merge pull request #1296 from VisLab/enhance_search

Implemented Phase II object-based search enhancements #1293

Author: VisLab

examples/validate_bids_dataset_with_libraries.ipynb

@@ -44,7 +44,7 @@
     "\n",
     "## Now validate URLs\n",
     "print(\"\\nNow validating with schema URLs.\")\n",
-    "base_version = \"8.3.0\"\n",
+    "base_version = \"8.4.0\"\n",
     "library1_url = (\n",
     "    \"https://raw.githubusercontent.com/hed-standard/hed-schemas/main/\"\n",
     "    + \"library_schemas/score/hedxml/HED_score_2.0.0.xml\"\n",

hed/models/hed_group.py

@@ -434,7 +434,12 @@ def __eq__(self, other):
 
     def find_tags(self, search_tags, recursive=False, include_groups=2) -> list:
         """Find the base tags and their containing groups.
-        This searches by short_base_tag, ignoring any ancestors or extensions/values.
+
+        Comparison property: ``short_base_tag`` (schema short name without any extension or value).
+        Rationale: callers pass bare tag names such as ``"Event"`` or ``"Def"`` and must
+        match regardless of any extension or value the tag carries in the source string.
+        Using ``short_base_tag`` strips the extension/value so ``"Def/MyDef"`` is found
+        by searching for ``"Def"``.
 
         Parameters:
             search_tags (container):  A container of short_base_tags to locate.
@@ -462,11 +467,16 @@ def find_tags(self, search_tags, recursive=False, include_groups=2) -> list:
         return found_tags
 
     def find_wildcard_tags(self, search_tags, recursive=False, include_groups=2) -> list:
-        """Find the tags and their containing groups.
-
-            This searches tag.short_tag.casefold(), with an implicit wildcard on the end.
+        """Find tags whose short form starts with a given prefix (implicit trailing wildcard).
 
-            e.g. "Eve" will find Event, but not Sensory-event.
+        Comparison property: ``short_tag`` (schema short name *including* any extension or value).
+        Rationale: the query is a prefix such as ``"Def/"`` or ``"Eve"``; the match must cover
+        the extension/value as well so that ``"Def/MyDef"`` is found by ``"Def/"`` but not by
+        an unrelated tag that merely shares the same base.  ``short_tag`` is used (not
+        ``short_base_tag``) so that value-bearing tags like ``"Duration/3 s"`` can be matched
+        by a prefix query such as ``"Duration/"``.
+        Note: prefix matching is anchored to the start of ``short_tag`` only, so ``"Eve"``
+        finds ``"Event"`` but not ``"Sensory-event"``.
 
         Parameters:
             search_tags (container): A container of the starts of short tags to search.
@@ -499,15 +509,27 @@ def find_wildcard_tags(self, search_tags, recursive=False, include_groups=2) ->
         return found_tags
 
     def find_exact_tags(self, exact_tags, recursive=False, include_groups=1) -> list:
-        """Find the given tags. This will only find complete matches, any extension or value must also match.
+        """Find tags that match exactly, including any extension or value.
+
+        Comparison property: ``HedTag.__eq__`` which compares ``short_tag.casefold()``
+        (falling back to ``org_tag.casefold()`` for unrecognised tags).
+        Rationale: callers pass a slash-path string such as ``"def/mydef"`` and need an
+        exact full-path match — the extension/value is part of the identity (``"Def/Foo"``
+        must not match ``"Def/Bar"``).  Because ``HedTag.__str__`` returns ``short_tag`` when
+        the tag is schema-identified, a tag written in long form in the source HED string
+        (e.g. ``"Event/Sensory-event"``) will still be found by a short-form query
+        (``"Sensory-event"``); the schema normalises them to the same ``short_tag``.
+        Unrecognised tags fall back to a case-insensitive comparison of the original text.
 
         Parameters:
-            exact_tags (list of HedTag): A container of tags to locate.
+            exact_tags (list of str or HedTag): Tags to locate; each is compared via
+                ``HedTag.__eq__``, which accepts both ``str`` and ``HedTag`` operands.
             recursive (bool): If true, also check subgroups.
-            include_groups (bool): 0, 1 or 2.
+            include_groups (0, 1 or 2):
                 If 0: Return only tags
                 If 1: Return only groups
                 If 2 or any other value: Return both
+
         Returns:
             list: A list of tuples. The contents depend on the values of the include_group.
         """
@@ -564,12 +586,21 @@ def _get_def_tags_from_group(group):
         return def_tags
 
     def find_tags_with_term(self, term, recursive=False, include_groups=2) -> list:
-        """Find any tags that contain the given term.
-
-            Note: This can only find identified tags.
+        """Find tags whose schema ancestry includes the given term.
+
+        Comparison property: ``tag_terms`` — a tuple of all path components in the tag's
+        long-form schema path, all casefolded (e.g. ``("event", "sensory-event")`` for the
+        ``Sensory-event`` tag).
+        Rationale: this implements HED's *ancestor search* — a bare query term such as
+        ``"Event"`` must match not only the ``Event`` tag itself but also every descendant
+        (``Sensory-event``, ``Agent-action``, etc.) because those descendants inherit the
+        ``Event`` parent.  ``tag_terms`` encodes the full ancestry, so membership testing
+        (``term in tag.tag_terms``) handles all descendants in O(k) time where k is the
+        schema depth.  This requires a schema-identified tag; unidentified tags have an
+        empty ``tag_terms`` tuple and will not be found.
 
         Parameters:
-            term (str): A single term to search for.
+            term (str): A single term to search for (compared case-insensitively).
             recursive (bool): If true, recursively check subgroups.
             include_groups (0, 1 or 2): Controls return values
                 If 0: Return only tags.
@@ -579,6 +610,7 @@ def find_tags_with_term(self, term, recursive=False, include_groups=2) -> list:
         Returns:
             list: A list of tuples. The contents depend on the values of the include_group.
         """
+        # Note: unidentified tags (tag_terms == ()) are silently skipped.
         found_tags = []
         if recursive:
             tags = self.get_all_tags()

hed/models/query_expressions.py

@@ -120,6 +120,7 @@ def merge_and_groups(groups1, groups2):
             list: Groups in both lists narrowed down to results where none of the children overlap.
         """
         return_list = []
+        seen = set()
         for group in groups1:
             for other_group in groups2:
                 if group.group is other_group.group:
@@ -128,16 +129,9 @@ def merge_and_groups(groups1, groups2):
                         continue
                     # Merge the two groups' children into one new result, now that we've verified they're unique
                     merged_result = group.merge_and_result(other_group)
-
-                    dont_add = False
-                    # This is trash and slow
-                    for finalized_value in return_list:
-                        if merged_result.has_same_children(finalized_value):
-                            dont_add = True
-                            break
-                    if dont_add:
-                        continue
-                    return_list.append(merged_result)
+                    if merged_result not in seen:
+                        seen.add(merged_result)
+                        return_list.append(merged_result)
 
         return return_list
 
@@ -190,9 +184,7 @@ def handle_expr(self, hed_group, exact=False):
                 for child in group.groups():
                     groups_found.append((child, group))
 
-        # Wildcards are only found in containing groups. I believe this is correct.
-        # todo: Is this code still needed for this kind of wildcard?  We already are registering every group, just not
-        # every group at every level.
+        # Wildcards are only found in containing groups — not propagated to every parent level.
         all_found_groups = [SearchResult(group, tag) for tag, group in groups_found]
         return all_found_groups
 
@@ -217,16 +209,9 @@ def handle_expr(self, hed_group, exact=False):
         groups1 = self.left.handle_expr(hed_group, exact=exact)
         # Don't early out as we need to gather all groups in case children appear more than once etc
         groups2 = self.right.handle_expr(hed_group, exact=exact)
-        # todo: optimize this eventually
-        # Filter out duplicates
-        duplicates = []
-        for group in groups1:
-            for other_group in groups2:
-                if group.has_same_children(other_group):
-                    duplicates.append(group)
-
-        groups1 = [group for group in groups1 if not any(other_group is group for other_group in duplicates)]
-
+        # Filter out results from groups1 that are already represented in groups2
+        groups2_set = set(groups2)
+        groups1 = [g for g in groups1 if g not in groups2_set]
         return groups1 + groups2
 
     def __str__(self):
@@ -258,16 +243,9 @@ def handle_expr(self, hed_group, exact=False):
 
         """
         found_groups = self.right.handle_expr(hed_group, exact=exact)
-
-        # Todo: this may need more thought with respects to wildcards and negation
-        # negated_groups = [group for group in hed_group.get_all_groups() if group not in groups]
-        # This simpler version works on python >= 3.10
-        # negated_groups = [SearchResult(group, []) for group in hed_group.get_all_groups() if group not in groups]
-        # Python 3.7/8 compatible version.
+        found_group_ids = {id(found_group.group) for found_group in found_groups}
         negated_groups = [
-            SearchResult(group, [])
-            for group in hed_group.get_all_groups()
-            if not any(group is found_group.group for found_group in found_groups)
+            SearchResult(group, []) for group in hed_group.get_all_groups() if id(group) not in found_group_ids
         ]
 
         return negated_groups

hed/models/query_handler.py

@@ -137,7 +137,7 @@ def _handle_negation(self):
         next_token = self._next_token_is([Token.LogicalNegation])
         if next_token == Token.LogicalNegation:
             interior = self._handle_grouping_op()
-            if "?" in str(interior):
+            if self._expr_has_wildcard(interior):
                 raise HedQueryError(
                     "Cannot negate wildcards, or expressions that contain wildcards."
                     "Use {required_expression : optional_expression}."
@@ -147,6 +147,19 @@ def _handle_negation(self):
         else:
             return self._handle_grouping_op()
 
+    @staticmethod
+    def _expr_has_wildcard(expr):
+        """Return True if the expression tree contains any wildcard node."""
+        if expr is None:
+            return False
+        if isinstance(expr, ExpressionWildcardNew):
+            return True
+        if QueryHandler._expr_has_wildcard(expr.left):
+            return True
+        if QueryHandler._expr_has_wildcard(expr.right):
+            return True
+        return False
+
     def _handle_grouping_op(self):
         next_token = self._next_token_is([Token.LogicalGroup, Token.DescendantGroup, Token.ExactMatch])
         if next_token == Token.LogicalGroup:

hed/models/query_util.py

@@ -51,7 +51,7 @@ def merge_and_result(self, other):
             new_children.append(child)
         new_children.sort(key=str)
 
-        if self.group != other.group:
+        if self.group is not other.group:
             raise ValueError("Internal error")
         return SearchResult(self.group, new_children)
 
@@ -64,14 +64,26 @@ def has_same_children(self, other):
         Returns:
             bool: True if both results have the same group and identical children.
         """
-        if self.group != other.group:
+        if self.group is not other.group:
             return False
 
         if len(self.children) != len(other.children):
             return False
 
         return all(child is child2 for child, child2 in zip(self.children, other.children, strict=False))
 
+    def __eq__(self, other):
+        if not isinstance(other, SearchResult):
+            return NotImplemented
+        return (
+            self.group is other.group
+            and len(self.children) == len(other.children)
+            and all(c is c2 for c, c2 in zip(self.children, other.children, strict=False))
+        )
+
+    def __hash__(self):
+        return hash((id(self.group), tuple(id(c) for c in self.children)))
+
     # Backward compatibility alias
     has_same_tags = has_same_children
 

tests/models/test_query_handler.py

@@ -747,3 +747,130 @@ def test_match_mode_constants(self):
         self.assertNotEqual(Expression.MATCH_TERM, Expression.MATCH_EXACT)
         self.assertNotEqual(Expression.MATCH_EXACT, Expression.MATCH_WILDCARD)
         self.assertNotEqual(Expression.MATCH_TERM, Expression.MATCH_WILDCARD)
+
+    def test_ancestor_term_search(self):
+        """Bare term uses MATCH_TERM, which searches tag_terms (long-form path components).
+
+        'Event' (no slash, no asterisk) matches any tag whose schema ancestry includes 'event',
+        including the tag 'Event' itself and 'Sensory-event' whose long-form path is
+        'Event/Sensory-event'.
+        """
+        test_strings = {
+            "Event": True,
+            "Sensory-event": True,  # 'event' is an ancestor component via tag_terms
+            "Clear-throat": False,  # under Action hierarchy, not Event
+        }
+        self.base_test("Event", test_strings)
+
+    def test_term_search_case_insensitive(self):
+        """MATCH_TERM searches are case-insensitive via casefold()."""
+        test_strings = {
+            "Event": True,
+            "Sensory-event": True,
+            "Clear-throat": False,
+        }
+        self.base_test("event", test_strings)
+        self.base_test("EVENT", test_strings)
+
+    def test_wildcard_matches_short_tag_prefix(self):
+        """MATCH_WILDCARD (asterisk) prefix-matches on short_tag, not on the long-form path.
+
+        'Sensory*' matches 'Sensory-event' (short_tag starts with 'sensory').
+        'Event*' matches 'Event' but NOT 'Sensory-event' because short_tag='Sensory-event'
+        does not start with 'event'.
+        """
+        test_strings = {
+            "Sensory-event": True,
+            "Event": False,  # short_tag 'Event' does not start with 'sensory'
+        }
+        self.base_test("Sensory*", test_strings)
+
+        test_strings = {
+            "Event": True,
+            "Sensory-event": False,  # short_tag 'Sensory-event' does not start with 'event'
+        }
+        self.base_test("Event*", test_strings)
+
+    def test_slash_path_query_uses_short_tag_comparison(self):
+        """A slash-path query uses MATCH_EXACT, comparing the query string against short_tag.
+
+        'Event/Sensory-event' does NOT match the stored tag 'Sensory-event' because
+        MATCH_EXACT compares the full query path 'event/sensory-event' against
+        short_tag.casefold() = 'sensory-event'.
+
+        Value-extension tags (e.g. 'Def/Name') are an exception: their short_tag already
+        includes the slash and value, so 'Def/Name' does match.
+        """
+        test_strings = {
+            "Sensory-event": False,  # short_tag is 'Sensory-event', not 'Event/Sensory-event'
+            "Event": False,
+        }
+        self.base_test("Event/Sensory-event", test_strings)
+
+    # --- Negation wildcard check (Phase C) ---
+
+    def test_negation_wildcard_raises(self):
+        """A wildcard token anywhere inside a negation must raise HedQueryError.
+
+        The check uses an expression-tree walk (_expr_has_wildcard) rather than
+        a string search on str(interior), so it handles wildcards at any nesting
+        depth inside the negation.
+        """
+        invalid_queries = [
+            "~?",  # bare wildcard negated
+            "~??",  # tag-only wildcard negated
+            "~???",  # group wildcard negated
+            "~(A, ?)",  # wildcard inside a logical group that is negated
+            "~(A, ??)",  # tag wildcard inside negated group
+            "~(A && ?)",  # wildcard as right operand of AND inside negation
+            "~[A, ?]",  # wildcard inside a descendant group that is negated
+        ]
+        for query in invalid_queries:
+            with self.subTest(query=query):
+                with self.assertRaises(HedQueryError):
+                    QueryHandler(query)
+
+    def test_negation_no_wildcard_valid(self):
+        """Negation without wildcards must parse without error."""
+        valid_queries = [
+            "~A",
+            "~(A)",
+            "~(A && B)",
+            "~(A || B)",
+            "~[A]",
+            "~[A, B]",
+        ]
+        for query in valid_queries:
+            with self.subTest(query=query):
+                qh = QueryHandler(query)
+                self.assertIsNotNone(qh.tree)
+
+    # --- OR dedup (Phase A) ---
+
+    def test_or_no_duplicate_results(self):
+        """A || A must not return the same SearchResult more than once.
+
+        With the set-based dedup introduced in Phase A, results present in both
+        sides of an OR are filtered before the lists are concatenated.
+        """
+        expression = QueryHandler("Event || Event")
+        hed_string = HedString("Event", self.hed_schema)
+        results = expression.search(hed_string)
+        for i, r1 in enumerate(results):
+            for j, r2 in enumerate(results):
+                if i != j:
+                    self.assertFalse(
+                        r1 == r2,
+                        f"Duplicate SearchResult at positions {i} and {j}: {r1}",
+                    )
+
+    def test_or_dedup_count(self):
+        """A || A should return the same number of results as A alone."""
+        hed_string = HedString("Event", self.hed_schema)
+        single_results = QueryHandler("Event").search(hed_string)
+        or_results = QueryHandler("Event || Event").search(hed_string)
+        self.assertEqual(
+            len(single_results),
+            len(or_results),
+            "Event || Event should match the same number of groups as Event alone",
+        )