Minor changes & cleanup

Author: shashjar

src/sentry/search/eap/occurrences/search_executor.py

@@ -11,14 +11,13 @@
 from sentry.search.eap.types import SearchResolverConfig
 from sentry.search.events.types import SnubaParams
 from sentry.snuba.occurrences_rpc import Occurrences
-from sentry.utils import metrics
 
 logger = logging.getLogger(__name__)
 
 
 # Filters that must be skipped because they have no EAP equivalent.
 # These would silently become dynamic tag lookups in the EAP SearchResolver
-# (resolver.py:1026-1060) and produce incorrect results.
+# and produce incorrect results.
 # TODO: these are potentially gaps between existing issue feed search behavior and EAP search behavior. May need to adddress.
 SKIP_FILTERS: frozenset[str] = frozenset(
     {
@@ -60,7 +59,8 @@
 def search_filters_to_query_string(
     search_filters: Sequence[SearchFilter],
 ) -> str:
-    """Convert Snuba-relevant SearchFilter objects to an EAP query string.
+    """
+    Convert Snuba-relevant SearchFilter objects to an EAP query string.
 
     Expects filters that have already been stripped of postgres-only fields
     (status, assigned_to, bookmarked_by, etc.) by the caller.
@@ -85,10 +85,6 @@ def _convert_single_filter(sf: SearchFilter) -> str | None:
         return _convert_aggregation_filter(sf)
 
     if key in SKIP_FILTERS:
-        metrics.incr(
-            "eap.search_executor.filter_skipped",
-            tags={"key": key},
-        )
         return None
 
     # error.unhandled requires special inversion logic.
@@ -126,17 +122,24 @@ def _convert_single_filter(sf: SearchFilter) -> str | None:
     return None
 
 
-def _convert_error_unhandled(sf: SearchFilter) -> str | None:
-    """Convert error.unhandled filter to the EAP error.handled attribute.
+def _convert_aggregation_filter(sf: SearchFilter) -> str | None:
+    eap_function = AGGREGATION_FIELD_TO_EAP_FUNCTION[sf.key.name]
+    formatted_value = _format_value(sf.value.raw_value)
 
-    error.unhandled:1 (or true)  → !error.handled:1
-    error.unhandled:0 (or false) → error.handled:1
-    !error.unhandled:1           → error.handled:1
-    """
+    if sf.operator in (">", ">=", "<", "<="):
+        return f"{eap_function}:{sf.operator}{formatted_value}"
+    elif sf.operator == "=":
+        return f"{eap_function}:{formatted_value}"
+    elif sf.operator == "!=":
+        return f"!{eap_function}:{formatted_value}"
+
+    return None
+
+
+def _convert_error_unhandled(sf: SearchFilter) -> str | None:
     raw_value = sf.value.raw_value
     op = sf.operator
 
-    # Determine if the user is looking for unhandled errors
     is_looking_for_unhandled = (op == "=" and raw_value in ("1", 1, True, "true")) or (
         op == "!=" and raw_value in ("0", 0, False, "false")
     )
@@ -147,24 +150,38 @@ def _convert_error_unhandled(sf: SearchFilter) -> str | None:
         return "error.handled:1"
 
 
-def _convert_aggregation_filter(sf: SearchFilter) -> str | None:
-    """Convert a legacy aggregation field filter to EAP function syntax.
+def _format_value(
+    raw_value: str | int | float | datetime | Sequence[str] | Sequence[float],
+) -> str:
+    if isinstance(raw_value, (list, tuple)):
+        parts = ", ".join(_format_single_value(v) for v in raw_value)
+        return f"[{parts}]"
+    if isinstance(raw_value, datetime):
+        return raw_value.isoformat()
+    if isinstance(raw_value, (int, float)):
+        return str(raw_value)
+    return _format_string_value(str(raw_value))
 
-    e.g. times_seen:>100 → count():>100
-         last_seen:>2024-01-01 → last_seen():>2024-01-01T00:00:00+00:00
-         user_count:>5 → count_unique(user):>5
-    """
-    eap_function = AGGREGATION_FIELD_TO_EAP_FUNCTION[sf.key.name]
-    formatted_value = _format_value(sf.value.raw_value)
 
-    if sf.operator in (">", ">=", "<", "<="):
-        return f"{eap_function}:{sf.operator}{formatted_value}"
-    elif sf.operator == "=":
-        return f"{eap_function}:{formatted_value}"
-    elif sf.operator == "!=":
-        return f"!{eap_function}:{formatted_value}"
+def _format_single_value(value: str | int | float | datetime) -> str:
+    if isinstance(value, datetime):
+        return value.isoformat()
+    if isinstance(value, (int, float)):
+        return str(value)
+    return _format_string_value(str(value))
 
-    return None
+
+def _format_string_value(s: str) -> str:
+    # Wildcard values pass through as-is for the SearchResolver to handle
+    if "*" in s:
+        return s
+
+    # Quote strings containing spaces or special characters
+    if " " in s or '"' in s or "," in s or "(" in s or ")" in s:
+        escaped = s.replace("\\", "\\\\").replace('"', '\\"')
+        return f'"{escaped}"'
+
+    return s
 
 
 # Maps legacy sort_field names (from PostgresSnubaQueryExecutor.sort_strategies values)
@@ -177,7 +194,7 @@ def _convert_aggregation_filter(sf: SearchFilter) -> str | None:
 #   "user" → "user_count" → uniq(tags[sentry:user])
 #   "trends" → "trends"   → complex ClickHouse expression (not supported)
 #   "recommended" → "recommended" → complex ClickHouse expression (not supported)
-#   "inbox" → ""           → Postgres only (not supported)
+#   "inbox" → ""          → Postgres only (not supported)
 EAP_SORT_STRATEGIES: dict[str, tuple[list[str], list[str]]] = {
     "last_seen": (["group_id", "last_seen()"], ["-last_seen()"]),
     "times_seen": (["group_id", "count()"], ["-count()"]),
@@ -199,14 +216,12 @@ def run_eap_group_search(
     search_filters: Sequence[SearchFilter] | None = None,
     referrer: str = "",
 ) -> tuple[list[tuple[int, Any]], int]:
-    """EAP equivalent of PostgresSnubaQueryExecutor.snuba_search().
+    """
+    EAP equivalent of PostgresSnubaQueryExecutor.snuba_search().
 
     Returns a tuple of:
         * a list of (group_id, sort_score) tuples,
         * total count (0 during double-reading; legacy provides the real total).
-
-    This matches the return signature of snuba_search() so it can be used
-    as the experimental branch in check_and_choose().
     """
     if sort_field not in EAP_SORT_STRATEGIES:
         return ([], 0)
@@ -269,41 +284,7 @@ def run_eap_group_search(
         if group_id is not None:
             tuples.append((int(group_id), score))
 
-    # The EAP RPC TraceItemTableResponse does not include a total count
+    # TODO: the EAP RPC TraceItemTableResponse does not include a total count
     # (unlike Snuba's totals=True). During double-reading the legacy result
     # provides the real total, so we return 0 here.
     return (tuples, 0)
-
-
-def _format_value(
-    raw_value: str | int | float | datetime | Sequence[str] | Sequence[float],
-) -> str:
-    if isinstance(raw_value, (list, tuple)):
-        parts = ", ".join(_format_single_value(v) for v in raw_value)
-        return f"[{parts}]"
-    if isinstance(raw_value, datetime):
-        return raw_value.isoformat()
-    if isinstance(raw_value, (int, float)):
-        return str(raw_value)
-    return _format_string_value(str(raw_value))
-
-
-def _format_single_value(value: str | int | float | datetime) -> str:
-    if isinstance(value, datetime):
-        return value.isoformat()
-    if isinstance(value, (int, float)):
-        return str(value)
-    return _format_string_value(str(value))
-
-
-def _format_string_value(s: str) -> str:
-    # Wildcard values pass through as-is for the SearchResolver to handle
-    if "*" in s:
-        return s
-
-    # Quote strings containing spaces or special characters
-    if " " in s or '"' in s or "," in s or "(" in s or ")" in s:
-        escaped = s.replace("\\", "\\\\").replace('"', '\\"')
-        return f'"{escaped}"'
-
-    return s

tests/sentry/search/eap/test_search_executor.py

@@ -10,7 +10,6 @@
 
 class TestSearchFiltersToQueryString:
     def test_all_operator_types(self):
-        """Each operator type produces the correct EAP query syntax."""
         cases = [
             (SearchFilter(SearchKey("level"), "=", SearchValue("error")), "level:error"),
             (SearchFilter(SearchKey("level"), "!=", SearchValue("error")), "!level:error"),
@@ -33,8 +32,6 @@ def test_all_operator_types(self):
             )
 
     def test_value_formatting(self):
-        """Values with special characters, wildcards, numerics, and datetimes
-        are formatted correctly."""
         dt = datetime(2024, 1, 15, 12, 0, 0, tzinfo=timezone.utc)
         cases = [
             # Wildcards pass through as-is
@@ -67,7 +64,6 @@ def test_value_formatting(self):
             assert search_filters_to_query_string([sf]) == expected
 
     def test_has_and_not_has_filters(self):
-        """Empty-value filters are converted to has:/!has: syntax."""
         # has:user.email → parsed as op=!=, value=""
         has_filter = SearchFilter(SearchKey("user.email"), "!=", SearchValue(""))
         assert search_filters_to_query_string([has_filter]) == "has:user.email"
@@ -77,7 +73,6 @@ def test_has_and_not_has_filters(self):
         assert search_filters_to_query_string([not_has_filter]) == "!has:user.email"
 
     def test_skipped_filters_are_dropped(self):
-        """All filters with no EAP equivalent are silently dropped."""
         filters = [
             SearchFilter(SearchKey("event.type"), "=", SearchValue("error")),
             SearchFilter(SearchKey("release.stage"), "=", SearchValue("adopted")),
@@ -91,8 +86,6 @@ def test_skipped_filters_are_dropped(self):
         assert search_filters_to_query_string(filters) == ""
 
     def test_aggregation_filters_translated(self):
-        """Legacy aggregation field names are translated to EAP function syntax
-        so the SearchResolver parses them as AggregateFilter (HAVING) conditions."""
         dt = datetime(2024, 1, 15, 12, 0, 0, tzinfo=timezone.utc)
         cases = [
             (
@@ -118,7 +111,6 @@ def test_aggregation_filters_translated(self):
             )
 
     def test_error_unhandled_translation(self):
-        """error.unhandled is inverted to use the EAP error.handled attribute."""
         # error.unhandled:1 → looking for unhandled → !error.handled:1
         assert (
             search_filters_to_query_string(
@@ -142,14 +134,10 @@ def test_error_unhandled_translation(self):
         )
 
     def test_error_main_thread_key_translated(self):
-        """error.main_thread is renamed to the EAP attribute name."""
         filters = [SearchFilter(SearchKey("error.main_thread"), "=", SearchValue("1"))]
         assert search_filters_to_query_string(filters) == "exception_main_thread:1"
 
     def test_realistic_mixed_query(self):
-        """A realistic issue feed query mixing supported, skipped, and translated filters.
-        Verifies that supported filters are converted, skipped filters are dropped,
-        aggregation filters are translated, and special filters are rewritten."""
         filters = [
             SearchFilter(SearchKey("level"), "=", SearchValue("error")),
             SearchFilter(SearchKey("error.unhandled"), "=", SearchValue("1")),
@@ -270,8 +258,6 @@ def test_user_count_sort(self) -> None:
         assert group_ids[1] == self.group1.id
 
     def test_unsupported_sort_returns_empty(self) -> None:
-        """Unsupported sort strategies (trends, recommended) return empty
-        so the caller can fall back to the legacy result."""
         result, total = run_eap_group_search(
             start=self.start,
             end=self.end,
@@ -285,7 +271,6 @@ def test_unsupported_sort_returns_empty(self) -> None:
         assert total == 0
 
     def test_filter_narrows_results(self) -> None:
-        """A level filter excludes groups that don't match."""
         result, _ = run_eap_group_search(
             start=self.start,
             end=self.end,
@@ -300,7 +285,6 @@ def test_filter_narrows_results(self) -> None:
         assert group_ids == {self.group1.id}
 
     def test_group_id_pre_filter(self) -> None:
-        """Pre-filtered group_ids are passed as extra_conditions, narrowing results."""
         result, _ = run_eap_group_search(
             start=self.start,
             end=self.end,
@@ -314,7 +298,6 @@ def test_group_id_pre_filter(self) -> None:
         assert {gid for gid, _ in result} == {self.group1.id}
 
     def test_environment_filter(self) -> None:
-        """Environment IDs are applied via SnubaParams to narrow results."""
         env = self.create_environment(project=self.project, name="production")
         occ = self.create_eap_occurrence(
             group_id=self.group1.id,
@@ -346,9 +329,6 @@ def test_environment_filter(self) -> None:
         assert self.group2.id not in group_ids
 
     def test_sort_and_filter(self) -> None:
-        """Combines sorting, filtering, and group_id pre-filtering in one query.
-        Creates 3 groups across 2 levels, pre-filters to 2 of them, filters by
-        level, and verifies the remaining group is returned with correct sort order."""
         group3 = self.create_group(project=self.project)
         for i in range(5):
             occ = self.create_eap_occurrence(