setup Gridify filters for entries #1524
Conversation
WalkthroughThis update introduces new filtering functionality within the MiniLcm API. A new test class validates query entries and filtering behavior based on Gridify criteria. The API now applies custom filtering using a new Gridify mapper and extended configuration in the kernel. Additionally, package references in the project files have been updated to support the added functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as CrdtMiniLcmApi
participant Mapper as EntryFilter.Mapper
participant DB as Database
C->>API: GetEntries(QueryOptions with Filter)
API->>API: Check for GridifyFilter in options
API->>Mapper: ApplyFiltering(GridifyFilter)
Mapper-->>API: Return filtered query
API->>DB: Execute filtered query
DB-->>API: Return matching entries
API-->>C: Deliver filtered entries
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
370-373: Good implementation of filtering with Gridify.The implementation correctly applies the Gridify filter when present using the
EntryFilter.Mapper. The null check ensures backward compatibility.Since this is a core feature of the PR, consider adding documentation comments to clarify how filters are applied and the expected format of filter strings as described in the PR objectives.
+ // Apply entry filtering using Gridify if a filter is specified + // Supports filtering syntax like: + // - Senses=null (entries with no senses) + // - Senses.ExampleSentences=null (entries with senses that lack example sentences) + // - LexemeForm[en]=*nan (lexeme forms in English that contain "nan") if (options.Filter?.GridifyFilter != null) { queryable = queryable.ApplyFiltering(options.Filter.GridifyFilter, EntryFilter.Mapper); }backend/FwLite/MiniLcm/Filtering/EntryFilter.cs (1)
27-31: Improve error handling in ConvertNullToEmptyListThe method currently only handles "null" or empty string values, throwing an exception for all other inputs. This could lead to unexpected runtime errors.
Consider enhancing the error handling to be more robust:
public static object ConvertNullToEmptyList<T>(string value) { - if (value is "null" or "") return new List<T>(); - throw new Exception($"Invalid value {value} for {typeof(T).Name}"); + if (value is "null" or "" or null) return new List<T>(); + try { + // Try to parse as a list if not null/empty + return JsonSerializer.Deserialize<List<T>>(value) ?? new List<T>(); + } catch (Exception ex) { + throw new Exception($"Invalid value '{value}' for {typeof(T).Name}. Expected null, empty string or valid JSON list.", ex); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/FwLite/LcmCrdt.Tests/MiniLcmTests/QueryEntryTests.cs(1 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(3 hunks)backend/FwLite/LcmCrdt/LcmCrdt.csproj(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtKernel.cs(2 hunks)backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs(1 hunks)backend/FwLite/MiniLcm/Filtering/EntryFilter.cs(1 hunks)backend/FwLite/MiniLcm/IMiniLcmReadApi.cs(2 hunks)backend/FwLite/MiniLcm/MiniLcm.csproj(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (15)
backend/FwLite/LcmCrdt/LcmCrdt.csproj (1)
12-12: Package addition aligns with PR objective.The addition of the Gridify.EntityFramework package supports the implementation of advanced filtering capabilities as described in the PR objectives.
backend/FwLite/MiniLcm/IMiniLcmReadApi.cs (2)
2-2: Appropriate namespace import for filtering.The import for the filtering namespace aligns with the new filter parameter added to QueryOptions.
29-30: Good implementation of the filter parameter.Adding the filter as an optional parameter with a default null value maintains backward compatibility while enabling the new filtering capability described in the PR objectives.
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (2)
3-3: Required imports for Gridify functionality.Appropriate namespaces are imported to support the new filtering functionality.
Also applies to: 19-19
370-373:❓ Verification inconclusive
Verify filter performance on large datasets.
Ensure that the filtering implementation performs well with large datasets. The current implementation looks correct but we should verify that it doesn't introduce performance issues.
🏁 Script executed:
#!/bin/bash # Look for any performance tests related to the new filtering functionality echo "Checking for performance tests related to filtering..." rg --type cs "performance|benchmark" --context 10 | grep -E "Filter|Gridify" # Check if there are any usage examples in the codebase echo "Checking for filter usage examples..." rg --type cs "GridifyFilter" --context 5Length of output: 9278
Performance Evaluation of Gridify Filtering Implementation
- The filtering code in
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(lines 370-373) appears correct and is validated by several functional tests inbackend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs.- However, no dedicated performance or benchmark tests targeting large datasets were identified. While the functional tests confirm expected behavior, we should verify scalability and performance under load.
- Please consider incorporating performance benchmarks or load tests to ensure that filtering remains efficient when handling larger datasets.
backend/FwLite/MiniLcm/MiniLcm.csproj (1)
9-10:❓ Verification inconclusive
Verify compatibility with these package versions
The addition of Gridify 2.16.2 looks good for implementing the filtering functionality. However, Microsoft.Extensions.DependencyInjection.Abstractions version 9.0.0 seems quite high. This might be a preview or very recent release.
🏁 Script executed:
#!/bin/bash # Check for latest stable versions of these packages dotnet list package --outdated # Alternatively, check NuGet directly curl -s https://api.nuget.org/v3-flatcontainer/gridify/index.json | jq '.versions | .[-1]' curl -s https://api.nuget.org/v3-flatcontainer/microsoft.extensions.dependencyinjection.abstractions/index.json | jq '.versions | .[-1]'Length of output: 397
Action: Confirm Dependency Stability and Compatibility
The addition of Gridify 2.16.2 is confirmed to be the latest stable release and works well for the filtering functionality. For Microsoft.Extensions.DependencyInjection.Abstractions, note that while NuGet now lists a preview version (10.0.0-preview.1.25080.5), the project is set to use version 9.0.0—which appears to be the latest stable release. Please verify that using the stable 9.0.0 release aligns with your project’s target platform and dependency requirements, and that no compatibility issues arise from not upgrading to the preview version.
- File: backend/FwLite/MiniLcm/MiniLcm.csproj, Lines 9-10
backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (3)
37-44: Good implementation of Gridify mappingsThe mappings for Entry properties look good and correctly handle JSON value extraction. The mappings for nested properties like Senses.Gloss and Senses.Definition are well-structured.
45-48: Properly handled null to empty list conversionThe mappings for ComplexFormTypes and SemanticDomains correctly use the ConvertNullToEmptyList method to ensure null values are treated as empty lists, which is a good practice.
44-44:✅ Verification successful
Consider the implications of disabling collection null checks
Disabling collection null checks might lead to null reference exceptions if collections are null during filtering operations. Ensure that null collections are properly handled elsewhere in the code.
🏁 Script executed:
#!/bin/bash # Check for potential null dereference issues rg -l "\.Senses\." --type cs | xargs rg -l "null" | xargs catLength of output: 82283
Action: Confirming Proper Null Handling for Collection Mappings
After reviewing the code and verifying against our tests:
- The configuration disables collection null checks intentionally so that JSON columns representing lists (like complex form types and semantic domains) are interpreted via the conversion function.
- The conversion function
ConvertNullToEmptyList<T>is registered for these mappings. It explicitly converts"null"or an empty string into an empty list, ensuring that null values are replaced rather than causing dereference issues.- The available unit tests and integration scenarios do not indicate any unexpected null reference exceptions.
It appears that null collections are intentionally handled and that the disabled null checks setting does not introduce risks under current usage.
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (3)
3-8: Well-structured test class setupThe abstract test class with test data fields is a good approach for creating reusable test scenarios.
9-52: Comprehensive test data initializationThe InitializeAsync method creates a good test dataset with varying properties that will allow testing different filtering scenarios. Including entries with and without senses, part of speech, examples, semantic domains, etc. provides good coverage.
54-129: Excellent test coverage for filtering capabilitiesThe tests thoroughly validate all the filtering capabilities mentioned in the PR objectives, including:
- Null checks for senses, part of speech, examples, semantic domains, and complex form types
- Exact, starts with, ends with, and contains filtering for lexeme forms
- Null and empty checks for glosses
This is a comprehensive set of tests that should ensure the filtering functionality works as expected.
backend/FwLite/MiniLcm/Filtering/EntryFilter.cs (3)
8-11: Address the TODO regarding shared mapper instancesThere's a TODO comment indicating that the static mapper shouldn't be shared between IMiniLcm instances. This could lead to issues in multi-tenant scenarios or concurrent operations.
Consider implementing a factory pattern or instance-specific mappers to address this concern.
12-24: Well-organized mapping configurationThe static constructor properly sets up all the necessary mappings for both direct properties and nested properties. The approach is consistent and handles the various property types correctly.
33-33: Property for filter string looks goodThe nullable string property for GridifyFilter is appropriate for the use case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/FwLite/FwDataMiniLcmBridge/Api/MorphoSyntaxExtensions.cs (1)
53-56: Consider adding a null check for consistency with similar methods.Other extension methods in this file use
ArgumentNullException.ThrowIfNull(msa). Including a similar check here would ensure consistent null protection and clarity.+ ArgumentNullException.ThrowIfNull(msa); return msa.GetPartOfSpeech()?.Guid;backend/FwLite/FwDataMiniLcmBridge/FwDataBridgeConfig.cs (1)
14-16: Helper methods for converting empty collections to null are effective.Converting empty lists to null enables straightforward filtering with conditions like
Senses=null. Be aware of potential confusion if other parts of the code expect empty instead of null. Document this behavior clearly.backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (3)
75-78: Consider adding error handling for non-existent writing systems.The method looks good overall, but it doesn't handle the case where the writing system with the specified handle doesn't exist. Consider adding a null check before accessing the
Idproperty.internal static WritingSystemId GetWritingSystemId(this LcmCache cache, int ws) { - return cache.ServiceLocator.WritingSystemManager.Get(ws).Id; + var writingSystem = cache.ServiceLocator.WritingSystemManager.Get(ws); + if (writingSystem == null) + { + throw new KeyNotFoundException($"Writing system with handle {ws} not found."); + } + return writingSystem.Id; }
80-113: Consider using a more specific exception type for missing writing systems.The method handles various edge cases well with appropriate exception messages. However,
NullReferenceExceptionis typically used for unexpected null values rather than for domain-specific errors like missing writing systems. Consider using a more appropriate exception type likeKeyNotFoundExceptionorArgumentException.if (lcmWs is null) { - throw new NullReferenceException($"unable to find writing system with id {ws}"); + throw new KeyNotFoundException($"Unable to find writing system with id {ws}"); }
115-119: Method looks good, but consider adding null parameter validation.The
PickTextmethod looks good and properly handles the case where no text is found for the writing system. Consider adding null parameter validation for robustness.internal static string? PickText(this ICmObject obj, ITsMultiString multiString, string ws) { + if (obj == null) throw new ArgumentNullException(nameof(obj)); + if (multiString == null) throw new ArgumentNullException(nameof(multiString)); + if (string.IsNullOrEmpty(ws)) throw new ArgumentException("Writing system ID cannot be null or empty", nameof(ws)); + var wsHandle = obj.Cache.GetWritingSystemHandle(ws); return multiString.get_String(wsHandle)?.Text ?? null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/QueryEntryTests.cs(1 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(4 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/MorphoSyntaxExtensions.cs(1 hunks)backend/FwLite/FwDataMiniLcmBridge/FwDataBridgeConfig.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs(3 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(3 hunks)backend/FwLite/LcmCrdt/LcmCrdtConfig.cs(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtKernel.cs(4 hunks)backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs(1 hunks)backend/FwLite/MiniLcm/Filtering/EntryFilter.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/FwLite/LcmCrdt/LcmCrdtKernel.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (23)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/QueryEntryTests.cs (1)
1-12: Looks good! Ensure sufficient coverage in derived tests.This new test class inherits from
QueryEntryTestsBase, which likely contains the core test logic. If additional scenarios or filtering behaviors need to be validated for this specific fixture, consider adding custom tests here.backend/FwLite/LcmCrdt/LcmCrdtConfig.cs (3)
1-2: Imports confirmed.These new
GridifyandMiniLcm.Filteringimports are necessary for the mapper functionality and filtering logic. No issues identified.
8-24: Mapping configuration appears coherent.The constructor ensures
Mapper.Configuration.DisableCollectionNullChecks = true, then sets up custom mappings for variousEntryproperties. The definitions look consistent and should handle null/empty behaviors effectively.
27-27: Mapper property usage is correct.Exposing
Mappervia a public property is in line with the rest of the design. This allows other components or tests to access and adjust mapped configurations if necessary.backend/FwLite/FwDataMiniLcmBridge/FwDataBridgeConfig.cs (3)
3-7: New using directives are valid.These imports from
FwDataMiniLcmBridge.Api,Gridify,MiniLcm.Models, andSIL.LCModelare needed to support the filtering and bridging functionalities introduced in this configuration file.
18-38: Constructor mappings align with advanced filtering needs.Each
Mapper.AddMapinvocation properly defines how variousEntryfields and related data (e.g.,Senses,ComplexFormTypes,PartOfSpeechId) are transformed, ensuring the Gridify filter can handle nulls and empties as intended. The logic appears consistent with the broader design.
71-71: Dedicated mapper instance is appropriate.Defining
Mapperas a public property with a newGridifyMapper<ILexEntry>instance allows the rest of the codebase to leverage these filtering transformations seamlessly. This is a clean approach to centralize mapping logic.backend/FwLite/MiniLcm/Filtering/EntryFilter.cs (1)
1-37: Well-structured filter mapper implementationThe EntryFilter class provides a clean implementation for mapping Gridify filter expressions to Entry properties. The mapper configuration allows for flexible querying of both direct properties and nested collections.
The implementation:
- Adds mappings for simple properties like
ComplexFormTypesandSenses- Handles nested properties like
Senses.Glosswith lambda selectors- Includes a utility method to convert null strings to empty lists
This approach effectively enables the string-based filtering capabilities described in the PR objectives.
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (6)
3-3: Added Gridify namespace for filtering functionalityThe Gridify namespace import is required for the new filtering capabilities.
13-13: Added Microsoft.Extensions.Options for configuration injectionThis import allows for dependency injection of the configuration options.
20-20: Added MiniLcm.Filtering for EntryFilter integrationThis namespace import provides access to the newly created EntryFilter class.
24-29: Updated constructor to include configuration parameterThe constructor now accepts an IOptions parameter, allowing the API to access the filter mapper configuration.
33-33: Added property to access configuration valueThis private property provides convenient access to the injected configuration.
377-380: Implemented Gridify filtering for entriesThe implementation applies the GridifyFilter to the query when a filter is specified in the options. This allows for the string-based filtering described in the PR objectives.
The filter is applied using the mapper from the configuration, ensuring consistency in how filters are interpreted.
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (4)
6-6: Added Gridify namespace for filtering functionalityThis import enables the use of Gridify filtering capabilities in the API.
8-8: Added Microsoft.Extensions.Options for configuration injectionThis import allows for dependency injection of the configuration options.
23-31: Updated constructor and added config propertyThe constructor now accepts an IOptions parameter, and a private property has been added to access the configuration value. This enables the API to use the filter mapper defined in the configuration.
690-695: Implemented Gridify filtering for entriesThe implementation creates a GridifyQuery with the filter string and uses the mapper from the configuration to generate and apply a filtering expression.
Note that this implementation differs slightly from the one in CrdtMiniLcmApi, as it compiles the expression and applies it directly to the entries collection rather than using the
ApplyFilteringextension method. This difference is appropriate given the different collection types being operated on.backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs (4)
7-7: Added Microsoft.Extensions.Options for configuration injectionThis import allows for dependency injection of the configuration options.
13-19: Updated primary constructor to include configuration parameterThe primary constructor now accepts the IOptions parameter, enabling propagation of the configuration to dependent components.
22-28: Updated secondary constructor for configuration parameterThe secondary constructor has been modified to chain to the primary constructor with the configuration parameter, maintaining consistency in initialization.
42-42: Passing configuration to FwDataMiniLcmApi constructorThe configuration is now properly passed to the FwDataMiniLcmApi when creating a new instance, ensuring the filter mapper is available for entry filtering.
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (1)
2-3: Added appropriate namespaces for new functionality.The additional namespaces support the new writing system methods being added to this class.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
This looks pretty awesome.
I wanted to play with it more, but didn't quite get to it.
I don't think you have any tests/examples that filter on entries with multiple senses, so it's not clear to me what would happen. If I do a search on gloss, is it an ALL or ANY? It should probably be an ANY. I'm not sure if that's the case.
I'm also thinking, we might want to filter on the "main sense" at some point. I don't think we want to add that to this PR, but I'm glad that also seems to be well supported.
|
Good question, the way it handles lists/arrays is by transforming Where(e => eSenses.Select(s => s.Gloss["en"]).Contains("test"))and Where(e => eSenses.Select(s => s.Gloss["en"]).Any(g => g.EndsWith("test"))) |
|
after chatting with Tim, we decided we should have an abstract class or interface for providing filter mappings. This will ensure that all filters are always mapped |
… whenever a new property is supported
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (2)
149-149: Document the/imodifier for case-insensitive filtering.The filter
LexemeForm[en]=*a/iincludes a/imodifier which appears to enable case-insensitive matching, but this feature isn't mentioned in the PR description. Consider adding a comment explaining this functionality for better documentation.
146-159: Consider adding more test cases for complex filtering scenarios.The test suite covers many filtering scenarios including AND conditions with the
,operator, but could be enhanced with:
- Tests for OR conditions using the
|operator mentioned in the PR description- Tests combining both AND and OR operators
- Tests for more complex patterns or edge cases
These additional test cases would ensure the filtering system is robust and fully validated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(2 hunks)backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Filtering/IEntryFilter.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/IQueryOptions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Filtering/IEntryFilter.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/IQueryOptions.ts (1)
7-7: Adding filter capability to IQueryOptions aligns with PR objectives.The addition of the
IEntryFilterimport and thefilterproperty to theIQueryOptionsinterface successfully implements the Gridify filtering capability mentioned in the PR objectives. This change enables the advanced filtering syntax described in the PR summary (e.g.,Senses=null,LexemeForm[en]=*nan).Since this is a generated file (as noted in the file header comments), ensure that the generation template has been updated accordingly to avoid these changes being overwritten in future regenerations.
Also applies to: 15-15
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (2)
10-10: Appropriate import for entry filtering functionalityThe addition of this namespace import is necessary to access the
EntryFilterclass that's being added to the TypeScript exports.
82-82: Well-integrated export of the EntryFilter interfaceGood addition of the
EntryFiltertype to the exported interfaces. This properly extends the TypeScript type system to support the new Gridify filtering capabilities mentioned in the PR description, allowing frontend code to leverage the string-based filtering syntax (e.g.,Senses=null,LexemeForm[en]=*nan, etc.).The implementation correctly follows the existing pattern used for related types like
QueryOptionsandSortOptions.backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (2)
1-60: Well-structured test setup with comprehensive data coverage.The initialization code creates a good variety of test data with different attributes (entries with/without senses, complex form types, semantic domains, etc.) that allows for thorough testing of the Gridify filtering functionality. The test data structure aligns well with the filtering requirements described in the PR objectives.
69-74:❓ Verification inconclusive
Verify filter behavior for nested collections.
The filters
Senses.PartOfSpeechId=nullandSenses.SemanticDomains=nullonly match the Peach entry, not the Banana entry, even though Banana has a second sense without these attributes. This suggests these filters match entries where all senses lack the attribute, not where any sense lacks it. This behavior differs from how filters likeSenses.Gloss[en]=testare described in the PR comments (which checks if any sense matches).Also applies to: 83-88
🏁 Script executed:
#!/bin/bash # Find the Gridify mapper implementation to understand how these filters are applied rg -A 10 "class.*Gridify.*Mapper" --type csharpLength of output: 49
I'm not seeing any output from the initial search for "class.*Gridify.*Mapper." To get a more comprehensive view, let's try broadening the search to capture any references to gridify in our C# files. This will help us understand how the filter is implemented and whether the nested collection behavior is as described.
Please run the following shell script:
This query uses a case-insensitive search across all C# files. Once we review its output, we can decide if manual verification of the Gridify filter implementation is required or if we need to adjust the test expectations.
🏁 Script executed:
#!/bin/bash rg -i "gridify" --type csharpLength of output: 4172
Action Required: Confirm Intended Filter Semantics for Nested Collections
It appears that our current implementation for filters like
Senses.PartOfSpeechId=nullandSenses.SemanticDomains=nullin the tests returns only the Peach entry. This indicates that—unlike filters such asSenses.Gloss[en]=test—the null-check filters are operating under an “all senses must lack the attribute” rule rather than an “any sense missing the attribute” rule.
- Observation: The test (
CanFilterToMissingPartOfSpeech) confirms that only entries where every sense lacks the attribute (Peach) are matched.- Concern: This behavior does not align with the PR commentary for filters (e.g.,
Senses.Gloss[en]=test), which suggests a match when any sense qualifies.- Next Steps: Please verify whether the current Gridify filtering implementation and its mapper (referenced in
backend/FwLite/Filtering/EntryFilter.csand used throughout the codebase) are intended to behave with an “all” semantics for null checks. If the intended behavior is indeed “any”, adjustments in the filtering logic or the test cases may be necessary.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Looks good to me. 🤩
I tried to add a SemanticDomain code filter and failed.
I'm not sure what's wrong, but it fails for FwData and Harmony 🤦.
I'm gonna open a draft PR and maybe you can explain what's wrong.
closes #1018
Adds rich filtering using Gridify, it uses a string syntax to convert into an expression which can be used for filtering
Some examples:
Senses=nullSenses.ExampleSentences=nullSenses.PartOfSpeechId=nullnan:LexemeForm[en]=*nanLexemeForm[en]=appleSenses.Gloss[en]=FruitLexemeForm[en]=*a, ComplexFormTypes=null, lexeme contains A and has no form types,,is and,|is orOne thing to note, this only filters Entries, it does not filter the list of senses on a given entry.
This is meant to be a low level API, some cases like
Senses.PartOfSpeechId=nullshould probably be combined withSenses=nullto makeSenses=null|Senses.PartOfSpeechId=nullSummary by CodeRabbit
New Features
Chores