Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Add delete limit support and standardized delete responses across clients and APIs This PR introduces a new Additionally, the JS SDK and generated types are updated to align delete-collection response typing with a dedicated This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Sicheng-Pan
changed the base branch from
03-06-_enh_delete_with_limit
to
graphite-base/6578
Sicheng-Pan
force-pushed
the
03-06-_enh_delete_with_limit_in_clients
branch
from
2eeff23 to
94b54ba
Compare
Sicheng-Pan
force-pushed
the
graphite-base/6578
branch
from
0e11fca to
9100022
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
One important logic concern about misleading delete counts was identified and needs adjustment.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Avoid defaulting deleted count to 0 when server returns none; use sentinel/warning:
chromadb/api/fastapi.py
Review Details
📁 23 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
| json=body, | ||
| ) | ||
| return None | ||
| return DeleteResult(deleted=resp.get("deleted", 0) if resp else 0) |
There was a problem hiding this comment.
[Logic] When communicating with an older server that returns None (or empty JSON) for delete operations, this code defaults deleted to 0.
return DeleteResult(deleted=resp.get("deleted", 0) if resp else 0)This is misleading because the operation might have successfully deleted thousands of records, but the client reports 0 deleted. Users relying on this return value for logic (e.g., if result['deleted'] > 0) will encounter bugs.
Consider verifying the server version or response structure and potentially returning a sentinel value (e.g., -1) or raising a warning if the count is unavailable, rather than silently returning a potentially incorrect zero.
Context for Agents
When communicating with an older server that returns `None` (or empty JSON) for delete operations, this code defaults `deleted` to `0`.
```python
return DeleteResult(deleted=resp.get("deleted", 0) if resp else 0)
```
This is misleading because the operation might have successfully deleted thousands of records, but the client reports `0` deleted. Users relying on this return value for logic (e.g., `if result['deleted'] > 0`) will encounter bugs.
Consider verifying the server version or response structure and potentially returning a sentinel value (e.g., `-1`) or raising a warning if the count is unavailable, rather than silently returning a potentially incorrect zero.
File: chromadb/api/fastapi.py
Line: 5672 participants
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?