Skip to content

fix: use group_by instead of find_many(distinct) in /tag/list to avoid full table scan#23136

Merged
RheagalFire merged 2 commits intoBerriAI:litellm_oss_staging_03_11_2026from
CAFxX:fix/list-tags-distinct-query-performance
Mar 11, 2026
Merged

fix: use group_by instead of find_many(distinct) in /tag/list to avoid full table scan#23136
RheagalFire merged 2 commits intoBerriAI:litellm_oss_staging_03_11_2026from
CAFxX:fix/list-tags-distinct-query-performance

Conversation

@CAFxX
Copy link
Contributor

@CAFxX CAFxX commented Mar 9, 2026

Summary

  • The /tag/list endpoint used find_many(distinct=["tag"]) on LiteLLM_DailyTagSpend to discover dynamic tags. Prisma's distinct is a client-side post-processing filter that fetches all rows with all columns before deduplicating, causing a full table scan of 3M+ rows on every usage page load.
  • Replaced with Prisma's group_by() which generates proper GROUP BY SQL, letting the database handle deduplication efficiently.
  • NULL tags are filtered out via where={"tag": {"not": None}}.
  • created_at and updated_at are preserved in the response via _min/_max aggregates.
  • Added unit tests for the list_tags endpoint.

Alternative: single raw SQL query

If raw SQL is acceptable, the entire endpoint could be simplified to a single query using UNION ALL, eliminating the need for two separate queries and Python-side deduplication:

-- Stored tags (with budget info)
SELECT
    t.tag_name AS name,
    t.description,
    t.models,
    t.model_info,
    t.created_at,
    t.updated_at,
    t.created_by,
    b.budget_id,
    b.max_budget,
    b.soft_budget,
    b.budget_duration,
    b.budget_reset_at
FROM "LiteLLM_TagTable" t
LEFT JOIN "LiteLLM_BudgetTable" b ON t.budget_id = b.budget_id

UNION ALL

-- Dynamic-only tags (not in TagTable)
SELECT
    dts.tag AS name,
    'This is just a spend tag that was passed dynamically in a request. It does not control any LLM models.' AS description,
    NULL::text[] AS models,
    NULL::jsonb AS model_info,
    MIN(dts.created_at) AS created_at,
    MAX(dts.updated_at) AS updated_at,
    NULL AS created_by,
    NULL AS budget_id,
    NULL::double precision AS max_budget,
    NULL::double precision AS soft_budget,
    NULL AS budget_duration,
    NULL::timestamptz AS budget_reset_at
FROM "LiteLLM_DailyTagSpend" dts
WHERE dts.tag IS NOT NULL
  AND dts.tag NOT IN (SELECT tag_name FROM "LiteLLM_TagTable")
GROUP BY dts.tag

This pushes all filtering and deduplication into the database. The NOT IN subquery is safe here because tag_name is the primary key of LiteLLM_TagTable (never NULL). This approach goes against the project's CLAUDE.md guideline to prefer Prisma model methods over raw SQL, so the current PR uses group_by() instead.

Test plan

  • test_list_tags_with_dynamic_tags — verifies group_by is called, dynamic tags are merged, duplicates with stored tags are excluded, and created_at/updated_at are present
  • test_list_tags_no_dynamic_tags — verifies correct behavior when no dynamic tags exist
  • All existing tests in test_tag_management_endpoints.py continue to pass (12/12)

🤖 Generated with Claude Code

@vercel
Copy link

vercel bot commented Mar 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
litellm Error Error Mar 11, 2026 6:55pm

Request Review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 9, 2026

Greptile Summary

This PR fixes a significant performance regression in the /tag/list endpoint by replacing find_many(distinct=["tag"]) — which caused a full client-side table scan on LiteLLM_DailyTagSpend — with a proper group_by(by=["tag"]) that delegates deduplication to the database via a GROUP BY SQL clause. NULL tags are correctly excluded with where={"tag": {"not": None}}, and created_at/updated_at timestamps are preserved through _min/_max aggregates with proper defensive null-checking.

Key improvements:

  • Core fix: group_by replaces find_many(distinct=...), eliminating full table scans on 3M+ row tables for every UI page load.
  • NULL filtering: where={"tag": {"not": None}} correctly guards against NULL tag rows being surfaced in the response.
  • Defensive aggregate access: row.get("_min", {}).get("created_at") pattern with proper None checks matches how other group_by callers in the codebase handle aggregates.
  • Tests added: Two new unit tests verify the group_by call signature (including the NULL filter) and correct merging/deduplication behavior. Both tests validate call arguments with assert_called_once_with(...) and are mock-only with no real network calls.
  • The LiteLLM_DailyTagSpendTable import is cleanly removed as it is no longer needed.

Confidence Score: 5/5

  • This PR is safe to merge — it is a well-scoped performance fix with no functional regressions, proper NULL filtering, and comprehensive unit test coverage.
  • The change is narrowly scoped to one query in list_tags. The new group_by approach is consistent with how other endpoints in the codebase use the same Prisma API. Aggregate fields are accessed defensively with proper null checks. Both new tests use assert_called_once_with(...) to fully validate the critical call arguments including NULL filtering. All previously flagged issues have been verified to be false positives—they either critique code that already implements the fix, misread the actual implementation, or misunderstood what the tests assert. No real issues remain.
  • No files require special attention.

Last reviewed commit: 2c1c6f1

@CAFxX CAFxX changed the title fix: replace slow find_many(distinct) with SELECT DISTINCT in /tag/list fix: use group_by instead of find_many(distinct) in /tag/list to avoid full table scan Mar 9, 2026
…d full table scan

The list_tags endpoint used Prisma's find_many(distinct=["tag"]) to discover
dynamic tags from LiteLLM_DailyTagSpend. Prisma's distinct is a client-side
post-processing filter that fetches all rows with all columns before
deduplicating, causing a full table scan of 3M+ rows on every usage page load.

Replace with Prisma's group_by() which generates proper GROUP BY SQL, letting
the database handle deduplication efficiently. NULL tags are filtered out via
where={"tag": {"not": None}}, and created_at/updated_at are preserved via
_min/_max aggregates.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +466 to +467
"created_at": row.get("_min", {}).get("created_at").isoformat() if row.get("_min", {}).get("created_at") else None,
"updated_at": row.get("_max", {}).get("updated_at").isoformat() if row.get("_max", {}).get("updated_at") else None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick:
I don't think the if-else guard is needed here. The group_by query returns "dynamically" created tags which are only created by the spend tracking system and are guaranteed to have a spend row corresponding to them.

Comment on lines +309 to +315
# Verify group_by was used instead of find_many(distinct=...)
mock_db.litellm_dailytagspend.group_by.assert_called_once_with(
by=["tag"],
where={"tag": {"not": None}},
_min={"created_at": True},
_max={"updated_at": True},
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for adding this test but let's update it to only test the response and not the exact db call that's being made.

- Remove unused LiteLLM_DailyTagSpendTable class and datetime import
- Simplify created_at/updated_at access (values guaranteed by spend system)
- Remove DB call assertions from tests, keep response-only assertions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@RheagalFire RheagalFire changed the base branch from main to litellm_oss_staging_03_11_2026 March 11, 2026 19:02
@RheagalFire RheagalFire merged commit d398d1f into BerriAI:litellm_oss_staging_03_11_2026 Mar 11, 2026
22 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants