-
-
Notifications
You must be signed in to change notification settings - Fork 123
feat: automatic merging for concurrent container inserts in maps #912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: automatic merging for concurrent container inserts in maps #912
Conversation
f857161 to
3395956
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8571612a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Set Parent in Arena | ||
| let child_idx = ans.idx(); | ||
| inner | ||
| .doc | ||
| .arena | ||
| .set_parent(child_idx, Some(inner.container_idx)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treat mergeable Root IDs as parentless
This explicit parent assignment won’t be observable because mergeable containers use ContainerID::Root and SharedArena::get_parent short‑circuits all root IDs to None. As a result, path/ancestor logic (e.g. DocState::get_path used by getPathToContainer and event path) will still treat mergeable containers as top‑level roots even after this call, so a getMergeableList created under a map will report a root path like cid:root-…/key instead of the map key path. That’s a functional regression for path‑based lookups/subscriptions; consider updating get_parent (and depth/root handling) to honor is_mergeable() root IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in latest push.
|
Should there also be a EDIT: Added getMergeableCounter |
3395956 to
9348727
Compare
Completes #759
Summary of Changes
Core Logic (
crates/loro-internal/src/handler.rs):get_or_create_mergeable_containerandinsert_mergeable_container_with_txninMapHandler.RootContainer ID using the formatparent_id/key. This ensures that concurrent creations by different peers result in the same Container ID, allowing their operations to merge naturally.Serialization (
crates/loro-internal/src/state.rs):get_value,get_deep_value, andget_deep_value_with_idto filter out these mergeable containers from the top-level root container list. They are identified by the reserved/character in their name. This ensures they only appear nested under their parent Map, not as detached root documents.Rust API (
crates/loro-internal/src/handler.rs):get_mergeable_list,get_mergeable_map,get_mergeable_movable_list,get_mergeable_text, andget_mergeable_treetoMapHandler.WASM/JS API (
crates/loro-wasm/src/lib.rs):LoroMap:getMergeableList(key)getMergeableMap(key)getMergeableMovableList(key)getMergeableText(key)getMergeableTree(key)crates/loro-wasm/src/lib.rs.Testing (
crates/loro-internal/tests/mergeable_container.rs):Map -> MergeableMap -> MergeableMap -> MergeableListzipper together correctly.Key Observations
LoroMapchildren, as it relies on stable keys for ID generation./, creating a safe namespace for these system IDs.Why
/is better than:Reserved Namespace:
/character (or\0). This is checked bycheck_root_container_name.:character, however, is allowed in user-created root container names.Preventing Collisions:
:, a user could theoretically create a root container with a name likecid:10@100:Map:myKey. If this matches our internal format, the system might confuse the user's root container with a mergeable child container, leading to unpredictable behavior./(e.g.,cid:10@100:Map/myKey), we are using a character that is strictly forbidden for users. This guarantees that our generated ID will never collide with any valid user-created root container. It effectively creates a private, reserved namespace for these internal containers.Parsing Simplicity:
name.contains('/'). Since users can't use/, any root container with a/in its name must be one of our internal mergeable containers.In summary, using
/leverages an existing system constraint to create a robust, collision-free identifier for this new feature.