Skip to content

feat(eap): translate OP_REGEXP to ClickHouse match() #8437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pbhandari merged 5 commits into from
Sep 10, 2026
Merged

feat(eap): translate OP_REGEXP to ClickHouse match() #8437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ffa24d
feat(eap): translate OP_REGEXP to ClickHouse match()
pbhandari Sep 8, 2026
e745cbf
test(eap): ingest-level OP_REGEXP coverage and SQL interpolation canary
pbhandari Sep 8, 2026
e6e0935
ref(eap): extract any-attribute op dispatch
pbhandari Sep 8, 2026
fdfc2bc
Cleanups
pbhandari Sep 9, 2026
4217985
Map CANNOT_COMPILE_REGEXP to INVALID_REQUEST so it does not hit the SLO
pbhandari Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions snuba/clickhouse/error_codes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@


class ErrorCodes(IntEnum):
CANNOT_COMPILE_REGEXP = 36
Comment thread
pbhandari marked this conversation as resolved.
ILLEGAL_TYPE_OF_ARGUMENT = 43
ILLEGAL_COLUMN = 44
UNKNOWN_FUNCTION = 46
Expand Down
1 change: 1 addition & 0 deletions snuba/querylog/query_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ def slo(self) -> SLO:
ErrorCodes.ILLEGAL_AGGREGATION: RequestStatus.INVALID_REQUEST,
ErrorCodes.TOO_MANY_SIMULTANEOUS_QUERIES: RequestStatus.CLICKHOUSE_MAX_QUERIES_EXCEEDED,
ErrorCodes.CANNOT_PARSE_DOMAIN_VALUE_FROM_STRING: RequestStatus.INVALID_REQUEST,
ErrorCodes.CANNOT_COMPILE_REGEXP: RequestStatus.INVALID_REQUEST,
}


Expand Down
138 changes: 97 additions & 41 deletions snuba/web/rpc/common/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,9 @@ def _attribute_value_to_expression(v: AttributeValue) -> Expression:
AnyAttributeFilter.OP_NOT_IN,
}

_POSITIVE_OP_FOR_NEGATIVE: dict[int, int] = {
_POSITIVE_OP_FOR_NEGATIVE: dict[
AnyAttributeFilter.Op.ValueType, AnyAttributeFilter.Op.ValueType
] = {
AnyAttributeFilter.OP_NOT_EQUALS: AnyAttributeFilter.OP_EQUALS,
AnyAttributeFilter.OP_NOT_LIKE: AnyAttributeFilter.OP_LIKE,
AnyAttributeFilter.OP_NOT_IN: AnyAttributeFilter.OP_IN,
Expand Down Expand Up @@ -607,15 +609,15 @@ def _array_value_length(v: AttributeValue, value_type: str) -> int:
def _validate_comparison_filter_type_array(
op: ComparisonFilter.Op.ValueType, v: AttributeValue, key: AttributeKey
) -> None:
if op in (ComparisonFilter.OP_LIKE, ComparisonFilter.OP_NOT_LIKE):
if op in (ComparisonFilter.OP_LIKE, ComparisonFilter.OP_NOT_LIKE, ComparisonFilter.OP_REGEXP):
if v.WhichOneof("value") != "val_str":
raise BadSnubaRPCRequestException(
"LIKE/NOT_LIKE on array keys requires a string pattern"
)
# LIKE only matches string elements, so it makes sense only for string arrays.
label = "REGEXP" if op == ComparisonFilter.OP_REGEXP else "LIKE/NOT_LIKE"
raise BadSnubaRPCRequestException(f"{label} on array keys requires a string pattern")
# LIKE/REGEXP only match string elements, so they make sense only for string arrays.
if key.type not in (AttributeKey.Type.TYPE_ARRAY, AttributeKey.Type.TYPE_ARRAY_STRING):
label = "REGEXP" if op == ComparisonFilter.OP_REGEXP else "LIKE/NOT_LIKE"
raise BadSnubaRPCRequestException(
"LIKE/NOT_LIKE on array keys is only supported on string arrays "
f"{label} on array keys is only supported on string arrays "
f"(TYPE_ARRAY_STRING), got {AttributeKey.Type.Name(key.type)}"
)
return
Expand Down Expand Up @@ -854,6 +856,57 @@ def _typed_array_like_expression(
)


def _regexp_match(value: Expression, pattern: Expression, ignore_case: bool) -> FunctionCall:
# Needs ClickHouse > 26.8 for matchCaseInsensitive; lower() is a stand-in until then.
if ignore_case:
return f.match(f.lower(value), f.lower(pattern))
return f.match(value, pattern)

@cursor cursor Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore-case regex mangles escape sequences

Medium Severity

ignore_case applies lower() to the regexp pattern as well as the haystack. That is not equivalent to case-insensitive matching: RE2 escapes whose meaning depends on case (\S, \D, \W, \B, \Q, \A, \P) invert or break, so logs searches with ignore_case can silently return the wrong rows.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 96b1da7. Configure here.

@pbhandari pbhandari Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can fix it when we upgrade our clickhouse to 26.8+ imo. This is fine as a stopgap solution.



def _is_valid_regexp_pattern(v: AttributeValue) -> None:
if v.WhichOneof("value") != "val_str" or v.val_str == "":
raise BadSnubaRPCRequestException("REGEXP pattern must be a non-empty string")
Comment thread
pbhandari marked this conversation as resolved.


def _any_attribute_op_expression(
Comment thread
pbhandari marked this conversation as resolved.
*,
filter_: AnyAttributeFilter,
op: AnyAttributeFilter.Op.ValueType,
element: Argument,
value_expr: Expression,
membership_as_has: bool,
) -> Expression:
match op:
case AnyAttributeFilter.OP_EQUALS:
if filter_.ignore_case:
return f.equals(f.lower(element), f.lower(value_expr))
return f.equals(element, value_expr)
case AnyAttributeFilter.OP_LIKE:
if filter_.ignore_case:
return f.ilike(element, value_expr)
return f.like(element, value_expr)
case AnyAttributeFilter.OP_REGEXP:
return _regexp_match(element, value_expr, filter_.ignore_case)
case AnyAttributeFilter.OP_IN:
if filter_.ignore_case:
if filter_.value.WhichOneof("value") == "val_str_array":
lowered = [literal(s.lower()) for s in filter_.value.val_str_array.values]
else:
lowered = [
literal(elem.val_str.lower()) for elem in filter_.value.val_array.values
]
return _in_or_has(
f.lower(element),
literals_array(None, lowered),
as_has=membership_as_has,
)
return _in_or_has(element, value_expr, as_has=membership_as_has)
case _:
raise BadSnubaRPCRequestException(
f"Unsupported any_attribute_filter op: {AnyAttributeFilter.Op.Name(filter_.op)}"
)


def _any_attribute_filter_to_expression(
Comment on lines +905 to 910

@sentry sentry Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The error message for unsupported operators in _any_attribute_op_expression uses the original filter_.op instead of the effective op, which could cause confusing error messages for future negative operators.
Severity: LOW

Suggested Fix

Update the exception logging in the default case of the match statement to use the op parameter instead of filter_.op. Change f"Unsupported any_attribute_filter op: {AnyAttributeFilter.Op.Name(filter_.op)}" to f"Unsupported any_attribute_filter op: {AnyAttributeFilter.Op.Name(op)}". This ensures the error message accurately reflects the operator being processed by the function.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: snuba/web/rpc/common/common.py#L905-L910

Potential issue: In the `_any_attribute_op_expression` function, the error handling for
unsupported operators incorrectly uses the original operator from the filter,
`filter_.op`, to generate the error message. The function logic, however, operates on an
`effective_op` (passed as the `op` parameter), which resolves negative operators (e.g.,
`OP_NOT_LIKE`) to their positive counterparts. If a new, unhandled operator is added in
the future that follows this negative-to-positive mapping, the resulting error message
will be misleading by referencing the original negative operator instead of the positive
one that was actually being processed.

filt: AnyAttributeFilter,
*,
Expand Down Expand Up @@ -918,11 +971,16 @@ def _any_attribute_filter_to_expression(
)
col_name = _VALUE_TYPE_TO_COLUMN[value_type]

# LIKE/NOT_LIKE only makes sense on string columns
if effective_op == AnyAttributeFilter.OP_LIKE and col_name not in _STRING_COLUMNS:
raise BadSnubaRPCRequestException(
"LIKE/NOT_LIKE operations are only supported on string values"
)
# LIKE/NOT_LIKE/REGEXP only makes sense on string columns
if (
effective_op in (AnyAttributeFilter.OP_LIKE, AnyAttributeFilter.OP_REGEXP)
and col_name not in _STRING_COLUMNS
):
label = "REGEXP" if effective_op == AnyAttributeFilter.OP_REGEXP else "LIKE/NOT_LIKE"
raise BadSnubaRPCRequestException(f"{label} operations are only supported on string values")

if effective_op == AnyAttributeFilter.OP_REGEXP:
_is_valid_regexp_pattern(v)

# ignore_case uses lower() which only works on string columns
if filt.ignore_case and col_name not in _STRING_COLUMNS:
Expand All @@ -932,35 +990,13 @@ def _any_attribute_filter_to_expression(

# 3. Build the lambda comparison
x = Argument(None, "x")

if effective_op == AnyAttributeFilter.OP_EQUALS:
if filt.ignore_case:
comparison = f.equals(f.lower(x), f.lower(v_expression))
else:
comparison = f.equals(x, v_expression)
elif effective_op == AnyAttributeFilter.OP_LIKE:
if filt.ignore_case:
comparison = f.ilike(x, v_expression)
else:
comparison = f.like(x, v_expression)
elif effective_op == AnyAttributeFilter.OP_IN:
if filt.ignore_case:
if value_type == "val_str_array":
lowered = [literal(s.lower()) for s in v.val_str_array.values]
else:
lowered = [literal(elem.val_str.lower()) for elem in v.val_array.values]
comparison = _in_or_has(
f.lower(x),
literals_array(None, lowered),
as_has=membership_as_has,
)
else:
comparison = _in_or_has(x, v_expression, as_has=membership_as_has)
else:
raise BadSnubaRPCRequestException(
f"Unsupported any_attribute_filter op: {AnyAttributeFilter.Op.Name(filt.op)}"
)

comparison = _any_attribute_op_expression(
filter_=filt,
op=effective_op,
element=x,
value_expr=v_expression,
membership_as_has=membership_as_has,
)
lam = Lambda(None, ("x",), comparison)

# 4. Build the arrayExists expression for the single matching column.
Expand Down Expand Up @@ -1261,6 +1297,26 @@ def trace_item_filters_to_expression(
value, exists = _map_backed_operands(k)
return and_cond(exists, comparison_function(value, v_expression))
return comparison_function(k_expression, v_expression)
if op == ComparisonFilter.OP_REGEXP:
_is_valid_regexp_pattern(v)
ignore_case = item_filter.comparison_filter.ignore_case
if k.type in ARRAY_TYPES:
return f.arrayExists(
Lambda(
None,
("x",),
_regexp_match(Argument(None, "x"), v_expression, ignore_case),
),
type_array_typed_column_native_array(k, "attributes_array_string"),
)
if k.type != AttributeKey.Type.TYPE_STRING:
raise BadSnubaRPCRequestException(
"the REGEXP comparison is only supported on string and array keys"
)
if _is_map_backed_key(k):
value, exists = _map_backed_operands(k)
return and_cond(exists, _regexp_match(value, v_expression, ignore_case))
return _regexp_match(k_expression, v_expression, ignore_case)
if op == ComparisonFilter.OP_NOT_LIKE:
if k.type in ARRAY_TYPES:
return not_cond(
Expand Down
5 changes: 5 additions & 0 deletions snuba/web/rpc/common/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from google.protobuf import any_pb2, struct_pb2
from sentry_protos.snuba.v1.error_pb2 import Error as ErrorProto

from snuba.clickhouse.error_codes import ErrorCodes
from snuba.web import QueryException


Expand Down Expand Up @@ -61,5 +62,9 @@ def convert_rpc_exception_to_proto(exc: RPCRequestException | QueryException) ->
inferred_status = 500
if exc.exception_type == "RateLimitExceeded":
inferred_status = 429
else:
error_code = exc.extra.get("stats", {}).get("error_code")
if error_code == ErrorCodes.CANNOT_COMPILE_REGEXP:
inferred_status = 400

return ErrorProto(code=inferred_status, message=str(exc))
6 changes: 6 additions & 0 deletions tests/querylog/test_query_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,9 @@ def test_get_request_status_timeout(self) -> None:
status = get_request_status(error)
assert status.status == RequestStatus.CLICKHOUSE_TIMEOUT
assert status.slo == SLO.AGAINST

def test_get_request_status_cannot_compile_regexp(self) -> None:
error = ClickhouseError("cannot compile regexp", code=ErrorCodes.CANNOT_COMPILE_REGEXP)
status = get_request_status(error)
assert status.status == RequestStatus.INVALID_REQUEST
assert status.slo == SLO.FOR
Loading
Loading