From bc5b99362890d0c25bcf11a531d32fc2089f720f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 03:07:02 +0000 Subject: [PATCH 01/27] feat(eap): add semver sort key for sentry.release ORDER BY in EAP Fixes lexicographic ordering of release strings by wrapping `sentry.release` in a `tuple(arrayResize(arrayMap(...), 4), is_stable)` sort key when used in ORDER BY / GROUP BY in EAP trace item table queries. This ensures: - 1.2.9 sorts before 1.2.10 (numeric, not lexicographic) - 1.2.3-beta.1 sorts before 1.2.3 (prerelease before stable) - 1.2.3-beta.1 sorts after 1.2.2 (prerelease of newer > older stable) - 1.2 and 1.2.0 are equal (arrayResize pads to 4 components) - package@version strips the prefix before comparison Uses only ClickHouse functions available on Altinity 25.3/25.8 (no naturalSortKey which requires upstream 26.3+). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 25 +++++ .../R_eap_items/resolver_trace_item_table.py | 8 +- tests/web/rpc/test_common.py | 29 ++++++ .../test_endpoint_trace_item_table.py | 97 +++++++++++++++++++ 4 files changed, 158 insertions(+), 1 deletion(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index d196a66e80..46c3ef8f56 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -85,6 +85,31 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: raise BadSnubaRPCRequestException(str(e)) from e +_SEMVER_COMPONENT_COUNT = 4 # major.minor.patch.build + + +def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: + """Return a Tuple(Array(UInt32), UInt8) sort key for semantic-version ORDER BY. + + Strips a leading 'package@' prefix, isolates the release part (before '-'), + maps each dot-component to UInt32, pads to 4 elements so "1.2" == "1.2.0", + and adds a stability flag (0=prerelease, 1=stable) so prerelease versions sort + before their corresponding stable release. Works on Altinity 25.3/25.8. + """ + x = Argument(None, "x") + version_no_prefix = f.arrayElement(f.splitByChar(literal("@"), expr), literal(-1)) + release_part = f.arrayElement(f.splitByChar(literal("-"), version_no_prefix), literal(1)) + numeric_key = f.arrayResize( + f.arrayMap( + Lambda(None, ("x",), f.toUInt32OrZero(x)), + f.splitByChar(literal("."), release_part), + ), + literal(_SEMVER_COMPONENT_COUNT), + ) + is_stable = f.equals(f.position(version_no_prefix, literal("-")), literal(0)) + return f.tuple(numeric_key, is_stable, alias=alias) + + def _trace_item_filter_key_expression( attr_to_key_expression_callable: Callable[[AttributeKey], Expression], key: AttributeKey ) -> Expression: diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index caa9cd2da9..f3ce9bf9f8 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -59,6 +59,7 @@ attribute_key_to_expression, base_conditions_and, get_field_existence_expression, + semver_sort_key, timestamp_in_range_condition, trace_item_filters_to_expression, treeify_or_and_conditions, @@ -88,6 +89,8 @@ _DEFAULT_ROW_LIMIT = 10_000 +_SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release"}) + OP_TO_EXPR = { Column.BinaryFormula.OP_ADD: f.plus, @@ -283,7 +286,10 @@ def _groupby_order_by_expression(attr_key: AttributeKey) -> Expression: """ if attr_key.name == "sentry.timestamp": return snuba_column("timestamp") - return attribute_key_to_expression(attr_key) + base = attribute_key_to_expression(attr_key) + if attr_key.name in _SEMVER_SORT_ATTRIBUTES: + return semver_sort_key(base) + return base def _convert_order_by( diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index d6ce06c540..6c7173ac2c 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -48,6 +48,7 @@ dedupe_and_conditions, next_monday, prev_monday, + semver_sort_key, trace_item_filters_to_expression, treeify_or_and_conditions, use_sampling_factor, @@ -1082,3 +1083,31 @@ def test_like_wildcard_matches_present_not_absent(self) -> None: def test_not_like_wildcard_matches_only_absent(self) -> None: # Present rows all `like '%'`, so only the absent key survives NOT LIKE. assert self._execute(ComparisonFilter.OP_NOT_LIKE, value="%") == ["green"] + + +class TestSemverSortKey: + def test_expression_structure(self) -> None: + from snuba.query.dsl import column as snuba_column + + expr = semver_sort_key(snuba_column("release")) + assert isinstance(expr, FunctionCall) + assert expr.function_name == "tuple" + assert len(expr.parameters) == 2 + numeric_key_expr, is_stable_expr = expr.parameters + assert isinstance(numeric_key_expr, FunctionCall) + assert numeric_key_expr.function_name == "arrayResize" + assert isinstance(is_stable_expr, FunctionCall) + assert is_stable_expr.function_name == "equals" + + def test_alias_is_forwarded(self) -> None: + from snuba.query.dsl import column as snuba_column + + expr = semver_sort_key(snuba_column("release"), alias="semver_key") + assert isinstance(expr, FunctionCall) + assert expr.alias == "semver_key" + + def test_no_alias_by_default(self) -> None: + from snuba.query.dsl import column as snuba_column + + expr = semver_sort_key(snuba_column("release")) + assert expr.alias is None diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index 203d217cb1..f52fb271ae 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4402,3 +4402,100 @@ def test_order_by_bug() -> None: ) with pytest.raises(BadSnubaRPCRequestException, match=error_message): _validate_order_by(message) + + +@pytest.mark.clickhouse_db +@pytest.mark.redis_db +class TestSemverSorting: + """ORDER BY sentry.release uses the tuple(arrayResize(…), is_stable) key so + versions sort numerically (1.2.9 before 1.2.10) with pre-releases before + their corresponding stable release. + """ + + _RELEASES = [ + "1.2.9", + "1.2.10", + "1.2.3", + "1.2.3-beta.1", + "1.2.2", + "my-pkg@2.0.0", + "1.2", + "1.2.0", + ] + + @pytest.fixture(autouse=True) + def _setup(self, clickhouse_db: None, redis_db: None) -> None: + for i, release in enumerate(self._RELEASES): + write_eap_item( + start_timestamp=BASE_TIME + timedelta(minutes=i), + raw_attributes={"sentry.release": release, "semver_test_marker": "1"}, + ) + + def _query_releases(self, descending: bool = False) -> list[str]: + message = TraceItemTableRequest( + meta=RequestMeta( + project_ids=[1], + organization_id=1, + cogs_category="something", + referrer="something", + start_timestamp=START_TIMESTAMP, + end_timestamp=END_TIMESTAMP, + trace_item_type=TraceItemType.TRACE_ITEM_TYPE_SPAN, + ), + filter=TraceItemFilter( + exists_filter=ExistsFilter( + key=AttributeKey(type=AttributeKey.TYPE_STRING, name="semver_test_marker") + ) + ), + columns=[ + Column(key=AttributeKey(type=AttributeKey.TYPE_STRING, name="sentry.release")) + ], + order_by=[ + TraceItemTableRequest.OrderBy( + column=Column( + key=AttributeKey(type=AttributeKey.TYPE_STRING, name="sentry.release") + ), + descending=descending, + ) + ], + limit=len(self._RELEASES) + 10, + ) + response = EndpointTraceItemTable().execute(message) + return [v.val_str for v in response.column_values[0].results] + + def test_numeric_ordering(self) -> None: + releases = self._query_releases() + assert releases.index("1.2.9") < releases.index("1.2.10"), ( + "1.2.9 must sort before 1.2.10 (numeric, not lexicographic)" + ) + + def test_prerelease_before_stable(self) -> None: + releases = self._query_releases() + assert releases.index("1.2.3-beta.1") < releases.index("1.2.3"), ( + "prerelease 1.2.3-beta.1 must sort before stable 1.2.3" + ) + + def test_prerelease_of_newer_after_older_stable(self) -> None: + releases = self._query_releases() + assert releases.index("1.2.3-beta.1") > releases.index("1.2.2"), ( + "prerelease 1.2.3-beta.1 must sort after older stable 1.2.2" + ) + + def test_package_prefix_stripped(self) -> None: + releases = self._query_releases() + assert releases.index("my-pkg@2.0.0") > releases.index("1.2.10"), ( + "my-pkg@2.0.0 should sort as version 2.0.0 (after 1.x)" + ) + + def test_length_normalisation(self) -> None: + releases = self._query_releases() + idx_12 = releases.index("1.2") + idx_120 = releases.index("1.2.0") + assert abs(idx_12 - idx_120) <= 1, ( + "1.2 and 1.2.0 should sort as equal (both normalise to [1,2,0,0])" + ) + + def test_desc_is_reverse_of_asc(self) -> None: + asc = self._query_releases(descending=False) + desc = self._query_releases(descending=True) + assert asc == list(reversed(desc)) From f08a5cdf7fc2428a4790ea5a011aa6b1ca0fefbf Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 03:23:20 +0000 Subject: [PATCH 02/27] fix(eap): fix mypy error and GROUP BY semver tuple mismatch Two fixes for the semver sort key implementation: 1. Replace f.tuple(..., alias=alias) with FunctionCall(alias, "tuple", ...) to fix mypy "Optional[str] not compatible with str" error on the alias argument. 2. Only apply semver_sort_key in the ORDER BY path, not GROUP BY. GROUP BY sentry.release must use the raw attribute expression so that the SELECT (also the raw string) is a function of the GROUP BY key and ClickHouse accepts the aggregation query. ORDER BY on a function of a GROUP BY key is valid in ClickHouse, so semver ordering still works. This also fixes the 500 error in Sentry's test_semver_build test where the spans events API auto-groups by release. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 2 +- .../R_eap_items/resolver_trace_item_table.py | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index 46c3ef8f56..ae32856390 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -107,7 +107,7 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: literal(_SEMVER_COMPONENT_COUNT), ) is_stable = f.equals(f.position(version_no_prefix, literal("-")), literal(0)) - return f.tuple(numeric_key, is_stable, alias=alias) + return FunctionCall(alias, "tuple", (numeric_key, is_stable)) def _trace_item_filter_key_expression( diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index f3ce9bf9f8..94eb6b940f 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -271,7 +271,9 @@ def aggregation_filter_to_expression( raise BadSnubaRPCRequestException(f"Unsupported aggregation filter type: {default}") -def _groupby_order_by_expression(attr_key: AttributeKey) -> Expression: +def _groupby_order_by_expression( + attr_key: AttributeKey, for_order_by: bool = False +) -> Expression: """ Maps an attribute key used in GROUP BY / ORDER BY to its expression. @@ -283,11 +285,15 @@ def _groupby_order_by_expression(attr_key: AttributeKey) -> Expression: valid (it is a function of the grouped column) while letting ORDER BY sort on the raw column. If the two diverged, ClickHouse would reject the query with "Column `timestamp` is not under aggregate function and not in GROUP BY". + + For `_SEMVER_SORT_ATTRIBUTES` (e.g. `sentry.release`), the ORDER BY uses + a semver tuple key while GROUP BY keeps the raw expression. ClickHouse + accepts ORDER BY on a function of a GROUP BY key, so the two can differ here. """ if attr_key.name == "sentry.timestamp": return snuba_column("timestamp") base = attribute_key_to_expression(attr_key) - if attr_key.name in _SEMVER_SORT_ATTRIBUTES: + if for_order_by and attr_key.name in _SEMVER_SORT_ATTRIBUTES: return semver_sort_key(base) return base @@ -341,7 +347,7 @@ def _convert_order_by( # covers `sentry.timestamp` ordering anywhere else.) The GROUP BY uses the # same expression (see `_groupby_order_by_expression`) so an aggregation # query that orders by `sentry.timestamp` stays valid. - expression = _groupby_order_by_expression(x.column.key) + expression = _groupby_order_by_expression(x.column.key, for_order_by=True) res.append( OrderBy( direction=direction, From 4be96cdcffcf637c248429c6c5e1ab698c2bf9c5 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Thu, 25 Jun 2026 03:25:50 +0000 Subject: [PATCH 03/27] [getsentry/action-github-commit] Auto commit --- .../rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index 94eb6b940f..67ecda1081 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -271,9 +271,7 @@ def aggregation_filter_to_expression( raise BadSnubaRPCRequestException(f"Unsupported aggregation filter type: {default}") -def _groupby_order_by_expression( - attr_key: AttributeKey, for_order_by: bool = False -) -> Expression: +def _groupby_order_by_expression(attr_key: AttributeKey, for_order_by: bool = False) -> Expression: """ Maps an attribute key used in GROUP BY / ORDER BY to its expression. From 1e2d24bf1a997419a763be055c40f5be3929c485 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 03:32:59 +0000 Subject: [PATCH 04/27] fix(eap): apply semver sort key to flextime pagination boundary comparison The flextime pagination filter was comparing the raw release string lexicographically, while ORDER BY now uses the semver tuple key. This caused rows to be skipped or repeated on follow-up pages when `sentry.release` was in the order clause (e.g. "1.2.3-beta.1" would be missed because it sorts after "1.2.3" lexicographically but before it semantically). Fix: move `SEMVER_SORT_ATTRIBUTES` to common.py so pagination.py can import it. In `get_filters`, wrap both the column reference and the stored literal value in `semver_sort_key(...)` for any semver attribute so the page-boundary `<` comparison uses the same ordering as ORDER BY. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 2 ++ snuba/web/rpc/common/pagination.py | 26 ++++++++++++++++--- .../R_eap_items/resolver_trace_item_table.py | 7 +++-- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index ae32856390..30214ffd84 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -87,6 +87,8 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: _SEMVER_COMPONENT_COUNT = 4 # major.minor.patch.build +SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release"}) + def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: """Return a Tuple(Array(UInt32), UInt8) sort key for semantic-version ORDER BY. diff --git a/snuba/web/rpc/common/pagination.py b/snuba/web/rpc/common/pagination.py index 5abcd23f9c..1d210c0f6b 100644 --- a/snuba/web/rpc/common/pagination.py +++ b/snuba/web/rpc/common/pagination.py @@ -18,7 +18,11 @@ from snuba.query.dsl import Functions as f from snuba.query.dsl import column, literal from snuba.query.expressions import Expression -from snuba.web.rpc.common.common import attribute_key_to_expression +from snuba.web.rpc.common.common import ( + SEMVER_SORT_ATTRIBUTES, + attribute_key_to_expression, + semver_sort_key, +) from snuba.web.rpc.storage_routing.routing_strategies.storage_routing import TimeWindow @@ -99,9 +103,23 @@ def get_filters(self) -> Expression | None: ) # Assumes everything in the ORDER BY is ordered by DESC if column_names: - res = f.less( - f.tuple(*(column(c_name) for c_name in column_names)), f.tuple(*column_values) - ) + col_exprs = [] + val_exprs = [] + for c_name, c_value in zip(column_names, column_values): + # c_name is the attribute expression alias: "{attr}.{Type.Name(type)}" + # For semver attributes (e.g. sentry.release_TYPE_STRING), apply the + # same semver sort key on both sides so the page boundary comparison + # uses the same ordering as ORDER BY. + attr_name = ( + c_name.removesuffix("_TYPE_STRING") if c_name.endswith("_TYPE_STRING") else None + ) + if attr_name in SEMVER_SORT_ATTRIBUTES: + col_exprs.append(semver_sort_key(column(c_name))) + val_exprs.append(semver_sort_key(c_value)) + else: + col_exprs.append(column(c_name)) + val_exprs.append(c_value) + res = f.less(f.tuple(*col_exprs), f.tuple(*val_exprs)) return res return None diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index 67ecda1081..9729c4b4b6 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -55,6 +55,7 @@ from snuba.utils.metrics.timer import Timer from snuba.web.query import run_query from snuba.web.rpc.common.common import ( + SEMVER_SORT_ATTRIBUTES, add_existence_check_to_subscriptable_references, attribute_key_to_expression, base_conditions_and, @@ -89,8 +90,6 @@ _DEFAULT_ROW_LIMIT = 10_000 -_SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release"}) - OP_TO_EXPR = { Column.BinaryFormula.OP_ADD: f.plus, @@ -284,14 +283,14 @@ def _groupby_order_by_expression(attr_key: AttributeKey, for_order_by: bool = Fa the raw column. If the two diverged, ClickHouse would reject the query with "Column `timestamp` is not under aggregate function and not in GROUP BY". - For `_SEMVER_SORT_ATTRIBUTES` (e.g. `sentry.release`), the ORDER BY uses + For `SEMVER_SORT_ATTRIBUTES` (e.g. `sentry.release`), the ORDER BY uses a semver tuple key while GROUP BY keeps the raw expression. ClickHouse accepts ORDER BY on a function of a GROUP BY key, so the two can differ here. """ if attr_key.name == "sentry.timestamp": return snuba_column("timestamp") base = attribute_key_to_expression(attr_key) - if for_order_by and attr_key.name in _SEMVER_SORT_ATTRIBUTES: + if for_order_by and attr_key.name in SEMVER_SORT_ATTRIBUTES: return semver_sort_key(base) return base From 62fcc84bdaf6281463fc18faee6b099c5b0857c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 03:49:35 +0000 Subject: [PATCH 05/27] fix(eap): handle Nullable(String) in semver_sort_key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sentry.release is coalesced from multiple attribute columns (attributes_string_25['sentry.release'] + attributes_string_30['release']), which makes the expression Nullable(String). ClickHouse forbids Nullable(Array(...)), so splitByChar on the raw expression would throw: Code: 43 – Nested type Array(String) cannot be inside Nullable type Fix: apply ifNull(expr, '') at the start of semver_sort_key to convert Nullable(String) to String before any string→array operations. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index 30214ffd84..69585febd3 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -99,7 +99,11 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: before their corresponding stable release. Works on Altinity 25.3/25.8. """ x = Argument(None, "x") - version_no_prefix = f.arrayElement(f.splitByChar(literal("@"), expr), literal(-1)) + # sentry.release is coalesced from multiple attribute columns and therefore + # returns Nullable(String). ClickHouse forbids Nullable(Array(…)), so strip + # the nullable wrapper before applying any string → array functions. + non_null = f.ifNull(expr, literal("")) + version_no_prefix = f.arrayElement(f.splitByChar(literal("@"), non_null), literal(-1)) release_part = f.arrayElement(f.splitByChar(literal("-"), version_no_prefix), literal(1)) numeric_key = f.arrayResize( f.arrayMap( From 0dc485b5721ceccaf2a1e7bef3c8efd2202a740c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 04:07:16 +0000 Subject: [PATCH 06/27] fix(test): handle tied semver sort keys in test_desc_is_reverse_of_asc "1.2" and "1.2.0" normalise to the same sort key ([1,2,0,0], 1). ClickHouse may return them in either relative order within the tied pair, making the strict `asc == list(reversed(desc))` assertion non-deterministic. Collapse contiguous tied elements into a frozenset before comparing so the test is order-insensitive within ties while still verifying that every other element is in exact reverse order. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- .../test_endpoint_trace_item_table.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index f52fb271ae..532360612f 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4498,4 +4498,20 @@ def test_length_normalisation(self) -> None: def test_desc_is_reverse_of_asc(self) -> None: asc = self._query_releases(descending=False) desc = self._query_releases(descending=True) - assert asc == list(reversed(desc)) + # "1.2" and "1.2.0" share the same sort key ([1,2,0,0], 1); ClickHouse + # may return them in either relative order within the tied pair. + # Collapse contiguous tied elements into a frozenset before comparing so + # the assertion is order-insensitive within ties. + TIED: frozenset[str] = frozenset({"1.2", "1.2.0"}) + + def _canonicalize(releases: list[str]) -> list[object]: + out: list[object] = [] + for r in releases: + if r in TIED: + if not out or out[-1] != TIED: + out.append(TIED) + else: + out.append(r) + return out + + assert _canonicalize(asc) == _canonicalize(list(reversed(desc))) From 6628441bb641c8dfbc0992e47ae89db97f276c25 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Fri, 26 Jun 2026 20:51:56 +0000 Subject: [PATCH 07/27] [getsentry/action-github-commit] Auto commit --- .../rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index 99aec209fc..4fa6c16451 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -353,9 +353,7 @@ def _convert_order_by( res.append( OrderBy( direction=direction, - expression=_groupby_order_by_expression( - x.column.key, for_order_by=True - ), + expression=_groupby_order_by_expression(x.column.key, for_order_by=True), ) ) elif x.column.HasField("conditional_aggregation"): From 6891fa8f5dd6440e4fc10b930892c84770e26ef1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 26 Jun 2026 20:54:08 +0000 Subject: [PATCH 08/27] fix(lint): add explicit strict=True to zip() in pagination.get_filters Master's ruff config enables flake8-bugbear; B905 requires an explicit strict= argument on zip(). column_names and column_values are built in lockstep so strict=True is correct (they always have equal length). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/pagination.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/snuba/web/rpc/common/pagination.py b/snuba/web/rpc/common/pagination.py index 1d210c0f6b..81968e07ee 100644 --- a/snuba/web/rpc/common/pagination.py +++ b/snuba/web/rpc/common/pagination.py @@ -105,7 +105,7 @@ def get_filters(self) -> Expression | None: if column_names: col_exprs = [] val_exprs = [] - for c_name, c_value in zip(column_names, column_values): + for c_name, c_value in zip(column_names, column_values, strict=True): # c_name is the attribute expression alias: "{attr}.{Type.Name(type)}" # For semver attributes (e.g. sentry.release_TYPE_STRING), apply the # same semver sort key on both sides so the page boundary comparison From b576c6cc11a2414468781c317d01787840919052 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 29 Jun 2026 13:48:06 +0000 Subject: [PATCH 09/27] feat(eap): also semver-sort sentry.sdk.version SDK versions follow the same semantic-versioning format as releases, so add sentry.sdk.version to SEMVER_SORT_ATTRIBUTES. It is a TYPE_STRING attribute, so ORDER BY and flextime pagination pick up the semver tuple key automatically. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index eb3f94594f..233c32a061 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -91,7 +91,9 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: _SEMVER_COMPONENT_COUNT = 4 # major.minor.patch.build -SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release"}) +SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset( + {"sentry.release", "sentry.sdk.version"} +) def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: From a67628ace056c2125afd9cd2a36d8f8164ecd2fc Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Mon, 29 Jun 2026 13:48:47 +0000 Subject: [PATCH 10/27] [getsentry/action-github-commit] Auto commit --- snuba/web/rpc/common/common.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index 233c32a061..0d7cad3048 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -91,9 +91,7 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: _SEMVER_COMPONENT_COUNT = 4 # major.minor.patch.build -SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset( - {"sentry.release", "sentry.sdk.version"} -) +SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release", "sentry.sdk.version"}) def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: From b989fe9acf14f8f2e34535dffc8c8b830d7d0107 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 29 Jun 2026 15:45:04 +0000 Subject: [PATCH 11/27] Revert sentry.sdk.version from SEMVER_SORT_ATTRIBUTES Per maintainer direction on the PR: keep the semver-sort attribute list hardcoded to sentry.release only for now. Broader/configurable attribute selection will be handled separately via sentry-protos. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index 0d7cad3048..eb3f94594f 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -91,7 +91,7 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: _SEMVER_COMPONENT_COUNT = 4 # major.minor.patch.build -SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release", "sentry.sdk.version"}) +SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release"}) def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: From 85e4223e69251a88bfb56f3ee3b865e9f33d8d53 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 29 Jun 2026 15:53:26 +0000 Subject: [PATCH 12/27] fix(eap): add raw-string tiebreaker to semver sort key Flextime pagination compares sentry.release with semver_sort_key under a strict `less` tuple predicate. Distinct strings sharing the same numeric key + stability flag (e.g. "1.2" and "1.2.0") compared equal, so rows tied with the page cursor but not on the previous page could be skipped on follow-up pages (Cursor Bugbot, Medium). Append the raw (non-null) release string as a third tuple element in semver_sort_key. This gives distinct release strings a deterministic total order and makes the page-boundary comparison exact. Both ORDER BY and pagination call semver_sort_key, so they stay consistent automatically. - test_common: assert the 3-element tuple shape (raw-string tiebreaker). - test_endpoint_trace_item_table: ties are now deterministic, so test_desc_is_reverse_of_asc reverts to exact-reverse and test_length_normalisation asserts "1.2" immediately precedes "1.2.0". Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 13 +++++++-- tests/web/rpc/test_common.py | 7 +++-- .../test_endpoint_trace_item_table.py | 29 +++++++------------ 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index eb3f94594f..89ec908c11 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -95,12 +95,21 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: - """Return a Tuple(Array(UInt32), UInt8) sort key for semantic-version ORDER BY. + """Return a Tuple(Array(UInt32), UInt8, String) sort key for semantic-version ORDER BY. Strips a leading 'package@' prefix, isolates the release part (before '-'), maps each dot-component to UInt32, pads to 4 elements so "1.2" == "1.2.0", and adds a stability flag (0=prerelease, 1=stable) so prerelease versions sort before their corresponding stable release. Works on Altinity 25.3/25.8. + + The third tuple element is the raw (non-null) string. It is a tiebreaker: + distinct strings that map to the same numeric key + stability flag (e.g. "1.2" + and "1.2.0", or two prereleases of the same version) would otherwise compare + equal, which makes ordering non-deterministic and — more importantly — lets + the flextime pagination's strict `less` boundary filter skip rows tied with + the cursor. Appending the raw string gives a deterministic total order over + distinct release strings and keeps the page-boundary comparison exact. Both + ORDER BY and pagination call this function, so they stay consistent. """ x = Argument(None, "x") # sentry.release is coalesced from multiple attribute columns and therefore @@ -117,7 +126,7 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: literal(_SEMVER_COMPONENT_COUNT), ) is_stable = f.equals(f.position(version_no_prefix, literal("-")), literal(0)) - return FunctionCall(alias, "tuple", (numeric_key, is_stable)) + return FunctionCall(alias, "tuple", (numeric_key, is_stable, non_null)) def _trace_item_filter_key_expression( diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index 81612c3329..7acb8e8cdb 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -1316,12 +1316,15 @@ def test_expression_structure(self) -> None: expr = semver_sort_key(snuba_column("release")) assert isinstance(expr, FunctionCall) assert expr.function_name == "tuple" - assert len(expr.parameters) == 2 - numeric_key_expr, is_stable_expr = expr.parameters + assert len(expr.parameters) == 3 + numeric_key_expr, is_stable_expr, raw_str_expr = expr.parameters assert isinstance(numeric_key_expr, FunctionCall) assert numeric_key_expr.function_name == "arrayResize" assert isinstance(is_stable_expr, FunctionCall) assert is_stable_expr.function_name == "equals" + # Third element is the raw (non-null) string tiebreaker: ifNull(expr, ''). + assert isinstance(raw_str_expr, FunctionCall) + assert raw_str_expr.function_name == "ifNull" def test_alias_is_forwarded(self) -> None: from snuba.query.dsl import column as snuba_column diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index 14ae1dd31a..14747b1156 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4697,30 +4697,21 @@ def test_length_normalisation(self) -> None: releases = self._query_releases() idx_12 = releases.index("1.2") idx_120 = releases.index("1.2.0") - assert abs(idx_12 - idx_120) <= 1, ( - "1.2 and 1.2.0 should sort as equal (both normalise to [1,2,0,0])" + # Both normalise to [1,2,0,0], so they are adjacent. The raw-string + # tiebreaker then breaks the tie deterministically: "1.2" < "1.2.0". + assert idx_120 - idx_12 == 1, ( + "1.2 and 1.2.0 normalise equally and must be adjacent, with '1.2' " + "first via the raw-string tiebreaker" ) def test_desc_is_reverse_of_asc(self) -> None: + # The raw-string tiebreaker in semver_sort_key gives distinct release + # strings a deterministic total order (e.g. "1.2" < "1.2.0"), so DESC is + # the exact reverse of ASC even for versions that share the same numeric + # key. asc = self._query_releases(descending=False) desc = self._query_releases(descending=True) - # "1.2" and "1.2.0" share the same sort key ([1,2,0,0], 1); ClickHouse - # may return them in either relative order within the tied pair. - # Collapse contiguous tied elements into a frozenset before comparing so - # the assertion is order-insensitive within ties. - TIED: frozenset[str] = frozenset({"1.2", "1.2.0"}) - - def _canonicalize(releases: list[str]) -> list[object]: - out: list[object] = [] - for r in releases: - if r in TIED: - if not out or out[-1] != TIED: - out.append(TIED) - else: - out.append(r) - return out - - assert _canonicalize(asc) == _canonicalize(list(reversed(desc))) + assert asc == list(reversed(desc)) def test_uniq_with_default_value_double_casts_to_float64() -> None: From 94eea20edee506974565d9b9bd9cb8b42c591995 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 29 Jun 2026 21:57:57 +0000 Subject: [PATCH 13/27] Remove test_uniq_with_default_value_double_casts_to_float64 Master reverted the "Cast aggregation to Float64 in coalesce" change (#7908), which deleted this regression test and the FunctionCall import from the test module. The merge incorrectly kept the test, leaving it referencing an undefined FunctionCall (ruff F821 / mypy) and asserting reverted behavior. Drop it to match master. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- .../test_endpoint_trace_item_table.py | 50 ------------------- 1 file changed, 50 deletions(-) diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index 7d703fb750..423663821b 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4610,53 +4610,3 @@ def test_desc_is_reverse_of_asc(self) -> None: asc = self._query_releases(descending=False) desc = self._query_releases(descending=True) assert asc == list(reversed(desc)) - - -def test_uniq_with_default_value_double_casts_to_float64() -> None: - """ - Regression test: FUNCTION_UNIQ (uniqIfOrNull) returns UInt64, which is - incompatible with Float64 in ClickHouse's coalesce(). When - default_value_double is set, the aggregation must be CAST to Float64. - """ - request = TraceItemTableRequest( - meta=RequestMeta( - project_ids=[1], - organization_id=1, - trace_item_type=TraceItemType.TRACE_ITEM_TYPE_SPAN, - ), - columns=[ - Column( - conditional_aggregation=AttributeConditionalAggregation( - aggregate=Function.FUNCTION_UNIQ, - key=AttributeKey(type=AttributeKey.TYPE_STRING, name="user"), - label="count_unique(user)", - extrapolation_mode=ExtrapolationMode.EXTRAPOLATION_MODE_NONE, - default_value_double=0.0, - ), - label="count_unique(user)", - ), - ], - ) - - wrapper = TraceItemTableRequestWrapper(request) - wrapper.accept(AggregationToConditionalAggregationVisitor()) - request = _apply_labels_to_columns(request) - - query = build_query(request) - selected = query.get_selected_columns() - # Find the count_unique(user) column - target_expr = None - for sel in selected: - if sel.name == "count_unique(user)": - target_expr = sel.expression - break - assert target_expr is not None, "count_unique(user) column not found in query" - - # The expression should be coalesce(CAST(..., 'Float64'), 0.0) - assert isinstance(target_expr, FunctionCall) - assert target_expr.function_name == "coalesce" - cast_arg = target_expr.parameters[0] - assert isinstance(cast_arg, FunctionCall) - assert cast_arg.function_name == "CAST", ( - f"Expected CAST as first arg of coalesce, got {cast_arg.function_name}" - ) From d3210eb6a4bbf001942ace1b17c80a6db7f6f09b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 03:46:32 +0000 Subject: [PATCH 14/27] feat(eap): respect SORT_NATURAL on attribute values endpoint Update sentry-protos to 0.39.0, which adds the OrderBy `sort` field (SORT_UNSPECIFIED/SORT_DEFAULT/SORT_NATURAL) to the attribute names and values requests (sentry-protos#334), and honor it in the attribute values endpoint. - Add natural_sort_key() to common.py: a general natural-order key that tokenizes into digit / non-digit runs and zero-pads digit runs so a lexicographic comparison matches natural order (e.g. "1.2.9" before "1.2.10", "item2" before "item10"). Built from primitives since naturalSortKey() is unavailable on Altinity 25.3/25.8. - trace_item_attribute_values: when order_by.sort == SORT_NATURAL, order the value column by natural_sort_key; unset/SORT_DEFAULT keeps the historical lexicographic order. count() remains the primary ordering. - Unit tests for natural_sort_key structure and integration tests contrasting natural vs lexicographic value ordering. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- pyproject.toml | 2 +- snuba/web/rpc/common/common.py | 41 +++++++++++++++++++ .../web/rpc/v1/trace_item_attribute_values.py | 14 ++++++- tests/web/rpc/test_common.py | 35 ++++++++++++++++ .../v1/test_trace_item_attribute_values_v1.py | 40 ++++++++++++++++++ uv.lock | 6 +-- 6 files changed, 133 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 2d7ed63e83..1850839f44 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,7 +33,7 @@ dependencies = [ "sentry-conventions>=0.15.0", "sentry-kafka-schemas>=2.1.36", "sentry-options>=1.1.1", - "sentry-protos>=0.34.0", + "sentry-protos>=0.39.0", "sentry-redis-tools>=0.5.1", "sentry-relay>=0.9.25", "sentry-sdk>=2.35.0", diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index 7d18a657f4..d6944c94fc 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -94,6 +94,47 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release"}) +# Zero-pad width for digit runs in natural_sort_key. Numbers with more digits +# than this still order correctly (see natural_sort_key), this is just the +# minimum alignment width. +_NATURAL_SORT_DIGIT_WIDTH = 20 + + +def natural_sort_key(expr: Expression, alias: str | None = None) -> Expression: + """Return a String sort key giving natural ordering for ``SORT_NATURAL``. + + Natural order compares embedded digit runs numerically, so ``"item2"`` sorts + before ``"item10"`` (plain lexicographic gives the reverse). Upstream + ClickHouse gained ``naturalSortKey()`` only in v26.3, which is unavailable on + Altinity 25.3/25.8, so the key is built from primitives: tokenize into + maximal digit / non-digit runs, left-pad each digit run with zeros so a + lexicographic comparison of the rebuilt string matches natural order. + """ + tok = Argument(None, "t") + non_null = f.ifNull(expr, literal("")) + # ``[0-9]+|[^0-9]+`` matches every character exactly once, so extractAll + # yields the runs in order with nothing dropped (unlike splitByRegexp, which + # discards the separators). + tokens = f.extractAll(non_null, literal("[0-9]+|[^0-9]+")) + # Pad to at least _NATURAL_SORT_DIGIT_WIDTH but never below the token's own + # length, so longer numbers are never truncated and still sort after shorter + # ones (a bigger magnitude keeps a longer, hence lexicographically greater, + # zero-padded form). + padded_digits = f.leftPad( + tok, + f.greatest(f.length(tok), literal(_NATURAL_SORT_DIGIT_WIDTH)), + literal("0"), + ) + padded = f.arrayMap( + Lambda( + None, + ("t",), + FunctionCall(None, "if", (f.match(tok, literal("^[0-9]")), padded_digits, tok)), + ), + tokens, + ) + return FunctionCall(alias, "arrayStringConcat", (padded, literal(""))) + def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: """Return a Tuple(Array(UInt32), UInt8, String) sort key for semantic-version ORDER BY. diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index f569c44353..15798da165 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -30,6 +30,7 @@ add_existence_check_to_subscriptable_references, attribute_key_to_expression, base_conditions_and, + natural_sort_key, treeify_or_and_conditions, ) from snuba.web.rpc.common.exceptions import BadSnubaRPCRequestException @@ -136,6 +137,17 @@ def _build_query( ) treeify_or_and_conditions(inner_query) add_existence_check_to_subscriptable_references(inner_query) + # The value column normally orders lexicographically. When the caller opts + # into SORT_NATURAL (sentry-protos#334), order it by a natural-sort key so + # embedded digit runs compare numerically (e.g. "1.2.9" before "1.2.10"). + # count() stays the primary ordering so the most common values still come + # first; the natural key only changes the tiebreak among equally frequent + # values. An unset/SORT_DEFAULT sort keeps the historical lexicographic order. + if request.order_by.sort == TraceItemAttributeValuesRequest.OrderBy.SORT_NATURAL: + value_order_expression: Expression = natural_sort_key(column("attr_value")) + else: + value_order_expression = column("attr_value") + res = CompositeQuery( from_clause=inner_query, selected_columns=[ @@ -154,7 +166,7 @@ def _build_query( ], order_by=[ OrderBy(direction=OrderByDirection.DESC, expression=column("count()")), - OrderBy(direction=OrderByDirection.ASC, expression=column("attr_value")), + OrderBy(direction=OrderByDirection.ASC, expression=value_order_expression), ], groupby=[column("attr_value")], limit=request.limit, diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index 69cd1eed19..4679fb9e7d 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -54,6 +54,7 @@ _any_attribute_filter_to_expression, attribute_key_to_expression, dedupe_and_conditions, + natural_sort_key, next_monday, prev_monday, semver_sort_key, @@ -1341,6 +1342,40 @@ def test_no_alias_by_default(self) -> None: assert expr.alias is None +class TestNaturalSortKey: + def test_expression_structure(self) -> None: + from snuba.query.dsl import column as snuba_column + + expr = natural_sort_key(snuba_column("attr_value")) + # arrayStringConcat(arrayMap(...), '') + assert isinstance(expr, FunctionCall) + assert expr.function_name == "arrayStringConcat" + assert len(expr.parameters) == 2 + array_map_expr, sep_expr = expr.parameters + assert isinstance(array_map_expr, FunctionCall) + assert array_map_expr.function_name == "arrayMap" + assert isinstance(sep_expr, Literal) + assert sep_expr.value == "" + # The mapped array is extractAll(ifNull(expr, ''), '[0-9]+|[^0-9]+'). + lam, tokens_expr = array_map_expr.parameters + assert isinstance(lam, Lambda) + assert isinstance(tokens_expr, FunctionCall) + assert tokens_expr.function_name == "extractAll" + + def test_alias_is_forwarded(self) -> None: + from snuba.query.dsl import column as snuba_column + + expr = natural_sort_key(snuba_column("attr_value"), alias="natural_key") + assert isinstance(expr, FunctionCall) + assert expr.alias == "natural_key" + + def test_no_alias_by_default(self) -> None: + from snuba.query.dsl import column as snuba_column + + expr = natural_sort_key(snuba_column("attr_value")) + assert expr.alias is None + + class TestAnyAttributeFilterOption: """The `enable_any_attribute_filter` sentry-option gates whether any_attribute_filter is translated into a predicate or treated as diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index 3467e698bd..951217f72f 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -119,6 +119,21 @@ def setup_teardown(eap: None, redis_db: None) -> Generator[list[bytes]]: "sentry.transaction": AnyValue(string_value="*foo"), }, ), + # Distinct version-like values (one occurrence each) used to exercise + # natural vs lexicographic ordering. Lexicographically "1.2.10" < "1.2.2" + # < "1.2.9"; naturally "1.2.2" < "1.2.9" < "1.2.10". + gen_item_message( + start_timestamp=start_timestamp, + attributes={"natural_ver": AnyValue(string_value="1.2.9")}, + ), + gen_item_message( + start_timestamp=start_timestamp, + attributes={"natural_ver": AnyValue(string_value="1.2.10")}, + ), + gen_item_message( + start_timestamp=start_timestamp, + attributes={"natural_ver": AnyValue(string_value="1.2.2")}, + ), ] write_raw_unprocessed_events(items_storage, messages) yield messages @@ -148,6 +163,31 @@ def test_simple_case(self, setup_teardown: Any) -> None: assert response.values == ["derpderp", "blah", "durp", "herp", "herpderp"] assert response.counts == [2, 1, 1, 1, 1] + def test_natural_sort(self, setup_teardown: Any) -> None: + # SORT_NATURAL orders embedded digit runs numerically. + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=10, + key=AttributeKey(name="natural_ver", type=AttributeKey.TYPE_STRING), + order_by=TraceItemAttributeValuesRequest.OrderBy( + column=TraceItemAttributeValuesRequest.OrderBy.COLUMN_VALUE, + sort=TraceItemAttributeValuesRequest.OrderBy.SORT_NATURAL, + ), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == ["1.2.2", "1.2.9", "1.2.10"] + + def test_default_sort_is_lexicographic(self, setup_teardown: Any) -> None: + # Unset sort keeps the historical lexicographic ordering, where "1.2.10" + # sorts before "1.2.2" and "1.2.9". + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=10, + key=AttributeKey(name="natural_ver", type=AttributeKey.TYPE_STRING), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == ["1.2.10", "1.2.2", "1.2.9"] + def test_with_value_substring_match(self, setup_teardown: Any) -> None: message = TraceItemAttributeValuesRequest( meta=COMMON_META, diff --git a/uv.lock b/uv.lock index 8df52ba2ee..d38f8e8d0f 100644 --- a/uv.lock +++ b/uv.lock @@ -1023,7 +1023,7 @@ wheels = [ [[package]] name = "sentry-protos" -version = "0.34.0" +version = "0.39.0" source = { registry = "https://pypi.devinfra.sentry.io/simple" } dependencies = [ { name = "grpc-stubs", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, @@ -1031,7 +1031,7 @@ dependencies = [ { name = "protobuf", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, ] wheels = [ - { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.34.0-py3-none-any.whl", hash = "sha256:917fc77bc17662680fbbb93ed056de5a44201982c367a1179842e355ae5ca244" }, + { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.39.0-py3-none-any.whl", hash = "sha256:741710bb31099da630e09725f129af4afb3dcb9942f063264197313008cf8791" }, ] [[package]] @@ -1234,7 +1234,7 @@ requires-dist = [ { name = "sentry-conventions", specifier = ">=0.15.0" }, { name = "sentry-kafka-schemas", specifier = ">=2.1.36" }, { name = "sentry-options", specifier = ">=1.1.1" }, - { name = "sentry-protos", specifier = ">=0.34.0" }, + { name = "sentry-protos", specifier = ">=0.39.0" }, { name = "sentry-redis-tools", specifier = ">=0.5.1" }, { name = "sentry-relay", specifier = ">=0.9.25" }, { name = "sentry-sdk", specifier = ">=2.35.0" }, From f8741ab4d549d772dc333beb6fd282c7e176fa1f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 04:06:01 +0000 Subject: [PATCH 15/27] test: write natural-sort fixtures per-test, not in shared fixture The 3 version-valued items added to the autouse fixture defaulted to sentry.is_segment=true, inflating test_boolean_case's count from 8 to 11. Move them into the two natural-sort tests via a helper so they don't perturb count-based assertions in sibling tests. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- .../v1/test_trace_item_attribute_values_v1.py | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index 951217f72f..b0cf7d018f 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -119,21 +119,6 @@ def setup_teardown(eap: None, redis_db: None) -> Generator[list[bytes]]: "sentry.transaction": AnyValue(string_value="*foo"), }, ), - # Distinct version-like values (one occurrence each) used to exercise - # natural vs lexicographic ordering. Lexicographically "1.2.10" < "1.2.2" - # < "1.2.9"; naturally "1.2.2" < "1.2.9" < "1.2.10". - gen_item_message( - start_timestamp=start_timestamp, - attributes={"natural_ver": AnyValue(string_value="1.2.9")}, - ), - gen_item_message( - start_timestamp=start_timestamp, - attributes={"natural_ver": AnyValue(string_value="1.2.10")}, - ), - gen_item_message( - start_timestamp=start_timestamp, - attributes={"natural_ver": AnyValue(string_value="1.2.2")}, - ), ] write_raw_unprocessed_events(items_storage, messages) yield messages @@ -163,8 +148,26 @@ def test_simple_case(self, setup_teardown: Any) -> None: assert response.values == ["derpderp", "blah", "durp", "herp", "herpderp"] assert response.counts == [2, 1, 1, 1, 1] + def _write_version_values(self) -> None: + # Distinct version-like values (one occurrence each). Lexicographically + # "1.2.10" < "1.2.2" < "1.2.9"; naturally "1.2.2" < "1.2.9" < "1.2.10". + # Written per-test (not in the shared fixture) so the extra items don't + # perturb the count-based assertions in other tests. + items_storage = get_writable_storage(StorageKey("eap_items")) + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={"natural_ver": AnyValue(string_value=v)}, + ) + for v in ("1.2.9", "1.2.10", "1.2.2") + ], + ) + def test_natural_sort(self, setup_teardown: Any) -> None: # SORT_NATURAL orders embedded digit runs numerically. + self._write_version_values() message = TraceItemAttributeValuesRequest( meta=COMMON_META, limit=10, @@ -180,6 +183,7 @@ def test_natural_sort(self, setup_teardown: Any) -> None: def test_default_sort_is_lexicographic(self, setup_teardown: Any) -> None: # Unset sort keeps the historical lexicographic ordering, where "1.2.10" # sorts before "1.2.2" and "1.2.9". + self._write_version_values() message = TraceItemAttributeValuesRequest( meta=COMMON_META, limit=10, From a9dbc5a4500b96a4c42bb1a4a4fd2fe762dec21e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 17:09:44 +0000 Subject: [PATCH 16/27] feat(eap): make SORT_NATURAL release-aware for release attributes SORT_NATURAL now selects the appropriate sort key per attribute: release attributes (SEMVER_SORT_ATTRIBUTES, e.g. sentry.release) use the semver key (prerelease before stable, "pkg@" prefix stripped), while every other attribute uses the general natural-sort key (digit runs compared numerically). This gives both a general natural sort and a release-aware sort under the single SORT_NATURAL option the proto exposes today. A dedicated proto value (e.g. SORT_RELEASE) would let clients pick semver ordering explicitly for any attribute; that needs a sentry-protos change and can be wired here once it lands. Add an integration test asserting the semver ordering for sentry.release values (prerelease-before-stable, prefix stripped). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- .../web/rpc/v1/trace_item_attribute_values.py | 19 ++++++---- .../v1/test_trace_item_attribute_values_v1.py | 35 +++++++++++++++++++ 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index 15798da165..fbc8ba86f0 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -27,10 +27,12 @@ from snuba.web.query import run_query from snuba.web.rpc import RPCEndpoint from snuba.web.rpc.common.common import ( + SEMVER_SORT_ATTRIBUTES, add_existence_check_to_subscriptable_references, attribute_key_to_expression, base_conditions_and, natural_sort_key, + semver_sort_key, treeify_or_and_conditions, ) from snuba.web.rpc.common.exceptions import BadSnubaRPCRequestException @@ -138,13 +140,18 @@ def _build_query( treeify_or_and_conditions(inner_query) add_existence_check_to_subscriptable_references(inner_query) # The value column normally orders lexicographically. When the caller opts - # into SORT_NATURAL (sentry-protos#334), order it by a natural-sort key so - # embedded digit runs compare numerically (e.g. "1.2.9" before "1.2.10"). - # count() stays the primary ordering so the most common values still come - # first; the natural key only changes the tiebreak among equally frequent - # values. An unset/SORT_DEFAULT sort keeps the historical lexicographic order. + # into SORT_NATURAL (sentry-protos#334), order it so embedded digit runs + # compare numerically (e.g. "1.2.9" before "1.2.10"). Release attributes use + # the release-aware semver key (prerelease before stable, "pkg@" prefix + # stripped); every other attribute uses the general natural-sort key. count() + # stays the primary ordering so the most common values still come first; the + # sort key only changes the tiebreak among equally frequent values. An + # unset/SORT_DEFAULT sort keeps the historical lexicographic order. if request.order_by.sort == TraceItemAttributeValuesRequest.OrderBy.SORT_NATURAL: - value_order_expression: Expression = natural_sort_key(column("attr_value")) + if request.key.name in SEMVER_SORT_ATTRIBUTES: + value_order_expression: Expression = semver_sort_key(column("attr_value")) + else: + value_order_expression = natural_sort_key(column("attr_value")) else: value_order_expression = column("attr_value") diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index b0cf7d018f..277809634e 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -192,6 +192,41 @@ def test_default_sort_is_lexicographic(self, setup_teardown: Any) -> None: response = AttributeValuesRequest().execute(message) assert response.values == ["1.2.10", "1.2.2", "1.2.9"] + def test_release_sort_is_semver_aware(self, setup_teardown: Any) -> None: + # sentry.release is a release attribute, so SORT_NATURAL uses the + # semver-aware key: prerelease sorts before its stable release and the + # "pkg@" prefix is stripped. (A general natural sort would instead put + # "1.2.3" before "1.2.3-beta.1" and treat "my-pkg@2.0.0" as an "m" string.) + items_storage = get_writable_storage(StorageKey("eap_items")) + releases = ["1.2.3-beta.1", "1.2.3", "1.2.9", "1.2.10", "my-pkg@2.0.0"] + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={"sentry.release": AnyValue(string_value=r)}, + ) + for r in releases + ], + ) + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=10, + key=AttributeKey(name="sentry.release", type=AttributeKey.TYPE_STRING), + order_by=TraceItemAttributeValuesRequest.OrderBy( + column=TraceItemAttributeValuesRequest.OrderBy.COLUMN_VALUE, + sort=TraceItemAttributeValuesRequest.OrderBy.SORT_NATURAL, + ), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == [ + "1.2.3-beta.1", + "1.2.3", + "1.2.9", + "1.2.10", + "my-pkg@2.0.0", + ] + def test_with_value_substring_match(self, setup_teardown: Any) -> None: message = TraceItemAttributeValuesRequest( meta=COMMON_META, From 7a45d04033bb7db12a53e5f401fbe25c42b27bf5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 17:28:50 +0000 Subject: [PATCH 17/27] test: make release-sort test robust to default sentry.release gen_item_message stamps a default sentry.release on every fixture item, so that value shows up (with a higher count) alongside the releases the test writes. Assert only the relative semver order of the written releases rather than the full value list. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- tests/web/rpc/v1/test_trace_item_attribute_values_v1.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index 277809634e..5873da902a 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -219,7 +219,11 @@ def test_release_sort_is_semver_aware(self, setup_teardown: Any) -> None: ), ) response = AttributeValuesRequest().execute(message) - assert response.values == [ + # gen_item_message stamps a default sentry.release on every fixture item, + # so other release values may appear; assert only the relative order of + # the releases we wrote, which must follow semver ordering. + ordered = [v for v in response.values if v in set(releases)] + assert ordered == [ "1.2.3-beta.1", "1.2.3", "1.2.9", From 75edf1dfa5bdcccb358eaa38f579b3def719c767 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 17:33:28 +0000 Subject: [PATCH 18/27] fix(eap): treat PEP 440 dot-dev builds as prereleases in semver key is_stable previously only flagged SemVer '-' prereleases, so PEP 440 dot-style dev builds like "24.7.0.dev0+" (Sentry's own sentry.release format) were classified stable and sorted after their GA release. Define stable as a plain dotted-numeric version instead, so both "1.2.3-beta.1" and "24.7.0.dev0+" sort before their matching stable release. Add a table integration test asserting the dev build sorts before GA. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/common/common.py | 8 +++++++- .../test_endpoint_trace_item_table.py | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index d6944c94fc..c0f4d1c5e9 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -167,7 +167,13 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: ), literal(_SEMVER_COMPONENT_COUNT), ) - is_stable = f.equals(f.position(version_no_prefix, literal("-")), literal(0)) + # A release is "stable" only when the version is a plain dotted-numeric + # string ("1.2.3", "1.2"). Anything else is a prerelease and sorts before + # the matching stable release: SemVer prereleases ("1.2.3-beta.1") as well as + # PEP 440 dot-style dev/pre builds ("24.7.0.dev0+", the common form on + # sentry.release) — the latter has no '-', so a bare '-' check would wrongly + # tag it stable and sort the dev build after its GA release. + is_stable = f.match(version_no_prefix, literal(r"^[0-9]+(\.[0-9]+)*$")) return FunctionCall(alias, "tuple", (numeric_key, is_stable, non_null)) diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index 37eee3c8ee..0274b12b0f 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4525,6 +4525,10 @@ class TestSemverSorting: "my-pkg@2.0.0", "1.2", "1.2.0", + # PEP 440 dot-style dev build (Sentry's own sentry.release format) and + # its GA release, to verify the dev build sorts before GA. + "24.7.0.dev0+abc123", + "24.7.0", ] @pytest.fixture(autouse=True) @@ -4579,6 +4583,12 @@ def test_prerelease_before_stable(self) -> None: "prerelease 1.2.3-beta.1 must sort before stable 1.2.3" ) + def test_dot_dev_prerelease_before_stable(self) -> None: + releases = self._query_releases() + assert releases.index("24.7.0.dev0+abc123") < releases.index("24.7.0"), ( + "PEP 440 dot-style dev build must sort before its GA release" + ) + def test_prerelease_of_newer_after_older_stable(self) -> None: releases = self._query_releases() assert releases.index("1.2.3-beta.1") > releases.index("1.2.2"), ( From bc680babe6394c8dd4877e3fddc186766016770b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 17:35:18 +0000 Subject: [PATCH 19/27] test: update semver structure assertion to match() stability flag The is_stable expression changed from equals(position(...)) to match(version, ...) in 75edf1d; update the unit test's structural assertion accordingly. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- tests/web/rpc/test_common.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index 4679fb9e7d..c46c99816d 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -1322,8 +1322,10 @@ def test_expression_structure(self) -> None: numeric_key_expr, is_stable_expr, raw_str_expr = expr.parameters assert isinstance(numeric_key_expr, FunctionCall) assert numeric_key_expr.function_name == "arrayResize" + # Stability flag: match(version, '^[0-9]+(\\.[0-9]+)*$') — 1 for a plain + # dotted-numeric (stable) version, 0 for any prerelease form. assert isinstance(is_stable_expr, FunctionCall) - assert is_stable_expr.function_name == "equals" + assert is_stable_expr.function_name == "match" # Third element is the raw (non-null) string tiebreaker: ifNull(expr, ''). assert isinstance(raw_str_expr, FunctionCall) assert raw_str_expr.function_name == "ifNull" From 267d3b548681dbd9b1afe37567ec43caa5721f9b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 18:17:05 +0000 Subject: [PATCH 20/27] feat(eap): drive semver ordering via SORT_NATURAL on all endpoints Update sentry-protos to 0.40.0, which adds the OrderBy `sort` field to the trace-item-table request as well as the attribute names/values requests. Make SORT_NATURAL the client-driven, semver-aware ordering everywhere and remove the hardcoded attribute list. - common: remove SEMVER_SORT_ATTRIBUTES and the general natural_sort_key; semver_sort_key is now the single implementation of SORT_NATURAL. - table resolver: apply semver_sort_key in ORDER BY when a string column's OrderBy.sort == SORT_NATURAL (no attribute gating); GROUP BY keeps the raw expression. - flextime pagination: mark SORT_NATURAL page-boundary columns and apply the semver key on both sides so pages stay consistent with ORDER BY. - attribute values + names endpoints: order by the semver key when SORT_NATURAL is requested; the names endpoint mirrors the ordering in its Python merge/re-sort of synthetic keys. - tests: drive semver via SORT_NATURAL, assert default (unset) stays lexicographic, and cover the names endpoint. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- pyproject.toml | 2 +- snuba/web/rpc/common/common.py | 47 +--------- snuba/web/rpc/common/pagination.py | 89 ++++++++++++------- .../v1/endpoint_trace_item_attribute_names.py | 62 +++++++++++-- .../R_eap_items/resolver_trace_item_table.py | 28 +++--- .../web/rpc/v1/trace_item_attribute_values.py | 19 ++-- tests/web/rpc/test_common.py | 35 -------- ...est_endpoint_trace_item_attribute_names.py | 33 +++++++ .../test_endpoint_trace_item_table.py | 23 ++++- .../v1/test_trace_item_attribute_values_v1.py | 9 +- uv.lock | 6 +- 11 files changed, 197 insertions(+), 156 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 1850839f44..d2efb29b63 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,7 +33,7 @@ dependencies = [ "sentry-conventions>=0.15.0", "sentry-kafka-schemas>=2.1.36", "sentry-options>=1.1.1", - "sentry-protos>=0.39.0", + "sentry-protos>=0.40.0", "sentry-redis-tools>=0.5.1", "sentry-relay>=0.9.25", "sentry-sdk>=2.35.0", diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index c0f4d1c5e9..554018c5ea 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -92,53 +92,14 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: _SEMVER_COMPONENT_COUNT = 4 # major.minor.patch.build -SEMVER_SORT_ATTRIBUTES: frozenset[str] = frozenset({"sentry.release"}) - -# Zero-pad width for digit runs in natural_sort_key. Numbers with more digits -# than this still order correctly (see natural_sort_key), this is just the -# minimum alignment width. -_NATURAL_SORT_DIGIT_WIDTH = 20 - - -def natural_sort_key(expr: Expression, alias: str | None = None) -> Expression: - """Return a String sort key giving natural ordering for ``SORT_NATURAL``. - - Natural order compares embedded digit runs numerically, so ``"item2"`` sorts - before ``"item10"`` (plain lexicographic gives the reverse). Upstream - ClickHouse gained ``naturalSortKey()`` only in v26.3, which is unavailable on - Altinity 25.3/25.8, so the key is built from primitives: tokenize into - maximal digit / non-digit runs, left-pad each digit run with zeros so a - lexicographic comparison of the rebuilt string matches natural order. - """ - tok = Argument(None, "t") - non_null = f.ifNull(expr, literal("")) - # ``[0-9]+|[^0-9]+`` matches every character exactly once, so extractAll - # yields the runs in order with nothing dropped (unlike splitByRegexp, which - # discards the separators). - tokens = f.extractAll(non_null, literal("[0-9]+|[^0-9]+")) - # Pad to at least _NATURAL_SORT_DIGIT_WIDTH but never below the token's own - # length, so longer numbers are never truncated and still sort after shorter - # ones (a bigger magnitude keeps a longer, hence lexicographically greater, - # zero-padded form). - padded_digits = f.leftPad( - tok, - f.greatest(f.length(tok), literal(_NATURAL_SORT_DIGIT_WIDTH)), - literal("0"), - ) - padded = f.arrayMap( - Lambda( - None, - ("t",), - FunctionCall(None, "if", (f.match(tok, literal("^[0-9]")), padded_digits, tok)), - ), - tokens, - ) - return FunctionCall(alias, "arrayStringConcat", (padded, literal(""))) - def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: """Return a Tuple(Array(UInt32), UInt8, String) sort key for semantic-version ORDER BY. + This is the sort applied for the ``SORT_NATURAL`` OrderBy option: callers opt + in per request (there is no hardcoded attribute list), and the key is applied + to whatever column they order by. + Strips a leading 'package@' prefix, isolates the release part (before '-'), maps each dot-component to UInt32, pads to 4 elements so "1.2" == "1.2.0", and adds a stability flag (0=prerelease, 1=stable) so prerelease versions sort diff --git a/snuba/web/rpc/common/pagination.py b/snuba/web/rpc/common/pagination.py index 81968e07ee..ec7c826179 100644 --- a/snuba/web/rpc/common/pagination.py +++ b/snuba/web/rpc/common/pagination.py @@ -19,7 +19,6 @@ from snuba.query.dsl import column, literal from snuba.query.expressions import Expression from snuba.web.rpc.common.common import ( - SEMVER_SORT_ATTRIBUTES, attribute_key_to_expression, semver_sort_key, ) @@ -31,6 +30,9 @@ class FlexibleTimeWindowPageWithFilters: _TIME_WINDOW_START_KEY = f"{_TIME_WINDOW_PREFIX}.start_timestamp" _TIME_WINDOW_END_KEY = f"{_TIME_WINDOW_PREFIX}.end_timestamp" _FILTER_PREFIX = "sentry__filter" + # Marks a page-boundary column whose ORDER BY used SORT_NATURAL (semver), so + # get_filters applies the same semver key on both sides of the comparison. + _NATURAL_FILTER_PREFIX = "sentry__natural_filter" def __init__(self, page_token: PageToken): self._page_token = page_token @@ -74,46 +76,51 @@ def get_filters(self) -> Expression | None: column_names: list[str] = [] column_values: list[Expression] = [] + # Parallel to column_names: True when that column's ORDER BY used + # SORT_NATURAL, so the boundary comparison must use the semver key too. + column_is_natural: list[bool] = [] for filter in self.page_token.filter_offset.and_filter.filters: - if filter.HasField( - "comparison_filter" - ) and filter.comparison_filter.key.name.startswith(self._FILTER_PREFIX): - if filter.comparison_filter.key.name == f"{self._FILTER_PREFIX}.timestamp": - column_names.append("timestamp") - if filter.comparison_filter.value.HasField("val_str"): - column_values.append(f.toDateTime(filter.comparison_filter.value.val_str)) - elif filter.comparison_filter.value.HasField("val_double"): - column_values.append(literal(filter.comparison_filter.value.val_double)) - elif filter.comparison_filter.value.HasField("val_int"): - column_values.append(literal(filter.comparison_filter.value.val_int)) - - else: - # strip the _FILTER_PREFIX from the attribute key and the dot - column_names.append( - filter.comparison_filter.key.name[len(self._FILTER_PREFIX) + 1 :] - ) - column_values.append( - literal( - getattr( - filter.comparison_filter.value, - str(filter.comparison_filter.value.WhichOneof("value")), - ) + if not filter.HasField("comparison_filter"): + continue + key_name = filter.comparison_filter.key.name + is_natural = key_name.startswith(f"{self._NATURAL_FILTER_PREFIX}.") + is_regular = key_name.startswith(f"{self._FILTER_PREFIX}.") + if not (is_natural or is_regular): + continue + + if key_name == f"{self._FILTER_PREFIX}.timestamp": + column_names.append("timestamp") + column_is_natural.append(False) + if filter.comparison_filter.value.HasField("val_str"): + column_values.append(f.toDateTime(filter.comparison_filter.value.val_str)) + elif filter.comparison_filter.value.HasField("val_double"): + column_values.append(literal(filter.comparison_filter.value.val_double)) + elif filter.comparison_filter.value.HasField("val_int"): + column_values.append(literal(filter.comparison_filter.value.val_int)) + else: + # strip the matching prefix (and the dot) to recover the alias + prefix = self._NATURAL_FILTER_PREFIX if is_natural else self._FILTER_PREFIX + column_names.append(key_name[len(prefix) + 1 :]) + column_is_natural.append(is_natural) + column_values.append( + literal( + getattr( + filter.comparison_filter.value, + str(filter.comparison_filter.value.WhichOneof("value")), ) ) + ) # Assumes everything in the ORDER BY is ordered by DESC if column_names: col_exprs = [] val_exprs = [] - for c_name, c_value in zip(column_names, column_values, strict=True): - # c_name is the attribute expression alias: "{attr}.{Type.Name(type)}" - # For semver attributes (e.g. sentry.release_TYPE_STRING), apply the - # same semver sort key on both sides so the page boundary comparison - # uses the same ordering as ORDER BY. - attr_name = ( - c_name.removesuffix("_TYPE_STRING") if c_name.endswith("_TYPE_STRING") else None - ) - if attr_name in SEMVER_SORT_ATTRIBUTES: + for c_name, c_value, is_natural in zip( + column_names, column_values, column_is_natural, strict=True + ): + # For SORT_NATURAL columns, apply the same semver key on both sides + # so the page-boundary comparison uses the same ordering as ORDER BY. + if is_natural: col_exprs.append(semver_sort_key(column(c_name))) val_exprs.append(semver_sort_key(c_value)) else: @@ -195,22 +202,36 @@ def create( # find the attribute in the in_msg.columns attribute that has the same label as the `column` attribute in the order_by_clause # call `attribute_key_to_expression` on it and us its alias as the name of the AttributeKey in the ComparisonFilter attribute_expression = None + selected_key = None for selected_column in in_msg.columns: if selected_column.label == order_by_clause.column.label: attribute_expression = attribute_key_to_expression( selected_column.key ) + selected_key = selected_column.key break if attribute_expression is None: raise ValueError( f"No attribute expression found for column: {order_by_clause.column.label}" ) + # Mark the column as SORT_NATURAL (semver) when the request + # ordered it that way, so get_filters wraps both sides in the + # semver key and the page boundary matches the ORDER BY. Mirror + # the resolver's guard: only string columns get the semver key. + is_natural = ( + order_by_clause.sort + == TraceItemTableRequest.OrderBy.SORT_NATURAL + and selected_key is not None + and selected_key.type == AttributeKey.TYPE_STRING + ) + prefix = cls._NATURAL_FILTER_PREFIX if is_natural else cls._FILTER_PREFIX + filters.append( TraceItemFilter( comparison_filter=ComparisonFilter( key=AttributeKey( - name=f"{cls._FILTER_PREFIX}.{attribute_expression.alias}", + name=f"{prefix}.{attribute_expression.alias}", ), op=ComparisonFilter.OP_LESS_THAN, value=last_result_value, diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index dc737bd35d..8d3e179aaa 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -1,3 +1,4 @@ +import re import uuid from google.protobuf.json_format import MessageToDict @@ -33,6 +34,7 @@ next_monday, prev_monday, project_id_and_org_conditions, + semver_sort_key, treeify_or_and_conditions, ) from snuba.web.rpc.common.debug_info import extract_response_meta @@ -79,6 +81,41 @@ def _order_by_name_descending(request: TraceItemAttributeNamesRequest) -> bool: ) +def _order_by_natural(request: TraceItemAttributeNamesRequest) -> bool: + """Whether the caller requested SORT_NATURAL (semver) ordering of names.""" + return request.order_by.sort == TraceItemAttributeNamesRequest.OrderBy.SORT_NATURAL + + +_SEMVER_NUMERIC_RE = re.compile(r"^[0-9]+(\.[0-9]+)*$") + + +def _semver_sort_key_py(name: str) -> tuple[tuple[int, int, int, int], int, str]: + """Python mirror of common.semver_sort_key, used to re-sort names in Python so + the merged (ClickHouse + synthetic) result matches the ClickHouse ORDER BY.""" + non_null = name or "" + version_no_prefix = non_null.split("@")[-1] + release_part = version_no_prefix.split("-")[0] + components = [int(c) if c.isdigit() else 0 for c in release_part.split(".")] + components = (components + [0, 0, 0, 0])[:4] + is_stable = 1 if _SEMVER_NUMERIC_RE.match(version_no_prefix) else 0 + return ( + (components[0], components[1], components[2], components[3]), + is_stable, + non_null, + ) + + +def _name_order_by_expression(natural: bool) -> Expression: + """ClickHouse ORDER BY expression for the attribute name. + + Default orders by the raw (type, name) tuple; SORT_NATURAL orders by the + semver key of the name part so versions sort numerically. + """ + if natural: + return semver_sort_key(f.tupleElement(column("attr_key"), 2)) + return column("attr_key") + + class AttributeKeyCollector(ProtoVisitor): def __init__(self) -> None: self.keys: set[str] = set() @@ -344,6 +381,7 @@ def get_co_occurring_attributes( alias="attr_key", ) + natural = _order_by_natural(request) if _order_by_count(request): # Opt-in frequency ordering: group by key and count how many rows # (co-occurring attribute sets) contain each key. @@ -359,8 +397,9 @@ def get_co_occurring_attributes( ), expression=column("count"), ), - # stable tiebreak for keys with the same frequency - OrderBy(direction=OrderByDirection.ASC, expression=column("attr_key")), + # stable tiebreak for keys with the same frequency (semver key when + # SORT_NATURAL was requested) + OrderBy(direction=OrderByDirection.ASC, expression=_name_order_by_expression(natural)), ] else: # Default (order_by unset or COLUMN_NAME): distinct keys ordered by name. @@ -374,7 +413,7 @@ def get_co_occurring_attributes( order_by = [ OrderBy( direction=OrderByDirection.DESC if name_descending else OrderByDirection.ASC, - expression=column("attr_key"), + expression=_name_order_by_expression(natural), ), ] @@ -428,6 +467,17 @@ def t(row: Row) -> TraceItemAttributeNamesResponse.Attribute: attribute.count = int(count) return attribute + # Name-ordering key that mirrors the ClickHouse ORDER BY: the raw (type, name) + # tuple by default, or the semver key of the name under SORT_NATURAL. + natural = _order_by_natural(request) + + def _name_key(row: dict) -> object: + attr_key = row.get("attr_key", ("TYPE_STRING", "")) + attr_type, attr_name = attr_key[0], attr_key[1] + if natural: + return _semver_sort_key_py(attr_name) + return (attr_type, attr_name) + data = query_res.result.get("data", []) if request.type in (AttributeKey.TYPE_UNSPECIFIED, AttributeKey.TYPE_STRING): non_stored = [ @@ -435,13 +485,13 @@ def t(row: Row) -> TraceItemAttributeNamesResponse.Attribute: for key_name in NON_STORED_ATTRIBUTE_KEYS if request.value_substring_match in key_name ] - non_stored.sort(key=lambda row: tuple(row["attr_key"])) + non_stored.sort(key=_name_key) if _order_by_count(request): # Order the real (counted) rows to match ClickHouse: count in the # requested direction, then name ASC (two stable passes). The synthetic # non-stored attributes have no real count, so pin them first regardless # of sort direction rather than relying on a sentinel value. - data.sort(key=lambda row: tuple(row.get("attr_key", ("TYPE_STRING", "")))) + data.sort(key=_name_key) data.sort(key=lambda row: row.get("count", 0), reverse=request.order_by.descending) data = non_stored + data else: @@ -450,7 +500,7 @@ def t(row: Row) -> TraceItemAttributeNamesResponse.Attribute: # ORDER BY (a COLUMN_NAME descending request must stay descending here too). data.extend(non_stored) data.sort( - key=lambda row: tuple(row.get("attr_key", ("TYPE_STRING", ""))), + key=_name_key, reverse=_order_by_name_descending(request), ) diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index d436a0dc4e..cec19afed4 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -60,7 +60,6 @@ from snuba.utils.metrics.timer import Timer from snuba.web.query import run_query from snuba.web.rpc.common.common import ( - SEMVER_SORT_ATTRIBUTES, add_existence_check_to_subscriptable_references, attribute_key_to_expression, base_conditions_and, @@ -278,7 +277,7 @@ def aggregation_filter_to_expression( raise BadSnubaRPCRequestException(f"Unsupported aggregation filter type: {default}") -def _groupby_order_by_expression(attr_key: AttributeKey, for_order_by: bool = False) -> Expression: +def _groupby_order_by_expression(attr_key: AttributeKey) -> Expression: """ Maps an attribute key used in GROUP BY / ORDER BY to its expression. @@ -288,16 +287,13 @@ def _groupby_order_by_expression(attr_key: AttributeKey, for_order_by: bool = Fa rejected upstream (see _validate_select_and_groupby / _validate_order_by), so they never reach here. - For `SEMVER_SORT_ATTRIBUTES` (e.g. `sentry.release`), the ORDER BY uses - a semver tuple key while GROUP BY keeps the raw expression. ClickHouse - accepts ORDER BY on a function of a GROUP BY key, so the two can differ here. + The SORT_NATURAL (semver) ordering is applied by `_convert_order_by`, not here, so + GROUP BY keeps the raw expression while ORDER BY can wrap it in the semver key. + ClickHouse accepts ORDER BY on a function of a GROUP BY key, so the two can differ. """ if attr_key.name == "sentry.timestamp": return snuba_column("timestamp") - base = attribute_key_to_expression(attr_key) - if for_order_by and attr_key.name in SEMVER_SORT_ATTRIBUTES: - return semver_sort_key(base) - return base + return attribute_key_to_expression(attr_key) def _convert_order_by( @@ -348,12 +344,20 @@ def _convert_order_by( # no-groupby branch above already expands to the full table sort key; this # covers `sentry.timestamp` ordering anywhere else.) GROUP BY uses the same # expression so an aggregation query that orders by `sentry.timestamp` stays - # valid. For `SEMVER_SORT_ATTRIBUTES`, `for_order_by=True` selects the semver - # tuple key here while GROUP BY keeps the raw expression. + # valid. + expression = _groupby_order_by_expression(x.column.key) + # SORT_NATURAL requests semver-aware ordering (client-driven, no hardcoded + # attribute list). It only applies to string columns; numeric/timestamp + # columns already sort numerically, so leave them untouched. + if ( + x.sort == TraceItemTableRequest.OrderBy.SORT_NATURAL + and x.column.key.type == AttributeKey.TYPE_STRING + ): + expression = semver_sort_key(expression) res.append( OrderBy( direction=direction, - expression=_groupby_order_by_expression(x.column.key, for_order_by=True), + expression=expression, ) ) elif x.column.HasField("conditional_aggregation"): diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index fbc8ba86f0..683897a21a 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -27,11 +27,9 @@ from snuba.web.query import run_query from snuba.web.rpc import RPCEndpoint from snuba.web.rpc.common.common import ( - SEMVER_SORT_ATTRIBUTES, add_existence_check_to_subscriptable_references, attribute_key_to_expression, base_conditions_and, - natural_sort_key, semver_sort_key, treeify_or_and_conditions, ) @@ -140,18 +138,13 @@ def _build_query( treeify_or_and_conditions(inner_query) add_existence_check_to_subscriptable_references(inner_query) # The value column normally orders lexicographically. When the caller opts - # into SORT_NATURAL (sentry-protos#334), order it so embedded digit runs - # compare numerically (e.g. "1.2.9" before "1.2.10"). Release attributes use - # the release-aware semver key (prerelease before stable, "pkg@" prefix - # stripped); every other attribute uses the general natural-sort key. count() - # stays the primary ordering so the most common values still come first; the - # sort key only changes the tiebreak among equally frequent values. An - # unset/SORT_DEFAULT sort keeps the historical lexicographic order. + # into SORT_NATURAL, order it by the semver key so versions sort numerically + # ("1.2.9" before "1.2.10") with prereleases before their stable release. + # count() stays the primary ordering so the most common values still come + # first; the sort key only changes the tiebreak among equally frequent values. + # An unset/SORT_DEFAULT sort keeps the historical lexicographic order. if request.order_by.sort == TraceItemAttributeValuesRequest.OrderBy.SORT_NATURAL: - if request.key.name in SEMVER_SORT_ATTRIBUTES: - value_order_expression: Expression = semver_sort_key(column("attr_value")) - else: - value_order_expression = natural_sort_key(column("attr_value")) + value_order_expression: Expression = semver_sort_key(column("attr_value")) else: value_order_expression = column("attr_value") diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index c46c99816d..b6d7bd6cd5 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -54,7 +54,6 @@ _any_attribute_filter_to_expression, attribute_key_to_expression, dedupe_and_conditions, - natural_sort_key, next_monday, prev_monday, semver_sort_key, @@ -1344,40 +1343,6 @@ def test_no_alias_by_default(self) -> None: assert expr.alias is None -class TestNaturalSortKey: - def test_expression_structure(self) -> None: - from snuba.query.dsl import column as snuba_column - - expr = natural_sort_key(snuba_column("attr_value")) - # arrayStringConcat(arrayMap(...), '') - assert isinstance(expr, FunctionCall) - assert expr.function_name == "arrayStringConcat" - assert len(expr.parameters) == 2 - array_map_expr, sep_expr = expr.parameters - assert isinstance(array_map_expr, FunctionCall) - assert array_map_expr.function_name == "arrayMap" - assert isinstance(sep_expr, Literal) - assert sep_expr.value == "" - # The mapped array is extractAll(ifNull(expr, ''), '[0-9]+|[^0-9]+'). - lam, tokens_expr = array_map_expr.parameters - assert isinstance(lam, Lambda) - assert isinstance(tokens_expr, FunctionCall) - assert tokens_expr.function_name == "extractAll" - - def test_alias_is_forwarded(self) -> None: - from snuba.query.dsl import column as snuba_column - - expr = natural_sort_key(snuba_column("attr_value"), alias="natural_key") - assert isinstance(expr, FunctionCall) - assert expr.alias == "natural_key" - - def test_no_alias_by_default(self) -> None: - from snuba.query.dsl import column as snuba_column - - expr = natural_sort_key(snuba_column("attr_value")) - assert expr.alias is None - - class TestAnyAttributeFilterOption: """The `enable_any_attribute_filter` sentry-option gates whether any_attribute_filter is translated into a predicate or treated as diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py index d29345f6a2..af321e5446 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py @@ -99,6 +99,39 @@ def test_basic(self) -> None: ) assert res.attributes == expected + def test_natural_sort(self) -> None: + # Version-like attribute names: lexicographically "1.2.10" < "1.2.2" < + # "1.2.9"; with SORT_NATURAL (semver) they order "1.2.2" < "1.2.9" < "1.2.10". + items_storage = get_writable_storage(StorageKey("eap_items")) + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={name: AnyValue(string_value="x")}, + ) + for name in ("1.2.9", "1.2.10", "1.2.2") + ], + ) + req = TraceItemAttributeNamesRequest( + meta=RequestMeta( + project_ids=[1, 2, 3], + organization_id=1, + cogs_category="something", + referrer="something", + start_timestamp=Timestamp(seconds=int((BASE_TIME - timedelta(days=1)).timestamp())), + end_timestamp=Timestamp(seconds=int((BASE_TIME + timedelta(days=1)).timestamp())), + ), + limit=100, + type=AttributeKey.Type.TYPE_STRING, + value_substring_match="1.2", + order_by=TraceItemAttributeNamesRequest.OrderBy( + sort=TraceItemAttributeNamesRequest.OrderBy.SORT_NATURAL, + ), + ) + res = EndpointTraceItemAttributeNames().execute(req) + assert [a.name for a in res.attributes] == ["1.2.2", "1.2.9", "1.2.10"] + def test_simple_float_backward_compat(self) -> None: req = TraceItemAttributeNamesRequest( meta=RequestMeta( diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index 0274b12b0f..70e21f59cf 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4511,9 +4511,10 @@ def test_order_by_bug() -> None: @pytest.mark.clickhouse_db @pytest.mark.redis_db class TestSemverSorting: - """ORDER BY sentry.release uses the tuple(arrayResize(…), is_stable) key so - versions sort numerically (1.2.9 before 1.2.10) with pre-releases before - their corresponding stable release. + """ORDER BY with the SORT_NATURAL option applies the semver key so versions + sort numerically (1.2.9 before 1.2.10) with pre-releases before their + corresponding stable release. Without SORT_NATURAL the ordering stays + lexicographic (there is no hardcoded per-attribute behavior). """ _RELEASES = [ @@ -4539,7 +4540,12 @@ def _setup(self, clickhouse_db: None, redis_db: None) -> None: raw_attributes={"sentry.release": release, "semver_test_marker": "1"}, ) - def _query_releases(self, descending: bool = False) -> list[str]: + def _query_releases(self, descending: bool = False, natural: bool = True) -> list[str]: + sort = ( + TraceItemTableRequest.OrderBy.SORT_NATURAL + if natural + else TraceItemTableRequest.OrderBy.SORT_UNSPECIFIED + ) message = TraceItemTableRequest( meta=RequestMeta( project_ids=[1], @@ -4564,6 +4570,7 @@ def _query_releases(self, descending: bool = False) -> list[str]: key=AttributeKey(type=AttributeKey.TYPE_STRING, name="sentry.release") ), descending=descending, + sort=sort, ) ], limit=len(self._RELEASES) + 10, @@ -4577,6 +4584,14 @@ def test_numeric_ordering(self) -> None: "1.2.9 must sort before 1.2.10 (numeric, not lexicographic)" ) + def test_default_sort_is_lexicographic(self) -> None: + # Without SORT_NATURAL there is no semver behavior (no hardcoded + # attributes), so plain lexicographic order applies: "1.2.10" < "1.2.9". + releases = self._query_releases(natural=False) + assert releases.index("1.2.10") < releases.index("1.2.9"), ( + "without SORT_NATURAL, ordering is lexicographic" + ) + def test_prerelease_before_stable(self) -> None: releases = self._query_releases() assert releases.index("1.2.3-beta.1") < releases.index("1.2.3"), ( diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index 5873da902a..6296d556be 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -166,7 +166,8 @@ def _write_version_values(self) -> None: ) def test_natural_sort(self, setup_teardown: Any) -> None: - # SORT_NATURAL orders embedded digit runs numerically. + # SORT_NATURAL applies the semver key, so version components sort + # numerically (1.2.2 < 1.2.9 < 1.2.10) regardless of the attribute. self._write_version_values() message = TraceItemAttributeValuesRequest( meta=COMMON_META, @@ -193,10 +194,8 @@ def test_default_sort_is_lexicographic(self, setup_teardown: Any) -> None: assert response.values == ["1.2.10", "1.2.2", "1.2.9"] def test_release_sort_is_semver_aware(self, setup_teardown: Any) -> None: - # sentry.release is a release attribute, so SORT_NATURAL uses the - # semver-aware key: prerelease sorts before its stable release and the - # "pkg@" prefix is stripped. (A general natural sort would instead put - # "1.2.3" before "1.2.3-beta.1" and treat "my-pkg@2.0.0" as an "m" string.) + # SORT_NATURAL is the semver sort, so prerelease sorts before its stable + # release and the "pkg@" prefix is stripped. items_storage = get_writable_storage(StorageKey("eap_items")) releases = ["1.2.3-beta.1", "1.2.3", "1.2.9", "1.2.10", "my-pkg@2.0.0"] write_raw_unprocessed_events( diff --git a/uv.lock b/uv.lock index d38f8e8d0f..aa0711b7c8 100644 --- a/uv.lock +++ b/uv.lock @@ -1023,7 +1023,7 @@ wheels = [ [[package]] name = "sentry-protos" -version = "0.39.0" +version = "0.40.0" source = { registry = "https://pypi.devinfra.sentry.io/simple" } dependencies = [ { name = "grpc-stubs", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, @@ -1031,7 +1031,7 @@ dependencies = [ { name = "protobuf", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, ] wheels = [ - { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.39.0-py3-none-any.whl", hash = "sha256:741710bb31099da630e09725f129af4afb3dcb9942f063264197313008cf8791" }, + { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.40.0-py3-none-any.whl", hash = "sha256:7705c5b409339ff670e6d9c5cbc9b19b03d9d8525c35bc09d47a162cba156fc9" }, ] [[package]] @@ -1234,7 +1234,7 @@ requires-dist = [ { name = "sentry-conventions", specifier = ">=0.15.0" }, { name = "sentry-kafka-schemas", specifier = ">=2.1.36" }, { name = "sentry-options", specifier = ">=1.1.1" }, - { name = "sentry-protos", specifier = ">=0.39.0" }, + { name = "sentry-protos", specifier = ">=0.40.0" }, { name = "sentry-redis-tools", specifier = ">=0.5.1" }, { name = "sentry-relay", specifier = ">=0.9.25" }, { name = "sentry-sdk", specifier = ">=2.35.0" }, From 56fac6c53806d78a0ee6e77ffbbd27b949bf09d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 18:21:52 +0000 Subject: [PATCH 21/27] fix: guard _semver_sort_key_py against non-ASCII Unicode digits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit str.isdigit() is True for characters like "²" that int() cannot parse, which would crash the names endpoint under SORT_NATURAL. Require c.isascii() too, matching ClickHouse toUInt32OrZero (ASCII decimal only, else 0). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index 8d3e179aaa..8a05bdf033 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -95,7 +95,12 @@ def _semver_sort_key_py(name: str) -> tuple[tuple[int, int, int, int], int, str] non_null = name or "" version_no_prefix = non_null.split("@")[-1] release_part = version_no_prefix.split("-")[0] - components = [int(c) if c.isdigit() else 0 for c in release_part.split(".")] + # Mirror ClickHouse toUInt32OrZero: only ASCII decimal parses, anything else + # (including Unicode digits like "²", where str.isdigit() is True but int() + # raises) maps to 0. + components = [ + int(c) if (c.isascii() and c.isdigit()) else 0 for c in release_part.split(".") + ] components = (components + [0, 0, 0, 0])[:4] is_stable = 1 if _SEMVER_NUMERIC_RE.match(version_no_prefix) else 0 return ( From d55cf19d741a46e0664cd8d7d92d5602f8ca931f Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Fri, 3 Jul 2026 18:27:48 +0000 Subject: [PATCH 22/27] [getsentry/action-github-commit] Auto commit --- snuba/web/rpc/common/pagination.py | 3 +-- snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/snuba/web/rpc/common/pagination.py b/snuba/web/rpc/common/pagination.py index ec7c826179..32f1650a0d 100644 --- a/snuba/web/rpc/common/pagination.py +++ b/snuba/web/rpc/common/pagination.py @@ -220,8 +220,7 @@ def create( # semver key and the page boundary matches the ORDER BY. Mirror # the resolver's guard: only string columns get the semver key. is_natural = ( - order_by_clause.sort - == TraceItemTableRequest.OrderBy.SORT_NATURAL + order_by_clause.sort == TraceItemTableRequest.OrderBy.SORT_NATURAL and selected_key is not None and selected_key.type == AttributeKey.TYPE_STRING ) diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index 8a05bdf033..d10b07f42b 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -98,9 +98,7 @@ def _semver_sort_key_py(name: str) -> tuple[tuple[int, int, int, int], int, str] # Mirror ClickHouse toUInt32OrZero: only ASCII decimal parses, anything else # (including Unicode digits like "²", where str.isdigit() is True but int() # raises) maps to 0. - components = [ - int(c) if (c.isascii() and c.isdigit()) else 0 for c in release_part.split(".") - ] + components = [int(c) if (c.isascii() and c.isdigit()) else 0 for c in release_part.split(".")] components = (components + [0, 0, 0, 0])[:4] is_stable = 1 if _SEMVER_NUMERIC_RE.match(version_no_prefix) else 0 return ( From 8ecbb2575aa996b29843ac2499f4b2ed36b45d39 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 18:30:21 +0000 Subject: [PATCH 23/27] fix(mypy): type _name_key with Mapping[str, Any] -> Any The re-sort key callback used a bare `dict` param (missing type args) and an `object` return, which mypy rejects for list.sort's key. Accept Mapping[str, Any] (covariant, fits both the synthetic dicts and Row) and return Any. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013TCwndq43WTDRf6jsERQHN --- snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index d10b07f42b..a3037576b8 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -1,5 +1,7 @@ import re import uuid +from collections.abc import Mapping +from typing import Any from google.protobuf.json_format import MessageToDict from sentry_protos.snuba.v1.endpoint_trace_item_attributes_pb2 import ( @@ -474,7 +476,7 @@ def t(row: Row) -> TraceItemAttributeNamesResponse.Attribute: # tuple by default, or the semver key of the name under SORT_NATURAL. natural = _order_by_natural(request) - def _name_key(row: dict) -> object: + def _name_key(row: Mapping[str, Any]) -> Any: attr_key = row.get("attr_key", ("TYPE_STRING", "")) attr_type, attr_name = attr_key[0], attr_key[1] if natural: From 25fe042976ac3a77d89530846b066317848c9b6f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 18:24:07 +0000 Subject: [PATCH 24/27] test: address review comments on semver tests - test_length_normalisation: use a `<` ordering check instead of a strict adjacency (`idx_120 - idx_12 == 1`) check, per review. - TestSemverSortKey: hoist the inlined `snuba.query.dsl.column` imports to the module-level import already present, using `column` directly. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Xfgiye86dAPn9JNeHp4kAi --- tests/web/rpc/test_common.py | 12 +++--------- .../test_endpoint_trace_item_table.py | 2 +- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index 4789b67fff..8e7959c1a6 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -1461,9 +1461,7 @@ def test_not_like_wildcard_matches_only_absent(self) -> None: class TestSemverSortKey: def test_expression_structure(self) -> None: - from snuba.query.dsl import column as snuba_column - - expr = semver_sort_key(snuba_column("release")) + expr = semver_sort_key(column("release")) assert isinstance(expr, FunctionCall) assert expr.function_name == "tuple" assert len(expr.parameters) == 3 @@ -1479,16 +1477,12 @@ def test_expression_structure(self) -> None: assert raw_str_expr.function_name == "ifNull" def test_alias_is_forwarded(self) -> None: - from snuba.query.dsl import column as snuba_column - - expr = semver_sort_key(snuba_column("release"), alias="semver_key") + expr = semver_sort_key(column("release"), alias="semver_key") assert isinstance(expr, FunctionCall) assert expr.alias == "semver_key" def test_no_alias_by_default(self) -> None: - from snuba.query.dsl import column as snuba_column - - expr = semver_sort_key(snuba_column("release")) + expr = semver_sort_key(column("release")) assert expr.alias is None diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index d00c393057..12581ca759 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4608,7 +4608,7 @@ def test_length_normalisation(self) -> None: idx_120 = releases.index("1.2.0") # Both normalise to [1,2,0,0], so they are adjacent. The raw-string # tiebreaker then breaks the tie deterministically: "1.2" < "1.2.0". - assert idx_120 - idx_12 == 1, ( + assert idx_12 < idx_120, ( "1.2 and 1.2.0 normalise equally and must be adjacent, with '1.2' " "first via the raw-string tiebreaker" ) From 4c74cfc5d3abfe99f606b10713c6c1c60ca57919 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 18:36:42 +0000 Subject: [PATCH 25/27] =?UTF-8?q?fix(eap):=20harden=20semver=20sort=20?= =?UTF-8?q?=E2=80=94=20build=20metadata,=20boolean=20guard,=20page=20token?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings on the SEMVER sort work: - semver_sort_key: strip SemVer build metadata ("1.2.3+build") before parsing. Left attached it zeroed the last numeric component and failed the stability match, so "1.2.3+build" sorted as a prerelease of 1.2.0 instead of as stable 1.2.3. Applied in both the ClickHouse expression and its Python mirror (_semver_sort_key_py). - trace_item_attribute_values: only apply the semver key under SORT_SEMVER for TYPE_STRING values. This endpoint also enumerates boolean values, and the string-only semver functions would fail at ClickHouse; mirrors the string-type guard already in the table resolver. - pagination.get_filters: a client-supplied page token with an unsupported timestamp value type desynced the parallel column lists and crashed the strict zip() with an opaque error. Resolve the value first and raise a clear error, mirroring the validation in create(). Added regression tests for each. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Xfgiye86dAPn9JNeHp4kAi --- snuba/web/rpc/common/common.py | 11 ++++-- snuba/web/rpc/common/pagination.py | 23 ++++++++--- .../v1/endpoint_trace_item_attribute_names.py | 7 +++- .../web/rpc/v1/trace_item_attribute_values.py | 8 +++- tests/web/rpc/test_common.py | 39 +++++++++++++++++++ .../test_endpoint_trace_item_table.py | 15 +++++++ .../v1/test_trace_item_attribute_values_v1.py | 16 ++++++++ 7 files changed, 107 insertions(+), 12 deletions(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index e1e50327b2..54f9151005 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -102,7 +102,8 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: in per request (there is no hardcoded attribute list), and the key is applied to whatever column they order by. - Strips a leading 'package@' prefix, isolates the release part (before '-'), + Strips a leading 'package@' prefix and any '+build' metadata (which per + SemVer does not affect precedence), isolates the release part (before '-'), maps each dot-component to UInt32, pads to 4 elements so "1.2" == "1.2.0", and adds a stability flag (0=prerelease, 1=stable) so prerelease versions sort before their corresponding stable release. Works on Altinity 25.3/25.8. @@ -122,7 +123,11 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: # the nullable wrapper before applying any string → array functions. non_null = f.ifNull(expr, literal("")) version_no_prefix = f.arrayElement(f.splitByChar(literal("@"), non_null), literal(-1)) - release_part = f.arrayElement(f.splitByChar(literal("-"), version_no_prefix), literal(1)) + # Drop SemVer build metadata ("1.2.3+build") before parsing: it must not + # affect precedence, and left attached it would zero the last numeric + # component (toUInt32OrZero("3+build") == 0) and fail the stability match. + version_no_build = f.arrayElement(f.splitByChar(literal("+"), version_no_prefix), literal(1)) + release_part = f.arrayElement(f.splitByChar(literal("-"), version_no_build), literal(1)) numeric_key = f.arrayResize( f.arrayMap( Lambda(None, ("x",), f.toUInt32OrZero(x)), @@ -136,7 +141,7 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: # PEP 440 dot-style dev/pre builds ("24.7.0.dev0+", the common form on # sentry.release) — the latter has no '-', so a bare '-' check would wrongly # tag it stable and sort the dev build after its GA release. - is_stable = f.match(version_no_prefix, literal(r"^[0-9]+(\.[0-9]+)*$")) + is_stable = f.match(version_no_build, literal(r"^[0-9]+(\.[0-9]+)*$")) return FunctionCall(alias, "tuple", (numeric_key, is_stable, non_null)) diff --git a/snuba/web/rpc/common/pagination.py b/snuba/web/rpc/common/pagination.py index bc3e2ecf1a..53690f5307 100644 --- a/snuba/web/rpc/common/pagination.py +++ b/snuba/web/rpc/common/pagination.py @@ -90,14 +90,25 @@ def get_filters(self) -> Expression | None: continue if key_name == f"{self._FILTER_PREFIX}.timestamp": + # Resolve the value first and raise on an unsupported type (page + # tokens are client-supplied): otherwise column_values would fall + # out of sync with column_names/column_is_semver and the + # strict zip() below would crash with an opaque error. Mirrors the + # timestamp validation in create(). + value = filter.comparison_filter.value + if value.HasField("val_str"): + column_values.append(f.toDateTime(value.val_str)) + elif value.HasField("val_double"): + column_values.append(literal(value.val_double)) + elif value.HasField("val_int"): + column_values.append(literal(value.val_int)) + else: + raise ValueError( + f"Timestamp value type {value.WhichOneof('value')} not supported " + "in page token" + ) column_names.append("timestamp") column_is_semver.append(False) - if filter.comparison_filter.value.HasField("val_str"): - column_values.append(f.toDateTime(filter.comparison_filter.value.val_str)) - elif filter.comparison_filter.value.HasField("val_double"): - column_values.append(literal(filter.comparison_filter.value.val_double)) - elif filter.comparison_filter.value.HasField("val_int"): - column_values.append(literal(filter.comparison_filter.value.val_int)) else: # strip the matching prefix (and the dot) to recover the alias prefix = self._SEMVER_FILTER_PREFIX if is_semver else self._FILTER_PREFIX diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index ff0819a3ad..216285d90d 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -96,13 +96,16 @@ def _semver_sort_key_py(name: str) -> tuple[tuple[int, int, int, int], int, str] the merged (ClickHouse + synthetic) result matches the ClickHouse ORDER BY.""" non_null = name or "" version_no_prefix = non_null.split("@")[-1] - release_part = version_no_prefix.split("-")[0] + # Drop SemVer build metadata ("1.2.3+build") before parsing; it must not + # affect precedence (mirrors common.semver_sort_key). + version_no_build = version_no_prefix.split("+")[0] + release_part = version_no_build.split("-")[0] # Mirror ClickHouse toUInt32OrZero: only ASCII decimal parses, anything else # (including Unicode digits like "²", where str.isdigit() is True but int() # raises) maps to 0. components = [int(c) if (c.isascii() and c.isdigit()) else 0 for c in release_part.split(".")] components = (components + [0, 0, 0, 0])[:4] - is_stable = 1 if _SEMVER_NUMERIC_RE.match(version_no_prefix) else 0 + is_stable = 1 if _SEMVER_NUMERIC_RE.match(version_no_build) else 0 return ( (components[0], components[1], components[2], components[3]), is_stable, diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index eb5f349460..cb241e7c31 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -143,7 +143,13 @@ def _build_query( # count() stays the primary ordering so the most common values still come # first; the sort key only changes the tiebreak among equally frequent values. # An unset/SORT_DEFAULT sort keeps the historical lexicographic order. - if request.order_by.sort == TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER: + # semver_sort_key applies string functions (splitByChar, match, ...), so it + # only makes sense for string values; boolean keys (also enumerable here) + # keep plain ordering, mirroring the string-type guard in the table resolver. + if ( + request.order_by.sort == TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER + and request.key.type == AttributeKey.TYPE_STRING + ): value_order_expression: Expression = semver_sort_key(column("attr_value")) else: value_order_expression = column("attr_value") diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index 8e7959c1a6..878003bdc1 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -11,6 +11,7 @@ ) from sentry_protos.snuba.v1.error_pb2 import Error as ErrorProto from sentry_protos.snuba.v1.request_common_pb2 import ( + PageToken, RequestMeta, TraceItemType, ) @@ -69,6 +70,7 @@ RPCAllocationPolicyException, convert_rpc_exception_to_proto, ) +from snuba.web.rpc.common.pagination import FlexibleTimeWindowPageWithFilters from snuba.web.rpc.v1.endpoint_trace_item_table import EndpointTraceItemTable from tests.conftest import SnubaSetConfig from tests.helpers import write_raw_unprocessed_events @@ -1486,6 +1488,43 @@ def test_no_alias_by_default(self) -> None: assert expr.alias is None +class TestFlexibleTimeWindowPageFilters: + def _timestamp_page_token(self, value: AttributeValue) -> PageToken: + prefix = FlexibleTimeWindowPageWithFilters._FILTER_PREFIX + return PageToken( + filter_offset=TraceItemFilter( + and_filter=AndFilter( + filters=[ + TraceItemFilter( + comparison_filter=ComparisonFilter( + key=AttributeKey(name=f"{prefix}.timestamp"), + op=ComparisonFilter.OP_LESS_THAN, + value=value, + ) + ) + ] + ) + ) + ) + + def test_get_filters_rejects_unsupported_timestamp_type(self) -> None: + # A client-supplied page token whose timestamp filter carries an + # unsupported value type must raise a clear error rather than desync the + # parallel column lists and crash the strict zip() with an opaque error. + pager = FlexibleTimeWindowPageWithFilters( + self._timestamp_page_token(AttributeValue(val_bool=True)) + ) + with pytest.raises(ValueError, match="not supported"): + pager.get_filters() + + def test_get_filters_accepts_int_timestamp(self) -> None: + pager = FlexibleTimeWindowPageWithFilters( + self._timestamp_page_token(AttributeValue(val_int=1741910400)) + ) + # A supported value type builds the boundary filter without raising. + assert pager.get_filters() is not None + + class TestAnyAttributeFilterOption: """The `enable_any_attribute_filter` sentry-option gates whether any_attribute_filter is translated into a predicate or treated as diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index 12581ca759..f2219d5f3f 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4516,6 +4516,9 @@ class TestSemverSorting: # its GA release, to verify the dev build sorts before GA. "24.7.0.dev0+abc123", "24.7.0", + # SemVer build metadata must not affect precedence: "1.2.3+build456" + # sorts as the stable 1.2.3, not as a prerelease of 1.2.0. + "1.2.3+build456", ] @pytest.fixture(autouse=True) @@ -4602,6 +4605,18 @@ def test_package_prefix_stripped(self) -> None: "my-pkg@2.0.0 should sort as version 2.0.0 (after 1.x)" ) + def test_build_metadata_ignored(self) -> None: + # Build metadata does not affect precedence: "1.2.3+build456" must sort + # as stable 1.2.3 (after 1.2.2 and after the 1.2.3-beta.1 prerelease), + # not as a prerelease of 1.2.0 (which would land before 1.2.2). + releases = self._query_releases() + assert releases.index("1.2.3+build456") > releases.index("1.2.2"), ( + "1.2.3+build456 must sort as 1.2.3 (stable), after 1.2.2" + ) + assert releases.index("1.2.3+build456") > releases.index("1.2.3-beta.1"), ( + "build metadata is not a prerelease: 1.2.3+build456 sorts after 1.2.3-beta.1" + ) + def test_length_normalisation(self) -> None: releases = self._query_releases() idx_12 = releases.index("1.2") diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index 890a25ed1e..7600f8d802 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -347,6 +347,22 @@ def test_boolean_case(self, setup_teardown: Any) -> None: assert response.values == ["true", "false"] assert response.counts == [8, 1] + def test_boolean_case_with_semver_sort_does_not_crash(self, setup_teardown: Any) -> None: + # SORT_SEMVER only applies the semver key to string values; a boolean key + # falls back to plain ordering instead of feeding a bool column into the + # string-only semver functions (which would fail at ClickHouse). + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=5, + key=AttributeKey(name="sentry.is_segment", type=AttributeKey.TYPE_BOOLEAN), + order_by=TraceItemAttributeValuesRequest.OrderBy( + sort=TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER, + ), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == ["true", "false"] + assert response.counts == [8, 1] + def test_boolean_existence_check(self, setup_teardown: Any) -> None: # `custom_flag` is set on exactly two items (one True, one False); the # other items do not have the key. The existence check must exclude the From de208f0560f52b0c45535c387c3ed68559667a5b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 18:42:41 +0000 Subject: [PATCH 26/27] fix(eap): honor descending for SORT_SEMVER name ordering SORT_SEMVER selects name ordering with the OrderBy column typically left unset, but _order_by_name_descending only recognized COLUMN_NAME, so a SORT_SEMVER request with descending=True still sorted ascending in both the ClickHouse ORDER BY and the Python merge re-sort. Recognize semver ordering in _order_by_name_descending so descending flips it. Added a regression test asserting the descending result is the reverse of ascending. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Xfgiye86dAPn9JNeHp4kAi --- .../v1/endpoint_trace_item_attribute_names.py | 22 ++++++------ ...est_endpoint_trace_item_attribute_names.py | 34 +++++++++++++++++++ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index 216285d90d..fe2e60db3c 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -71,23 +71,25 @@ def _order_by_count(request: TraceItemAttributeNamesRequest) -> bool: return request.order_by.column == TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_COUNT +def _order_by_semver(request: TraceItemAttributeNamesRequest) -> bool: + """Whether the caller requested SORT_SEMVER (semver) ordering of names.""" + return request.order_by.sort == TraceItemAttributeNamesRequest.OrderBy.SORT_SEMVER + + def _order_by_name_descending(request: TraceItemAttributeNamesRequest) -> bool: - """Whether the caller explicitly requested name ordering in descending order. + """Whether the caller requested name ordering in descending order. - Only ``COLUMN_NAME`` + ``descending`` flips the default; unset ordering stays - name-ascending for backwards compatibility. + Both an explicit ``COLUMN_NAME`` and ``SORT_SEMVER`` (which orders by the + semver key of the name, typically with ``column`` left unset) select name + ordering, so ``descending`` flips either. Unset ordering stays name-ascending + for backwards compatibility. """ - return ( + return request.order_by.descending and ( request.order_by.column == TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_NAME - and request.order_by.descending + or _order_by_semver(request) ) -def _order_by_semver(request: TraceItemAttributeNamesRequest) -> bool: - """Whether the caller requested SORT_SEMVER (semver) ordering of names.""" - return request.order_by.sort == TraceItemAttributeNamesRequest.OrderBy.SORT_SEMVER - - _SEMVER_NUMERIC_RE = re.compile(r"^[0-9]+(\.[0-9]+)*$") diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py index 11a787b382..35c8043fa3 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py @@ -132,6 +132,40 @@ def test_semver_sort(self) -> None: res = EndpointTraceItemAttributeNames().execute(req) assert [a.name for a in res.attributes] == ["1.2.2", "1.2.9", "1.2.10"] + def test_semver_sort_descending(self) -> None: + # descending applies to SORT_SEMVER name ordering even with column unset, + # so the result is the exact reverse of the ascending semver order. + items_storage = get_writable_storage(StorageKey("eap_items")) + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={name: AnyValue(string_value="x")}, + ) + for name in ("1.2.9", "1.2.10", "1.2.2") + ], + ) + req = TraceItemAttributeNamesRequest( + meta=RequestMeta( + project_ids=[1, 2, 3], + organization_id=1, + cogs_category="something", + referrer="something", + start_timestamp=Timestamp(seconds=int((BASE_TIME - timedelta(days=1)).timestamp())), + end_timestamp=Timestamp(seconds=int((BASE_TIME + timedelta(days=1)).timestamp())), + ), + limit=100, + type=AttributeKey.Type.TYPE_STRING, + value_substring_match="1.2", + order_by=TraceItemAttributeNamesRequest.OrderBy( + sort=TraceItemAttributeNamesRequest.OrderBy.SORT_SEMVER, + descending=True, + ), + ) + res = EndpointTraceItemAttributeNames().execute(req) + assert [a.name for a in res.attributes] == ["1.2.10", "1.2.9", "1.2.2"] + def test_simple_float_backward_compat(self) -> None: req = TraceItemAttributeNamesRequest( meta=RequestMeta( From c83aa5482fb14551455c0e4b2e82d73036c7b396 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 19:53:53 +0000 Subject: [PATCH 27/27] docs(eap): condense semver code comments Trim the verbose docstrings/inline comments added for the semver work (semver_sort_key, pagination boundary handling, attribute-name re-sort, resolver ORDER BY guard, values ordering) down to their essential rationale. No behavior change. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Xfgiye86dAPn9JNeHp4kAi --- snuba/web/rpc/common/common.py | 46 ++++++------------- snuba/web/rpc/common/pagination.py | 19 ++++---- .../v1/endpoint_trace_item_attribute_names.py | 12 ++--- .../R_eap_items/resolver_trace_item_table.py | 5 +- .../web/rpc/v1/trace_item_attribute_values.py | 13 ++---- 5 files changed, 33 insertions(+), 62 deletions(-) diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index 54f9151005..403f24475c 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -96,36 +96,22 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: - """Return a Tuple(Array(UInt32), UInt8, String) sort key for semantic-version ORDER BY. - - This is the sort applied for the ``SORT_SEMVER`` OrderBy option: callers opt - in per request (there is no hardcoded attribute list), and the key is applied - to whatever column they order by. - - Strips a leading 'package@' prefix and any '+build' metadata (which per - SemVer does not affect precedence), isolates the release part (before '-'), - maps each dot-component to UInt32, pads to 4 elements so "1.2" == "1.2.0", - and adds a stability flag (0=prerelease, 1=stable) so prerelease versions sort - before their corresponding stable release. Works on Altinity 25.3/25.8. - - The third tuple element is the raw (non-null) string. It is a tiebreaker: - distinct strings that map to the same numeric key + stability flag (e.g. "1.2" - and "1.2.0", or two prereleases of the same version) would otherwise compare - equal, which makes ordering non-deterministic and — more importantly — lets - the flextime pagination's strict `less` boundary filter skip rows tied with - the cursor. Appending the raw string gives a deterministic total order over - distinct release strings and keeps the page-boundary comparison exact. Both - ORDER BY and pagination call this function, so they stay consistent. + """Return a Tuple(Array(UInt32), UInt8, String) semver sort key for ``SORT_SEMVER``. + + Callers opt in per request (no hardcoded attribute list) and the key is + applied to whatever column they order by. Strips a 'package@' prefix and + '+build' metadata, maps the release part to 4 UInt32 components (so "1.2" == + "1.2.0"), then adds a stability flag (0=prerelease, 1=stable) so prereleases + sort before their stable release, and the raw string as a tiebreaker for a + deterministic total order. Works on Altinity 25.3/25.8 (no naturalSortKey). """ x = Argument(None, "x") - # sentry.release is coalesced from multiple attribute columns and therefore - # returns Nullable(String). ClickHouse forbids Nullable(Array(…)), so strip - # the nullable wrapper before applying any string → array functions. + # sentry.release is coalesced, so Nullable(String); strip the nullable + # wrapper (ClickHouse forbids Nullable(Array(…))) before string→array funcs. non_null = f.ifNull(expr, literal("")) version_no_prefix = f.arrayElement(f.splitByChar(literal("@"), non_null), literal(-1)) - # Drop SemVer build metadata ("1.2.3+build") before parsing: it must not - # affect precedence, and left attached it would zero the last numeric - # component (toUInt32OrZero("3+build") == 0) and fail the stability match. + # Drop build metadata (does not affect precedence); left attached it would + # zero the last component and fail the stability match. version_no_build = f.arrayElement(f.splitByChar(literal("+"), version_no_prefix), literal(1)) release_part = f.arrayElement(f.splitByChar(literal("-"), version_no_build), literal(1)) numeric_key = f.arrayResize( @@ -135,12 +121,8 @@ def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: ), literal(_SEMVER_COMPONENT_COUNT), ) - # A release is "stable" only when the version is a plain dotted-numeric - # string ("1.2.3", "1.2"). Anything else is a prerelease and sorts before - # the matching stable release: SemVer prereleases ("1.2.3-beta.1") as well as - # PEP 440 dot-style dev/pre builds ("24.7.0.dev0+", the common form on - # sentry.release) — the latter has no '-', so a bare '-' check would wrongly - # tag it stable and sort the dev build after its GA release. + # Stable only for plain dotted-numeric versions; anything else (SemVer + # "-beta.1" or PEP 440 dot-dev "24.7.0.dev0+") is a prerelease. is_stable = f.match(version_no_build, literal(r"^[0-9]+(\.[0-9]+)*$")) return FunctionCall(alias, "tuple", (numeric_key, is_stable, non_null)) diff --git a/snuba/web/rpc/common/pagination.py b/snuba/web/rpc/common/pagination.py index 53690f5307..f9c3693ec0 100644 --- a/snuba/web/rpc/common/pagination.py +++ b/snuba/web/rpc/common/pagination.py @@ -30,8 +30,8 @@ class FlexibleTimeWindowPageWithFilters: _TIME_WINDOW_START_KEY = f"{_TIME_WINDOW_PREFIX}.start_timestamp" _TIME_WINDOW_END_KEY = f"{_TIME_WINDOW_PREFIX}.end_timestamp" _FILTER_PREFIX = "sentry__filter" - # Marks a page-boundary column whose ORDER BY used SORT_SEMVER (semver), so - # get_filters applies the same semver key on both sides of the comparison. + # Marks a page-boundary column whose ORDER BY used SORT_SEMVER, so + # get_filters applies the semver key on both sides of the comparison. _SEMVER_FILTER_PREFIX = "sentry__semver_filter" def __init__(self, page_token: PageToken): @@ -90,11 +90,9 @@ def get_filters(self) -> Expression | None: continue if key_name == f"{self._FILTER_PREFIX}.timestamp": - # Resolve the value first and raise on an unsupported type (page - # tokens are client-supplied): otherwise column_values would fall - # out of sync with column_names/column_is_semver and the - # strict zip() below would crash with an opaque error. Mirrors the - # timestamp validation in create(). + # Resolve the value first and raise on an unsupported type + # (tokens are client-supplied) so the parallel lists stay in + # sync and the strict zip() below can't crash. Mirrors create(). value = filter.comparison_filter.value if value.HasField("val_str"): column_values.append(f.toDateTime(value.val_str)) @@ -226,10 +224,9 @@ def create( f"No attribute expression found for column: {order_by_clause.column.label}" ) - # Mark the column as SORT_SEMVER (semver) when the request - # ordered it that way, so get_filters wraps both sides in the - # semver key and the page boundary matches the ORDER BY. Mirror - # the resolver's guard: only string columns get the semver key. + # Mark SORT_SEMVER string columns so get_filters wraps + # both sides in the semver key (matching ORDER BY); mirrors + # the resolver's string-only guard. is_semver = ( order_by_clause.sort == TraceItemTableRequest.OrderBy.SORT_SEMVER and selected_key is not None diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index fe2e60db3c..00279b3ee2 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -497,17 +497,15 @@ def _name_key(row: Mapping[str, Any]) -> Any: ] non_stored.sort(key=_name_key) if _order_by_count(request): - # Order the real (counted) rows to match ClickHouse: count in the - # requested direction, then name ASC (two stable passes). The synthetic - # non-stored attributes have no real count, so pin them first regardless - # of sort direction rather than relying on a sentinel value. + # Match ClickHouse: count in the requested direction, then name ASC + # (two stable passes). Synthetic non-stored keys have no count, so + # pin them first rather than relying on a sentinel. data.sort(key=_name_key) data.sort(key=lambda row: row.get("count", 0), reverse=request.order_by.descending) data = non_stored + data else: - # Default name ordering: merge the synthetic non-stored keys in and re-sort - # by name, honoring the requested direction so it matches the ClickHouse - # ORDER BY (a COLUMN_NAME descending request must stay descending here too). + # Merge synthetic non-stored keys in and re-sort by name in the + # requested direction, matching the ClickHouse ORDER BY. data.extend(non_stored) data.sort( key=_name_key, diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index b22c5218c3..238597b7d1 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -344,9 +344,8 @@ def _convert_order_by( # expression so an aggregation query that orders by `sentry.timestamp` stays # valid. expression = _groupby_order_by_expression(x.column.key) - # SORT_SEMVER requests semver-aware ordering (client-driven, no hardcoded - # attribute list). It only applies to string columns; numeric/timestamp - # columns already sort numerically, so leave them untouched. + # SORT_SEMVER: client-driven semver ordering, string columns only + # (numeric/timestamp columns already sort numerically). if ( x.sort == TraceItemTableRequest.OrderBy.SORT_SEMVER and x.column.key.type == AttributeKey.TYPE_STRING diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index cb241e7c31..69d46c26f9 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -137,15 +137,10 @@ def _build_query( ) treeify_or_and_conditions(inner_query) add_existence_check_to_map_attribute_reads(inner_query) - # The value column normally orders lexicographically. When the caller opts - # into SORT_SEMVER, order it by the semver key so versions sort numerically - # ("1.2.9" before "1.2.10") with prereleases before their stable release. - # count() stays the primary ordering so the most common values still come - # first; the sort key only changes the tiebreak among equally frequent values. - # An unset/SORT_DEFAULT sort keeps the historical lexicographic order. - # semver_sort_key applies string functions (splitByChar, match, ...), so it - # only makes sense for string values; boolean keys (also enumerable here) - # keep plain ordering, mirroring the string-type guard in the table resolver. + # Under SORT_SEMVER, tiebreak equally-frequent values by the semver key + # instead of lexicographically (count() stays the primary ordering). Only + # string values: semver_sort_key uses string functions, so boolean keys + # (also enumerable here) keep plain ordering, like the table resolver's guard. if ( request.order_by.sort == TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER and request.key.type == AttributeKey.TYPE_STRING