Update indexes when renaming/deleting fields #2
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
|
Clone of the PR voxel51/fiftyone#4698 |
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
WalkthroughThe recent modifications enhance the management of schema paths and indexes in the dataset, specifically through the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dataset
participant IndexManager
User->>Dataset: Modify fields
Dataset->>IndexManager: Update schema paths
IndexManager->>Dataset: Reload fields
IndexManager->>IndexManager: Determine index updates
IndexManager->>Dataset: Renaming/deleting indexes
User->>Dataset: Verify changes
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 as PR comments)
Additionally, you can add Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- fiftyone/core/odm/mixins.py (10 hunks)
- tests/unittests/dataset_tests.py (1 hunks)
Additional context used
Ruff
fiftyone/core/odm/mixins.py
1140-1140: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (9)
fiftyone/core/odm/mixins.py (8)
483-487: Field reloading after schema path processing is correctly placed.The reloading of fields after processing schema paths ensures that the schema is up-to-date before any further operations.
569-573: Field reloading after schema path processing is correctly placed.The reloading of fields after processing schema paths ensures that the schema is up-to-date before any further operations.
691-692: Field deletion is correctly handled.The deletion of fields is correctly handled, ensuring that schema paths are processed before any further operations.
1129-1135: Index renaming logic is correctly implemented.The logic for renaming indexes by dropping and recreating them is correctly implemented, considering MongoDB's limitations.
1137-1142: Index deletion logic is correctly implemented.The logic for deleting indexes is correctly implemented, ensuring that outdated indexes are removed.
Tools
Ruff
1140-1140: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
1737-1785: Index update logic is correctly implemented.The logic for determining necessary index updates based on field path changes is correctly implemented, handling both renaming and deletion scenarios.
707-709: Ensure index deletion is correctly handled.The addition of index deletion after field deletion is a good change. Ensure that all indexes are correctly removed from the database.
Run the following script to verify that index deletion is correctly handled:
Verification successful
Index deletion is correctly handled and tested.
The
_delete_indexesmethod is implemented to drop indexes from the dataset, and there is a corresponding test intests/unittests/dataset_tests.pyto verify this functionality.
- Method implementation:
fiftyone/core/odm/mixins.py- Test function:
tests/unittests/dataset_tests.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that index deletion is correctly handled. # Test: Search for the `_delete_indexes` method usage. Expect: Correct handling of index deletion. rg --type python -A 5 $'_delete_indexes'Length of output: 70
Script:
#!/bin/bash # Description: Verify that index deletion is correctly handled. # Test: Search for the `_delete_indexes` method usage. Expect: Correct handling of index deletion. rg --type py -A 5 $'_delete_indexes'Length of output: 1160
501-503: Ensure index renaming is correctly handled.The addition of index renaming after schema path processing is a good change. Ensure that all indexes are correctly updated in the database.
Run the following script to verify that index renaming is correctly handled:
Verification successful
Index renaming is correctly handled.
The
_rename_indexesmethod infiftyone/core/odm/mixins.pycorrectly updates indexes by dropping old ones and creating new ones based on the provided paths. This ensures that all indexes are updated in the database as intended.
- Location:
fiftyone/core/odm/mixins.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that index renaming is correctly handled. # Test: Search for the `_rename_indexes` method usage. Expect: Correct handling of index renaming. rg --type python -A 5 $'_rename_indexes'Length of output: 70
Script:
#!/bin/bash # Description: Verify that index renaming is correctly handled. # Test: Search for the `_rename_indexes` method usage. Expect: Correct handling of index renaming. rg --type py -A 5 $'_rename_indexes'Length of output: 853
tests/unittests/dataset_tests.py (1)
2217-2275: LGTM! The test logic is comprehensive.The test method thoroughly covers the scenarios of renaming and deleting indexes, ensuring the integrity of index management in the dataset.
| def _delete_indexes(cls, paths): | ||
| updates = _get_index_updates(cls._dataset, paths) | ||
|
|
||
| for name in updates.keys(): |
There was a problem hiding this comment.
Optimize dictionary key check.
Use key in dict instead of key in dict.keys() for better performance.
Use this diff to optimize the code:
- for name in updates.keys():
+ for name in updates:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for name in updates.keys(): | |
| for name in updates: |
Tools
Ruff
1140-1140: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
| def test_rename_delete_indexes(self): | ||
| sample = fo.Sample( | ||
| filepath="video.mp4", | ||
| field=1, | ||
| predictions=fo.Detections(detections=[fo.Detection(field=1)]), | ||
| ) | ||
| sample.frames[1] = fo.Frame( | ||
| field=1, | ||
| predictions=fo.Detections(detections=[fo.Detection(field=1)]), | ||
| ) | ||
|
|
||
| dataset = fo.Dataset() | ||
| dataset.add_sample(sample, dynamic=True) | ||
|
|
||
| dataset.create_index("field") | ||
| dataset.create_index([("id", 1), ("field", -1)]) | ||
| dataset.create_index("predictions.detections.field") | ||
|
|
||
| dataset.create_index("frames.field") | ||
| dataset.create_index([("frames.id", 1), ("frames.field", -1)]) | ||
| dataset.create_index("frames.predictions.detections.field") | ||
|
|
||
| index_info = dataset.get_index_information() | ||
|
|
||
| self.assertIn("field", index_info) | ||
| self.assertIn("_id_1_field_-1", index_info) | ||
| self.assertIn("predictions.detections.field", index_info) | ||
|
|
||
| self.assertIn("frames.field", index_info) | ||
| self.assertIn("frames._id_1_field_-1", index_info) | ||
| self.assertIn("frames.predictions.detections.field", index_info) | ||
|
|
||
| dataset.rename_sample_fields({"field": "f", "predictions": "p"}) | ||
| dataset.rename_frame_fields({"field": "f", "predictions": "p"}) | ||
|
|
||
| index_info = dataset.get_index_information() | ||
|
|
||
| self.assertIn("f", index_info) | ||
| self.assertIn("_id_1_f_-1", index_info) | ||
| self.assertIn("p.detections.field", index_info) | ||
|
|
||
| self.assertIn("frames.f", index_info) | ||
| self.assertIn("frames._id_1_f_-1", index_info) | ||
| self.assertIn("frames.p.detections.field", index_info) | ||
|
|
||
| dataset.delete_sample_fields(["f", "p"]) | ||
| dataset.delete_frame_fields(["f", "p"]) | ||
|
|
||
| indexes = dataset.list_indexes() | ||
|
|
||
| self.assertSetEqual( | ||
| set(indexes), | ||
| { | ||
| "id", | ||
| "filepath", | ||
| "frames.id", | ||
| "frames._sample_id_1_frame_number_1", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Consider adding assertions to verify the absence of old indexes.
The test method effectively verifies the presence of new indexes after renaming and deletion. However, it would be beneficial to add assertions to ensure that old indexes are no longer present after these operations.
You can add assertions like the following to verify the absence of old indexes:
self.assertNotIn("field", index_info)
self.assertNotIn("predictions.detections.field", index_info)
self.assertNotIn("frames.field", index_info)
self.assertNotIn("frames.predictions.detections.field", index_info)
|
/review |
PR Reviewer Guide 🔍
|
When renaming and deleting fields, any indexes that touch the affected fields should also be updated, just like how
dataset.app_configis automatically synchronized.Note: unfortunately MongoDB does not support renaming indexes; they must be dropped and recreated
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Description by Korbit AI
What change is being made?
Update the code to handle index renaming and deletion when fields are renamed or deleted, and add corresponding unit tests.
Why are these changes being made?
This change addresses the issue where indexes were not being updated when fields were renamed or deleted, as reported in issue #3185. The new approach ensures that indexes are correctly managed, maintaining database integrity and improving the robustness of field operations.