Scope NULLS LAST to ttft_ms only

The previous version appended NULLS LAST to every ORDER BY, which would
silently change DESC semantics for any nullable sort column added to the
whitelist later. Today the existing sort columns (spend, total_tokens,
startTime, endTime, request_duration_ms, model) are all non-null in the
result set, so the clause is a no-op for them — but the broader form is
misleading.

Apply NULLS LAST only when sorting by ttft_ms (the only column whose
computed expression actually produces NULLs). Update the model test to
assert the clause is absent for non-nullable columns.
This commit is contained in:
Yuneng Jiang 2026-04-24 22:52:04 -07:00
parent 5c0349a635
commit 2047446546
No known key found for this signature in database
2 changed files with 14 additions and 7 deletions

View file

@ -2047,18 +2047,22 @@ async def ui_view_spend_logs( # noqa: PLR0915
sql_params.append(f"%{error_message}%")
p += 1
# Build the ORDER BY expression. NULLS LAST keeps rows without a
# meaningful value for the sorted column at the bottom regardless of
# direction. ttft_ms is computed from completionStartTime - startTime;
# non-streaming rows (where completionStartTime is null or equals
# endTime) yield NULL so they sort last.
# Build the ORDER BY expression. ttft_ms is computed from
# completionStartTime - startTime; non-streaming rows (where
# completionStartTime is null or equals endTime) yield NULL, so we
# append NULLS LAST in that case to keep them at the bottom regardless
# of direction. The other sort columns are non-null in the result set,
# so we leave the NULLS clause off and preserve their existing DESC
# semantics.
_sql_dir = "ASC" if order_direction == "asc" else "DESC"
_nulls_clause = ""
if order_column == "ttft_ms":
_order_expr = (
'CASE WHEN "completionStartTime" IS NULL '
'OR "completionStartTime" = "endTime" THEN NULL '
'ELSE (EXTRACT(EPOCH FROM ("completionStartTime" - "startTime")) * 1000) END'
)
_nulls_clause = " NULLS LAST"
elif order_column in ("startTime", "endTime"):
_order_expr = f'"{order_column}"'
else:
@ -2076,7 +2080,7 @@ async def ui_view_spend_logs( # noqa: PLR0915
COALESCE(request_duration_ms, (EXTRACT(EPOCH FROM ("endTime" - "startTime")) * 1000)::INTEGER) AS request_duration_ms
FROM "LiteLLM_SpendLogs"
WHERE {" AND ".join(sql_conditions)}
ORDER BY {_order_expr} {_sql_dir} NULLS LAST
ORDER BY {_order_expr} {_sql_dir}{_nulls_clause}
LIMIT ${p} OFFSET ${p + 1}
"""
sql_params.extend([page_size, skip])

View file

@ -833,7 +833,10 @@ async def test_ui_view_spend_logs_sort_by_model(
async def mock_query_raw(sql_query, *params):
assert "model" in sql_query
assert "NULLS LAST" in sql_query
# model is non-nullable in the schema, so NULLS LAST should NOT be
# appended — only ttft_ms gets that clause. This guards against
# accidentally widening the change to all sort columns.
assert "NULLS LAST" not in sql_query
reverse = "DESC" in sql_query
sorted_logs = sorted(
base_logs, key=lambda x: x.get("model", ""), reverse=reverse