Fix duplicate key error for insert_documents [FOEPD-209] #5503
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Caller
participant F as insert_documents
participant G as ObjectId Generator
participant M as MongoDB Collection
U->>F: Call insert_documents(docs, coll, ...)
loop Process each document
F->>F: Remove existing _id if present
F->>G: Generate new ObjectId
G-->>F: Return new _id
F->>F: Prepend new _id to document
end
F->>M: Insert modified documents batch
M-->>F: Return inserted document IDs
F->>U: Return list of new _id values
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fiftyone/core/odm/database.py (1)
959-964: Well-structured solution for preventing duplicate key errors!The changes follow the MongoDB specification for document insertion by ensuring that:
- Each document has a unique
_id- The
_idfield appears first in the document structureThis approach effectively prevents duplicate key errors that can occur during batch insertions.
Consider adding input validation.
To make the function more robust, consider adding validation for the input documents:
for i, doc in enumerate(batch): + if not isinstance(doc, dict): + raise ValueError(f"Document at index {i} must be a dict, got {type(doc)}") _id = doc.pop("_id", ObjectId()) ids.append(_id) batch[i] = {"_id": _id, **doc}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/core/odm/database.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: build
what does this mean? |
| batch = list(batch) | ||
| # Modify each document client-side and ensure _id exists and is first | ||
| # https://github.com/mongodb/specifications/blob/master/source/crud/bulk-write.md#insert | ||
| for i, doc in enumerate(batch): |
There was a problem hiding this comment.
Can we parallelize this :P (jk haha!)
|
Damn how do you figure this out |
|
What changes are proposed in this pull request?
Add _id first to new docs instead of letting the pymongo client doing it to avoid duplicate key errors if the internal batching doesn't align with the batching done in the db client
With static batcher batch size set to 100k, progress stalls at 100k and throws bulk write error:
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changes