Skip to content

Commit f623713

Browse files
committed
fix(eap): Use notEmpty for exists() on the ai_conversation_id column
1 parent 1ae1b4c commit f623713

3 files changed

Lines changed: 36 additions & 0 deletions

File tree

snuba/protos/common.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,12 @@ def _generate_subscriptable_reference(
191191
"attributes_array_bool",
192192
)
193193

194+
# Normalized String columns whose unset value ingests as an empty string (not NULL) — e.g.
195+
# ai_conversation_id when a TraceItem has no conversation_id. Because the column is never NULL,
196+
# existence must be checked with notEmpty(...) instead of isNotNull(...), which would otherwise
197+
# match every row (see get_field_existence_expression).
198+
EMPTY_STRING_DEFAULT_COLUMNS: tuple[str, ...] = ("ai_conversation_id",)
199+
194200

195201
def type_array_typed_columns_select_expressions(attr_key: AttributeKey) -> list[FunctionCall]:
196202
"""Native ``arrayElement`` read per typed array map column, for a deprecated untyped

snuba/web/rpc/common/common.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from snuba.protos.common import (
1818
ARRAY_TYPES,
1919
ATTRIBUTES_TO_COALESCE,
20+
EMPTY_STRING_DEFAULT_COLUMNS,
2021
NORMALIZED_COLUMNS_EAP_ITEMS,
2122
PROTO_TYPE_TO_ATTRIBUTE_COLUMN,
2223
PROTO_TYPE_TO_CLICKHOUSE_TYPE,
@@ -1256,6 +1257,7 @@ def get_subscriptable_field(field: Expression) -> SubscriptableReference | None:
12561257
# is the right existence check. Scalar map lookups still use map_key_exists.
12571258
if isinstance(base, Column) and base.column_name in TYPED_ARRAY_MAP_COLUMNS:
12581259
return f.notEmpty(field)
1260+
12591261
return map_key_exists(field.parameters[0], field.parameters[1])
12601262

12611263
if isinstance(field, FunctionCall) and field.function_name in ("arrayMap", "arrayConcat"):
@@ -1265,4 +1267,12 @@ def get_subscriptable_field(field: Expression) -> SubscriptableReference | None:
12651267
# type_array_to_membership_array_expression_from_typed_columns).
12661268
return f.notEmpty(field)
12671269

1270+
if isinstance(field, FunctionCall) and field.function_name == "cast":
1271+
# A normalized String column with an empty-string default (e.g. ai_conversation_id)
1272+
# is never NULL, so isNotNull would always be true. notEmpty distinguishes an unset
1273+
# (empty) value from a real one.
1274+
base = field.parameters[0]
1275+
if isinstance(base, Column) and base.column_name in EMPTY_STRING_DEFAULT_COLUMNS:
1276+
return f.notEmpty(field)
1277+
12681278
return f.isNotNull(field)

tests/web/rpc/v1/test_indexed_columns.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
)
2424
from sentry_protos.snuba.v1.trace_item_filter_pb2 import (
2525
ComparisonFilter,
26+
ExistsFilter,
2627
TraceItemFilter,
2728
)
2829
from sentry_protos.snuba.v1.trace_item_pb2 import AnyValue, TraceItem
@@ -83,6 +84,11 @@ def setup(self, eap: None, redis_db: None) -> None:
8384
messages = [
8485
_item_message(self.base_time, SESSION_A, CONV_A, "red"),
8586
_item_message(self.base_time + timedelta(minutes=1), SESSION_B, CONV_B, "blue"),
87+
# "green" sets neither: an unset conversation_id ingests as an empty
88+
# ai_conversation_id, so exists on gen_ai.conversation.id must exclude it.
89+
# (session_id is intentionally not exercised for exists: an unset session_id
90+
# ingests as a random non-nil UUID, so absence is indistinguishable there.)
91+
_item_message(self.base_time + timedelta(minutes=2), "", "", "green"),
8692
]
8793
write_raw_unprocessed_events(get_writable_storage(StorageKey("eap_items")), messages)
8894

@@ -178,3 +184,17 @@ def test_filter_uses_skip_index_and_matches(
178184
)
179185
assert colors == expected_colors
180186
assert self._explain_uses_index(sql, expected_index), sql
187+
188+
def test_exists_on_conversation_id_excludes_rows_without_a_value(self) -> None:
189+
# gen_ai.conversation.id is optional: the "green" span never set it, so its
190+
# ai_conversation_id is empty. exists() must treat empty as absent and exclude
191+
# it, rather than matching every row (a non-nullable column is never NULL, so a
192+
# plain isNotNull check would always be true).
193+
_, colors = self._execute(
194+
TraceItemFilter(
195+
exists_filter=ExistsFilter(
196+
key=AttributeKey(type=AttributeKey.TYPE_STRING, name="gen_ai.conversation.id")
197+
)
198+
)
199+
)
200+
assert colors == ["blue", "red"]

0 commit comments

Comments
 (0)