feat(partitions): scope chunk listing to a single file (fix O(partition) detail view) - #515
Conversation
The document detail view loaded every chunk (with content) in the whole
partition via GET /partition/{p}/chunks and filtered client-side by file_id,
making it O(partition) and slow as partitions grow.
Add optional file_id and limit params to list_all_chunks (service + route).
When file_id is given, the filter is pushed down to the vector store
(query_chunks_by_filter), so the detail view is O(file); limit caps the result
as a defensive bound. Default partition-wide behavior is unchanged.
Closes #514
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTwo new optional parameters, ChangesPer-file chunk filtering and limit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
listFileChunks now passes file_id to GET /partition/{p}/chunks so the server
returns just that file's chunks instead of the whole partition's. The
client-side filter is kept as a defensive no-op so results stay correct against
backends that don't yet support the param (#515).
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 `@openrag/services/orchestrators/partition_service.py`:
- Around line 362-364: The `limit` parameter in the partition service method
accepts negative values, which causes unintended behavior when used in list
slicing operations like `rows[:limit]` where a negative limit returns
all-but-tail rows instead of being rejected. Add validation to ensure the
`limit` parameter is either None or a non-negative integer before it is used in
any slicing operations. Additionally, apply the same constraint at the API layer
in the admin partitions router by using `Query(default=None, ge=0)` to enforce
non-negative limits at the endpoint level.
🪄 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: Pro
Run ID: b9df2c16-348e-4403-9311-8430120ae82a
📒 Files selected for processing (3)
openrag/api/routers/admin/partitions.pyopenrag/services/orchestrators/partition_service.pytests/unit/services/orchestrators/test_partition_service.py
A negative `limit` slipped past `rows[:limit]` and silently returned all-but-tail rows instead of capping. Guard it in the service (422) and constrain the router params with `Query(ge=0)`.
listFileChunks now passes file_id to GET /partition/{p}/chunks so the server
returns just that file's chunks instead of the whole partition's. The
client-side filter is kept as a defensive no-op so results stay correct against
backends that don't yet support the param (#515).
What
Adds an optional
file_id(andlimit) parameter toGET /partition/{p}/chunksso the document detail view can fetch a single file's chunks with content in one bounded, server-filtered call.Fixes #514.
Why
The detail view's chunk loading was O(partition): the only content-returning chunk endpoint was partition-wide, and the UI filtered client-side by
file_id— so opening one document downloaded every chunk's text in the partition (the endpoint even warns about large payloads). This is the "Loading file…" lag that grows with partition size.Change
PartitionService.list_all_chunks(partition, include_embedding=True, file_id=None, limit=None)— whenfile_idis set it's added to the vector-store filter (query_chunks_by_filter({partition, file_id})), pushing the work down to Milvus;limitcaps the result. Nofile_id→ identical behavior to before.GET /partition/{p}/chunksexposesfile_idandlimitquery params (passthrough). Permissions unchanged (partition viewer+).The data path already existed (
get_file_chunksfilters byfile_idserver-side but dropstext); this exposes it on the content endpoint.Test
Adds service tests: partition-only filter when no
file_id,{partition, file_id}filter when given, andlimitcapping. Suite: 23 passed, ruff clean.Frontend follow-up
Separate change on the UI branch:
listFileChunksswitches to…/chunks?file_id=<id>&include_embedding=falseand drops the client-side filter.Summary by CodeRabbit
New Features
limitis available to cap returned chunks for improved performance on large partitions.Bug Fixes
limitvalues are now rejected for both chunk listing and file chunk retrieval.Tests