feat(intake): add column sorting to evaluation sessions endpoint [ASE-591]#780
Merged
Conversation
Add _SESSION_SORT_FIELDS whitelist and _parse_session_sort_keys parser. The parser validates field names and converts the comma-separated sort string to an ordered list of (field, descending) tuples, raising 422 on an unknown field. Parsed sort_keys are threaded through to list_sessions (repository layer) rather than passing the raw string, keeping validation at the HTTP boundary. Signed-off-by: Nathan Walston <[email protected]>
Replace hardcoded ORDER BY with user-driven multi-field sort in _list_sql. Sort composes with LIMIT/OFFSET in ClickHouse so it is global (not per-page). For trace_index columns (started_at, ended_at, latency_ms, status, test_case_id): swap the ORDER BY in page_sessions directly — the values exist in scoped_sessions without any join. For cost_total_usd and tokens: inject a pre_page_metrics CTE that aggregates spans for ALL scoped sessions before pagination runs. This makes the ORDER BY see the full result set, not just the current page. NULLS LAST on every user key; root_span_id always appended as a stable tiebreaker. Default (no sort param) preserves the original start_time ASC behaviour. Tests: default order preserved, single-field asc/desc, multi-field, pre_page_metrics injection for cost/tokens, tiebreaker presence, _build_order_by unit tests. Signed-off-by: Nathan Walston <[email protected]>
make refresh-openapi adds sort to ListEvaluationSessionsParams. make stainless (Python SDK sync) requires STAINLESS_API_KEY — run separately once the key is available. Signed-off-by: Nathan Walston <[email protected]>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEvaluation session listing now supports validated multi-field sorting, including cost and token metrics, with updated API documentation, paginated ClickHouse ordering, size-limit handling, and SQL-generation tests. ChangesEvaluation session sorting
Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionsEndpoint
participant EvaluationSessionRepository
participant ClickHouse
Client->>SessionsEndpoint: Request sessions with sort
SessionsEndpoint->>SessionsEndpoint: Validate and parse sort fields
SessionsEndpoint->>EvaluationSessionRepository: Pass sort_keys
EvaluationSessionRepository->>ClickHouse: Execute paginated ordered query
ClickHouse-->>Client: Return sorted sessions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
services/intake/src/nmp/intake/spans/evaluation_session_repository.py (1)
229-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-file allowlist coupling is fragile.
_build_order_bydoes a bareexpr_map[field]lookup. Correctness depends on_SESSION_SORT_FIELDSinendpoints.pystaying exactly in sync with_SORT_EXPR_PAGE/_SORT_EXPR_FINALhere. A future field added to one but not the other becomes an unhandledKeyError(500) instead of the intended 400. Consider deriving the endpoint allowlist from_SORT_EXPR_PAGE.keys() & _SORT_EXPR_FINAL.keys(), or asserting parity in a test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/intake/src/nmp/intake/spans/evaluation_session_repository.py` around lines 229 - 241, The sort-field allowlist is duplicated across endpoint and repository modules, allowing unsupported fields to reach _build_order_by and raise KeyError. Derive _SESSION_SORT_FIELDS from the intersection of _SORT_EXPR_PAGE and _SORT_EXPR_FINAL keys, or add an explicit parity test, so only fields supported by both expression maps are accepted and invalid input remains a 400.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/intake/src/nmp/intake/api/v2/experiments/endpoints.py`:
- Around line 687-690: The session endpoint’s sort parameter uses a truncated
placeholder description. Update _parse_session_sort_keys’s endpoint definition
in services/intake/src/nmp/intake/api/v2/experiments/endpoints.py:687-690 with a
complete description covering all 7 supported fields, “-” descending prefixes,
comma-separated multi-field ordering, and omitted-value defaults; then
regenerate the corresponding descriptions in
openapi/ga/individual/platform.openapi.yaml:3688-3695,
openapi/ga/openapi.yaml:3688-3695, and openapi/openapi.yaml:3685-3695.
- Around line 1044-1067: Reconcile the sort-validation contract around
_parse_session_sort_keys: keep its existing 400 behavior consistent with sibling
validators, update the PR description if 422 is unintended, and extend the
endpoint’s OpenAPI 400 response documentation to include invalid or empty sort
fields.
In `@services/intake/src/nmp/intake/spans/evaluation_session_repository.py`:
- Around line 54-63: Update _pre_page_metrics_cte_sql’s total_tokens calculation
and _SORT_EXPR_FINAL["tokens"] to coalesce each guarded input/output token sum
to zero before addition, while preserving NULL only when both sums are NULL.
Ensure sessions with only one recorded token type receive that value for sorting
rather than a NULL result.
- Around line 243-271: The _pre_page_metrics_cte_sql path performs unbounded
aggregation for cost/token sorting; add the same size guard used by the sibling
evaluations metric-sort flow. In list_sessions, use the already-available total
before invoking this query, and return the established 413 response when it
exceeds the configured threshold, while preserving normal pagination for
eligible result sizes.
---
Nitpick comments:
In `@services/intake/src/nmp/intake/spans/evaluation_session_repository.py`:
- Around line 229-241: The sort-field allowlist is duplicated across endpoint
and repository modules, allowing unsupported fields to reach _build_order_by and
raise KeyError. Derive _SESSION_SORT_FIELDS from the intersection of
_SORT_EXPR_PAGE and _SORT_EXPR_FINAL keys, or add an explicit parity test, so
only fields supported by both expression maps are accepted and invalid input
remains a 400.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec4517ec-787d-403a-8722-cc0aaf9dcfc3
📒 Files selected for processing (6)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/spans/evaluation_session_repository.pyservices/intake/tests/test_evaluation_session_clickhouse_repository.py
Contributor
|
Signed-off-by: Nathan Walston <[email protected]>
shanaiabuggy
approved these changes
Jul 20, 2026
shanaiabuggy
left a comment
shanaiabuggy
left a comment
Contributor
There was a problem hiding this comment.
approving with 1 comment! also tagging in @BrianNewsom to do a quick review as the clickhouse connoisseur
- Sort param description: replace placeholder with full docs covering all 7 sortable fields, '-' prefix semantics, and default behaviour. - Response codes: add 413 and 422 to endpoint responses dict; add 'Invalid sort field' note to 400 description. - Token NULL propagation: coalesce each guarded sum to 0 before adding so sessions with only input or only output tokens sort by their partial usage rather than NULL-ing to the bottom via NULLS LAST. NULL is preserved only when both sides are absent. - Pre-metrics size cap: raise MetricSortTooLargeError (domain exception) when a cost/tokens sort is requested on more than 10 000 sessions. The endpoint catches it and returns 413, mirroring the evaluations list cap. The limit and upgrade path are documented in comments. Signed-off-by: Nathan Walston <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/openapi.yaml`:
- Around line 3692-3704: The sort documentation in the OpenAPI schema conflicts
with _parse_session_sort_keys, which returns 400 for unsupported or empty sort
values. Align both sort descriptions and the endpoint behavior by either
changing _parse_session_sort_keys to return 422 as documented or updating the
OpenAPI descriptions to state 400, then regenerate the specs consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f65b298d-0911-4716-a4b2-03341c3957b5
⛔ Files ignored due to path filters (5)
sdk/python/nemo-platform/.github/workflows/ci.ymlis excluded by!sdk/**sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/evaluations/sessions.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/evaluations/session_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/evaluations/test_sessions.pyis excluded by!sdk/**
📒 Files selected for processing (6)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/spans/evaluation_session_repository.pyservices/intake/tests/test_evaluation_session_clickhouse_repository.py
🚧 Files skipped from review as they are similar to previous changes (5)
- openapi/ga/openapi.yaml
- services/intake/src/nmp/intake/api/v2/experiments/endpoints.py
- openapi/ga/individual/platform.openapi.yaml
- services/intake/src/nmp/intake/spans/evaluation_session_repository.py
- services/intake/tests/test_evaluation_session_clickhouse_repository.py
Signed-off-by: Nathan Walston <[email protected]>
_parse_session_sort_keys raises 400 (matching sibling _parse_sort_keys). Remove 422 from the endpoint responses dict and fold the sort-error case into the existing 400 description. Signed-off-by: Nathan Walston <[email protected]>
Contributor
|
Performance concern for cost_total_usd/tokens sorts: ClickHouse 24.3 inlines CTEs rather than materializing them. page_sessions is referenced by current_page_spans, session_metrics, session_scores, and the Suggested direction: Query 1: count scoped sessions Query 2: compute sorted page Query 3: hydrate page This should preserve global metric sorting while ensuring the expensive all-session aggregation runs once per request. |
ClickHouse 24.3 inlines CTEs rather than materialising them, so the previous single-query approach re-executed the all-session span aggregation (pre_page_metrics) once per downstream CTE reference (current_page_spans, session_metrics, session_scores, final SELECT) — 4x per request. Split the pre-metrics sort into two separate queries: Query 2 (_metric_sort_page_ids_sql): aggregate cost/tokens once across all scoped sessions, apply ORDER BY + LIMIT/OFFSET, return ordered (workspace, session_id) pairs only. Query 3 (_hydrate_by_ids_sql): given the page session IDs, fetch session fields, page-bounded span metrics, and evaluator scores. No ORDER BY — caller restores Query 2's ordering in Python. This ensures the expensive all-session aggregation runs exactly once per request. The single-query path (_list_sql) is unchanged and still used for trace_index column sorts where CTE inlining is harmless. Signed-off-by: Nathan Walston <[email protected]>
BrianNewsom
approved these changes
Jul 20, 2026
Signed-off-by: Nathan Walston <[email protected]>
walston
enabled auto-merge
July 20, 2026 19:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds multi-field column sorting to the
GET /v2/workspaces/{workspace}/evaluations/{name}/sessionsendpoint. Sort composes globally with pagination (ORDER BY runs in ClickHouse before LIMIT/OFFSET).Changes
Endpoint (
endpoints.py)_SESSION_SORT_FIELDSwhitelist constant (module-level, per codebase convention)_parse_session_sort_keys(sort)parser: comma-separated-fieldformat, 422 on invalid fieldsort: str | None = Query(default=None)param tolist_evaluation_sessionssort_keysthreaded tolist_sessions(raw string never reaches the repository)Repository + SQL (
evaluation_session_repository.py)list_sessionsacceptssort_keys: list[tuple[str, bool]] | None = None_build_order_by: builds a comma-separated ORDER BY clause from sort keys + tiebreaker; NULLS LAST on all user keys_pre_page_metrics_cte_sql: computes cost/tokens for all sessions before pagination when sorting by those fields — necessary because those values are not intrace_indexand would otherwise only be computed for the current page_list_sqlnow acceptssort_keysand generates dynamic ORDER BY in bothpage_sessionsand the final SELECT; conditionally injectspre_page_metricsCTE for cost/tokens sortsstart_time ASC, root_span_id ASCbehaviourSortable fields
test_case_id,started_at,ended_at,latency_ms,status,cost_total_usd,tokensTests
sort_keysarg_build_order_byunit testsOpenAPI
make refresh-openapirun;ListEvaluationSessionsParamsgainssortmake stainless(Python SDK sync) requiresSTAINLESS_API_KEY— run separatelyOut of scope
Frontend wiring — separate PR on a separate branch (ASE-591 FE)
Closes ASE-591 (partial — BE only)
Summary by CodeRabbit
sortquery parameter for evaluation sessions, supporting comma-separated multi-field ordering with-for descending and default ordering bystarted_atascending when omitted.sort(400) and requests too large to sort by metrics (413), while retaining503for backend unavailability.