Merge release/v1.1.0 to develop
#5130
Conversation
WalkthroughThe changes in this pull request involve modifications to several files related to the filtering logic within the application. Key updates include enhancements to the Changes
Possibly related PRs
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/packages/state/src/recoil/pathData/lightningString.ts (1)
11-11: Add JSDoc comments explaining thefiltersparameterConsider adding documentation to explain:
- The purpose and expected structure of the
filtersparameter- How it affects the results
- Example usage
+ /** + * @param filters - Serializable filter criteria to narrow down the results + * @example + * { + * path: "metadata.tags", + * filters: { field: "value" } + * } + */ filters: SerializableParam;app/packages/core/src/components/Filters/StringFilter/useSelected.ts (1)
38-40: Consider extracting complex condition into a named functionThe condition for determining
useSearchis becoming complex with multiple checks. Consider extracting it into a descriptive function to improve readability and maintainability.+ const shouldDisableSearch = (path: string, queryPerformance: boolean, isFrameField: boolean, modal: boolean) => { + return path === "_label_tags" && queryPerformance && !isFrameField && !modal; + }; return { results, - useSearch: - path === "_label_tags" && queryPerformance && !isFrameField && !modal - ? undefined - : useSearch, + useSearch: shouldDisableSearch(path, queryPerformance, isFrameField, modal) ? undefined : useSearch, showSearch: Boolean(shown) && !boolean, };app/packages/core/src/components/Filters/StringFilter/state.ts (1)
71-74: Consider type safety and performance improvementsWhile the current implementation is functionally correct, consider these improvements:
- Add explicit type annotations for better type safety
- Use object spread for better performance when filtering out a single key
- const filters = Object.fromEntries( - Object.entries(get(fos.filters) || {}).filter(([p]) => p !== path) - ); + const { [path]: _, ...filters } = get(fos.filters) || {};app/packages/state/src/recoil/types.ts (1)
169-171: Consider adding JSDoc documentation for the Filter interface.The new Filter interface provides good flexibility with its index signature. However, adding documentation would help other developers understand its purpose and usage better.
Add documentation like this:
+/** + * Represents a flexible filter structure where each key can hold either a single value + * or an array of values of various primitive types. + */ export interface Filter { [key: string]: FilterValues | Array<FilterValues>; }app/schema.graphql (1)
458-458: LGTM! Consider adding documentation for the filters field.The addition of the optional
filtersfield is well-structured and maintains backward compatibility. The use of theBSONscalar type provides flexibility for complex filtering criteria.Consider adding a documentation string to describe the purpose and expected format of the filters, similar to how the
excludefield is documented. Example:input LightningPathInput { path: String! """ exclude these values from results """ exclude: [String!] = null first: Int = 200 search: String = null + """ + additional filtering criteria in BSON format + """ filters: BSON = null }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/packages/relay/src/queries/__generated__/lightningQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (7)
app/packages/core/src/components/Filters/StringFilter/state.ts(1 hunks)app/packages/core/src/components/Filters/StringFilter/useSelected.ts(1 hunks)app/packages/state/src/recoil/filters.ts(1 hunks)app/packages/state/src/recoil/pathData/lightningString.ts(1 hunks)app/packages/state/src/recoil/types.ts(1 hunks)app/schema.graphql(1 hunks)fiftyone/server/lightning.py(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/state/src/recoil/filters.ts
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/core/src/components/Filters/StringFilter/state.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Filters/StringFilter/useSelected.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/pathData/lightningString.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/types.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (15)
app/packages/state/src/recoil/pathData/lightningString.ts (1)
7-12: Verify the impact of making filters a required parameter
Adding a required filters parameter to the selector's parameter object could be a breaking change. Existing code that uses lightningStringResults will need to be updated to provide this parameter.
app/packages/core/src/components/Filters/StringFilter/useSelected.ts (1)
32-32: LGTM: Clean addition of frame field check
The addition of isFrameField check follows the established pattern and naming conventions in the codebase.
app/packages/core/src/components/Filters/StringFilter/state.ts (1)
81-81: LGTM! Verify lightningStringResults compatibility
The addition of the filters parameter is well-integrated into the existing structure.
Let's verify that the lightningStringResults selector properly handles the new filters parameter:
✅ Verification successful
The filters parameter is properly handled by lightningStringResults selector
The verification confirms that:
- The
lightningStringResultsselector is defined withfilters: SerializableParamin its parameter type - The usage in StringFilter correctly passes the filters parameter
- The implementation follows Recoil's selectorFamily pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for lightningStringResults definition and usage to ensure parameter compatibility
ast-grep --pattern 'export const lightningStringResults = selectorFamily<$_>({
$$$
get: ({ $$$, filters }) => $_
$$$
})'
Length of output: 128
Script:
#!/bin/bash
# Let's try a broader search for lightningStringResults definition
rg "lightningStringResults" -A 10
Length of output: 3545
app/packages/state/src/recoil/types.ts (2)
167-167: LGTM! Well-defined union type for filter values.
The FilterValues type effectively covers all possible primitive value types that might be needed for filtering, while keeping the type definition clean and maintainable.
167-171: Verify compatibility with existing filter implementations.
The change from CategoricalFilter<T> to this new filter structure is a significant change that could affect existing implementations.
Let's check for potential compatibility issues:
✅ Verification successful
Based on the analysis of the code changes and search results, I can now provide a final response:
Type changes are safe and consistent with the codebase
The change from CategoricalFilter<T> to the new Filter interface with FilterValues type is well-aligned with the codebase's existing filter implementations. The search results show:
- The new type definition matches existing usage patterns across the codebase
- The type is already used in key filter-related files like
filters.ts,pathFilters/, etc. - The change provides better type safety by explicitly defining allowed filter value types
- No incompatible usages were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing filter usage patterns that might need updates
# Look for existing filter usage patterns
echo "Searching for filter usage patterns..."
rg -t typescript "CategoricalFilter|Filter\s*[:<]" --glob '!{node_modules,dist}/**'
# Look for potential type errors in filter implementations
echo "Searching for filter implementations..."
ast-grep --pattern 'interface $name implements Filter {
$$$
}'
Length of output: 363
Script:
#!/bin/bash
# Let's try a different approach to search for filter usage patterns
# Search for Filter type usage
echo "Searching for Filter type usage..."
rg "Filter" -A 3 -B 3 --type ts
# Search for specific filter value patterns
echo "Searching for filter value assignments..."
rg ":\s*(string|boolean|number|null)\s*\[\s*\]" --type ts
# Look for array filter values
echo "Searching for array filter values..."
rg "Array<.*>" --type ts
Length of output: 333418
fiftyone/server/lightning.py (10)
25-27: Appropriate Addition of Necessary Imports
The added imports of BSON and get_view are necessary for the new functionality involving filters and are correctly included.
160-160: Consistent Addition of filters to DistinctQuery
The filters field is added to the DistinctQuery class, aligning with the changes made in LightningPathInput. This ensures that filter parameters propagate correctly through the query processing logic.
Line range hint 265-281: Proper Propagation of filters in Query Preparation
Assigning d["filters"] = path.filters correctly propagates the filters parameter when preparing the query dictionary. This ensures that filters provided by the user are included in the query execution.
292-295: Update Function _do_async_pooled_queries Signature to Include dataset
The function _do_async_pooled_queries now includes dataset as a parameter, allowing for proper context during asynchronous query execution. Ensure all calls to this function are updated accordingly.
306-306: Update Function _do_async_query Signature to Include dataset
By adding dataset to _do_async_query, the function now has access to the dataset context necessary for executing queries with filters. Verify that all invocations of this function pass the correct dataset argument.
314-314: Pass dataset to _do_distinct_pipeline in _do_async_query
Passing dataset to _do_distinct_pipeline ensures that the dataset context is available when constructing the aggregation pipeline, particularly when applying filters.
354-356: Integrate filters into the Aggregation Pipeline in _do_distinct_pipeline
The code correctly incorporates filters into the aggregation pipeline using:
if query.filters:
pipeline += get_view(dataset, filters=query.filters)._pipeline()This allows the filters to be applied effectively in the distinct pipeline.
368-371: Ensure Function Signatures of _match_arrays and _unwind Include dataset
The dataset parameter is now passed to _match_arrays and _unwind. Confirm that the function definitions for these helper functions have been updated accordingly and that they utilize the dataset parameter as intended.
42-42:
Ensure Secure Handling of filters Input in LightningPathInput
By adding filters: t.Optional[BSON] = None to LightningPathInput, the code now accepts user-provided BSON filters. Verify that these filters are properly validated and sanitized to prevent security vulnerabilities such as injection attacks.
Consider auditing the get_view function to ensure it safely processes the filters input without exposing the system to injection risks.
360-367:
Handle Potential Exceptions When Constructing ObjectId from query.search
When constructing an ObjectId using ObjectId(f"{query.search}{add}"), ensure that query.search contains valid hexadecimal characters and an appropriate length. Invalid input could raise a bson.errors.InvalidId exception.
Apply this diff to validate query.search before constructing the ObjectId:
if query.search:
if query.is_object_id_field:
add = (_TWENTY_FOUR - len(query.search)) * "0"
- value = {"$gte": ObjectId(f"{query.search}{add}")}
+ try:
+ obj_id = ObjectId(f"{query.search}{add}")
+ value = {"$gte": obj_id}
+ except bson.errors.InvalidId:
+ # Handle invalid ObjectId input gracefully
+ value = {"$gte": ObjectId("0" * _TWENTY_FOUR)}
else:
value = Regex(f"^{query.search}")Alternatively, validate query.search to ensure it's a valid hexadecimal string before using it to construct an ObjectId.
extend compatibility of array deserialization in lookers
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/looker/src/worker/deserializer.ts (2)
3-14: Consider strengthening type safetyThe function implementation is clean and effectively centralizes the mask extraction logic. However, the type safety could be improved.
Consider this type-strengthened version:
-const extractSerializedMask = ( - label: object, - maskProp: string -): string | undefined => { +interface BinaryData { + $binary: { + base64: string; + }; +} + +type Label = { + [key: string]: string | BinaryData | unknown; +}; + +const extractSerializedMask = ( + label: Label, + maskProp: string +): string | undefined => {
38-41: Consider further reducing code duplicationWhile the mask extraction has been centralized, there's still significant duplication in the buffer management code across Detection, Heatmap, and Segmentation methods.
Consider extracting the common buffer management logic:
+const createBufferedMask = (serializedData: string, buffers: ArrayBuffer[]) => { + const data = deserialize(serializedData); + const [height, width] = data.shape; + const mask = { + data, + image: new ArrayBuffer(width * height * 4), + }; + buffers.push(data.buffer); + buffers.push(mask.image); + return mask; +}; Heatmap: (label, buffers) => { const serializedMask = extractSerializedMask(label, "map"); if (serializedMask) { - const data = deserialize(serializedMask); - const [height, width] = data.shape; - label.map = { - data, - image: new ArrayBuffer(width * height * 4), - }; - buffers.push(data.buffer); - buffers.push(label.map.image); + label.map = createBufferedMask(serializedMask, buffers); } },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/looker/src/overlays/segmentation.ts(1 hunks)app/packages/looker/src/worker/deserializer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/looker/src/overlays/segmentation.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/deserializer.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (3)
app/packages/looker/src/worker/deserializer.ts (2)
18-21: LGTM! Clean refactoring of mask extraction
The change successfully implements the centralized mask extraction while maintaining the existing buffer management logic.
54-57: Verify consistent behavior across all deserialization methods
The implementation is consistent with other methods. Let's verify that all methods handle the mask extraction and buffer management consistently.
✅ Verification successful
Let me analyze the code to verify the consistency of mask handling across deserialization methods.
Mask deserialization is consistent across all methods
The verification shows that all deserialization methods follow the same pattern:
- Use
extractSerializedMaskwith appropriate property name ("mask" or "map") - Call
deserializeon the extracted data - Store the result in the label's corresponding property
The implementation is consistent and safe across Detection, Segmentation, and Heatmap types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent mask handling across all methods
# Look for any inconsistencies in how masks are handled
# Search for all mask-related property accesses
rg -U "mask|map" --type ts
# Search for buffer management patterns
rg -U "ArrayBuffer|buffer" --type ts
# Look for any other deserialization methods we might have missed
ast-grep --pattern 'const $_ = deserialize($_)'
Length of output: 136882
Script:
#!/bin/bash
# Search for all deserialization methods and their implementations
ast-grep --pattern 'DeserializerFactory.$_ = ($_, $_) => {
$$$
}'
# Search for all mask extraction patterns
ast-grep --pattern 'extractSerializedMask($_, $_)'
# Search for all deserialize function calls
ast-grep --pattern 'deserialize($_)'
Length of output: 905
app/packages/looker/src/overlays/segmentation.ts (1)
276-279: LGTM! Robust error handling improvement.
The enhanced bounds checking is a solid defensive programming practice that prevents potential runtime errors from:
- Accessing an undefined targets array
- Out-of-bounds array access
This makes the code more robust and maintainable.
Merge
release/v1.1.0todevelopSummary by CodeRabbit
New Features
filtersto theLightningPathInputandDistinctQueryclasses.Improvements
Bug Fixes
getTargetmethod to prevent out-of-bounds access.