Summary field and index improvements #5091
Conversation
WalkthroughThe changes introduce a new private method Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
| fo.FloatField, | ||
| fo.IntField, | ||
| fo.DateField, | ||
| fo.DateTimeField, |
There was a problem hiding this comment.
FrameNumberField could be apply here. I only note it because it came up in the lightning backend. Reported by @minhtuev
There was a problem hiding this comment.
In the backend we have the luxury of using isinstance, which understands that FrameNumberField is a subclass of IntField. So we're all set 😎
| fo.StringField, | ||
| fo.BooleanField, | ||
| fo.ObjectIdField, | ||
| ) | ||
| numeric_field_types = ( | ||
| fo.FloatField, | ||
| fo.IntField, | ||
| fo.DateField, | ||
| fo.DateTimeField, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
fiftyone/operators/builtin.py(18 hunks)
🔇 Additional comments (4)
fiftyone/operators/builtin.py (4)
1031-1041: LGTM! Clear separation of field types and support for ObjectIdField
The changes improve code organization by clearly separating field types into categorical and numeric groups, while adding support for ObjectIdField as mentioned in the PR objectives.
Also applies to: 1204-1215
1086-1086: Performance improvement: Asynchronous index creation
The addition of wait=False makes index creation asynchronous, which can improve responsiveness by not blocking the operation.
1106-1107: Bug fix: Correct frame-level default index path construction
The fix properly constructs the frame-level default index paths by correctly prefixing them, addressing the bug mentioned in the PR objectives.
1161-1170: Improved field name handling with better null checks
The changes enhance field name handling by:
- Adding a reusable
_get_dynamichelper function - Improving null checks for field names
- Making the code more maintainable
This aligns with the PR objective of improving default summary field name logic.
Also applies to: 1187-1191
| path_keys = list(schema.keys()) | ||
| path_selector = types.AutocompleteView() | ||
| for key in sorted(schema.keys()): | ||
| for key in path_keys: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated field selection logic
The same pattern for field selection appears in multiple places:
- Converting keys to a list
- Creating an AutocompleteView
- Adding choices in a loop
Consider extracting this into a helper function to reduce code duplication.
Example implementation:
def create_field_selector(keys):
"""Create an AutocompleteView for the given field keys.
Args:
keys: Iterable of field keys
Returns:
types.AutocompleteView: The configured selector
"""
field_selector = types.AutocompleteView()
for key in sorted(keys): # Sort for consistent ordering
field_selector.add_choice(key, label=key)
return field_selectorThen use it like:
- field_keys = list(schema.keys())
- field_selector = types.AutocompleteView()
- for key in field_keys:
- field_selector.add_choice(key, label=key)
+ field_selector = create_field_selector(schema.keys())Also applies to: 245-246, 373-374, 460-462, 556-558, 672-674, 772-774, 915-917, 985-987
| @@ -1125,13 +1158,16 @@ def resolve_input(self, ctx): | |||
|
|
|||
| def execute(self, ctx): | |||
| path = ctx.params["path"] | |||
| field_name = ctx.params.get("field_name", None) | |||
| _, field_name = _get_dynamic(ctx.params, "field_name", path, None) | |||
There was a problem hiding this comment.
Do we pass in any default value that is not None? We can potentially remove the arg to reduce confusion.
|
Merging so that we can create an OSS -> Teams merge PR |
Change log
ObjectIdFieldandListFieldcreate_indexoperatordrop_indexoperatorExample usage
Summary by CodeRabbit
New Features
ObjectIdField.waitparameter.Bug Fixes
Refactor